From: Fujii Masao <fujii@postgresql.org>
Date: Tue, 12 Aug 2014 07:50:09 +0000 (+0900)
Subject: Change first call of ProcessConfigFile so as to process only data_directory.
X-Git-Tag: REL9_4_BETA3~113
X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=700f060528b1485970e6414fbf96aa1f87766a40;p=postgresql

Change first call of ProcessConfigFile so as to process only data_directory.

When both postgresql.conf and postgresql.auto.conf have their own entry of
the same parameter, PostgreSQL uses the entry in postgresql.auto.conf because
it appears last in the configuration scan. IOW, the other entries which appear
earlier are ignored. But, previously, ProcessConfigFile() detected the invalid
settings of even those unused entries and emitted the error messages
complaining about them, at postmaster startup. Complaining about the entries
to ignore is basically useless.

This problem happened because ProcessConfigFile() was called twice at
postmaster startup and the first call read only postgresql.conf. That is, the
first call could check the entry which might be ignored eventually by
the second call which read both postgresql.conf and postgresql.auto.conf.
To work around the problem, this commit changes ProcessConfigFile so that
its first call processes only data_directory and the second one does all the
entries. It's OK to process data_directory in the first call because it's
ensured that data_directory doesn't exist in postgresql.auto.conf.

Back-patch to 9.4 where postgresql.auto.conf was added.

Patch by me. Review by Amit Kapila
---

diff --git a/src/backend/utils/misc/guc-file.l b/src/backend/utils/misc/guc-file.l
index e6e11090ab..bb9207a777 100644
--- a/src/backend/utils/misc/guc-file.l
+++ b/src/backend/utils/misc/guc-file.l
@@ -45,6 +45,8 @@ static unsigned int ConfigFileLineno;
 static const char *GUC_flex_fatal_errmsg;
 static sigjmp_buf *GUC_flex_fatal_jmp;
 
+static void FreeConfigVariable(ConfigVariable *item);
+
 /* flex fails to supply a prototype for yylex, so provide one */
 int GUC_yylex(void);
 
@@ -151,14 +153,66 @@ ProcessConfigFile(GucContext context)
 	 * file is in the data directory, we can't read it until the DataDir has
 	 * been set.
 	 */
-	if (DataDir &&
-		!ParseConfigFile(PG_AUTOCONF_FILENAME, NULL, false, 0, elevel,
-						 &head, &tail))
+	if (DataDir)
 	{
-		/* Syntax error(s) detected in the file, so bail out */
-		error = true;
-		ErrorConfFile = PG_AUTOCONF_FILENAME;
-		goto cleanup_list;
+		if (!ParseConfigFile(PG_AUTOCONF_FILENAME, NULL, false, 0, elevel,
+							 &head, &tail))
+		{
+			/* Syntax error(s) detected in the file, so bail out */
+			error = true;
+			ErrorConfFile = PG_AUTOCONF_FILENAME;
+			goto cleanup_list;
+		}
+	}
+	else
+	{
+		ConfigVariable *prev = NULL;
+
+		/*
+		 * Pick up only the data_directory if DataDir is not set, which
+		 * means that the configuration file is read for the first time and
+		 * PG_AUTOCONF_FILENAME file cannot be read yet. In this case,
+		 * we shouldn't pick any settings except the data_directory
+		 * from postgresql.conf because they might be overwritten
+		 * with the settings in PG_AUTOCONF_FILENAME file which will be
+		 * read later. OTOH, since it's ensured that data_directory doesn't
+		 * exist in PG_AUTOCONF_FILENAME file, it will never be overwritten
+		 * later.
+		 */
+		for (item = head; item;)
+		{
+			ConfigVariable *ptr = item;
+
+			item = item->next;
+			if (strcmp(ptr->name, "data_directory") != 0)
+			{
+				if (prev == NULL)
+					head = ptr->next;
+				else
+				{
+					prev->next = ptr->next;
+					/*
+					 * On removing last item in list, we need to update tail
+					 * to ensure that list will be maintianed.
+					 */
+					if (prev->next == NULL)
+						tail = prev;
+				}
+				FreeConfigVariable(ptr);
+			}
+			else
+				prev = ptr;
+		}
+
+		/*
+		 * Quick exit if data_directory is not present in list.
+		 *
+		 * Don't remember when we last successfully loaded the config file in
+		 * this case because that time will be set soon by subsequent load of
+		 * the config file.
+		 */
+		if (head == NULL)
+			return;
 	}
 
 	/*
@@ -677,7 +731,7 @@ ParseConfigFp(FILE *fp, const char *config_file, int depth, int elevel,
 							*tail_p = prev_item;
 					}
 
-					pfree(cur_item);
+					FreeConfigVariable(cur_item);
 					break;
 				}
 			}
@@ -857,6 +911,21 @@ cleanup:
 	return status;
 }
 
+/*
+ * Free a ConfigVariable
+ */
+static void
+FreeConfigVariable(ConfigVariable *item)
+{
+	if (item != NULL)
+	{
+		pfree(item->name);
+		pfree(item->value);
+		pfree(item->filename);
+		pfree(item);
+	}
+}
+
 /*
  * Free a list of ConfigVariables, including the names and the values
  */
@@ -870,10 +939,7 @@ FreeConfigVariables(ConfigVariable *list)
 	{
 		ConfigVariable *next = item->next;
 
-		pfree(item->name);
-		pfree(item->value);
-		pfree(item->filename);
-		pfree(item);
+		FreeConfigVariable(item);
 		item = next;
 	}
 }
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 8ca065b7fe..e887ec3a37 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -4334,6 +4334,13 @@ SelectConfigFiles(const char *userDoption, const char *progname)
 		return false;
 	}
 
+	/*
+	 * Read the configuration file for the first time. This time only
+	 * data_directory parameter is picked up to determine the data directory
+	 * so that we can read PG_AUTOCONF_FILENAME file next time. Then don't
+	 * forget to read the configuration file again later to pick up all the
+	 * parameters.
+	 */
 	ProcessConfigFile(PGC_POSTMASTER);
 
 	/*