Use GUC lexer for recovery.conf parsing.

This eliminates some crufty, special-purpose code and, as a non-trivial
side benefit, allows recovery.conf parameters to be unquoted.

Dimitri Fontaine, with review and cleanup by Alvaro Herrera, Itagaki
Takahiro, and me.

src/backend/access/transam/xlog.c

@@ -5023,116 +5023,21 @@ str_time(pg_time_t tnow)
 	return buf;
 }
 
-/*
- * Parse one line from recovery.conf. 'cmdline' is the raw line from the
- * file. If the line is parsed successfully, returns true, false indicates
- * syntax error. On success, *key_p and *value_p are set to the parameter
- * name and value on the line, respectively. If the line is an empty line,
- * consisting entirely of whitespace and comments, function returns true
- * and *keyp_p and *value_p are set to NULL.
- *
- * The pointers returned in *key_p and *value_p point to an internal buffer
- * that is valid only until the next call of parseRecoveryCommandFile().
- */
-static bool
-parseRecoveryCommandFileLine(char *cmdline, char **key_p, char **value_p)
-{
-	char	   *ptr;
-	char	   *bufp;
-	char	   *key;
-	char	   *value;
-	static char *buf = NULL;
-
-	*key_p = *value_p = NULL;
-
-	/*
-	 * Allocate the buffer on first use. It's used to hold both the parameter
-	 * name and value.
-	 */
-	if (buf == NULL)
-		buf = malloc(MAXPGPATH + 1);
-	bufp = buf;
-
-	/* Skip any whitespace at the beginning of line */
-	for (ptr = cmdline; *ptr; ptr++)
-	{
-		if (!isspace((unsigned char) *ptr))
-			break;
-	}
-	/* Ignore empty lines */
-	if (*ptr == '\0' || *ptr == '#')
-		return true;
-
-	/* Read the parameter name */
-	key = bufp;
-	while (*ptr && !isspace((unsigned char) *ptr) &&
-		   *ptr != '=' && *ptr != '\'')
-		*(bufp++) = *(ptr++);
-	*(bufp++) = '\0';
-
-	/* Skip to the beginning quote of the parameter value */
-	ptr = strchr(ptr, '\'');
-	if (!ptr)
-		return false;
-	ptr++;
-
-	/* Read the parameter value to *bufp. Collapse any '' escapes as we go. */
-	value = bufp;
-	for (;;)
-	{
-		if (*ptr == '\'')
-		{
-			ptr++;
-			if (*ptr == '\'')
-				*(bufp++) = '\'';
-			else
-			{
-				/* end of parameter */
-				*bufp = '\0';
-				break;
-			}
-		}
-		else if (*ptr == '\0')
-			return false;		/* unterminated quoted string */
-		else
-			*(bufp++) = *ptr;
-
-		ptr++;
-	}
-	*(bufp++) = '\0';
-
-	/* Check that there's no garbage after the value */
-	while (*ptr)
-	{
-		if (*ptr == '#')
-			break;
-		if (!isspace((unsigned char) *ptr))
-			return false;
-		ptr++;
-	}
-
-	/* Success! */
-	*key_p = key;
-	*value_p = value;
-	return true;
-}
-
 /*
  * See if there is a recovery command file (recovery.conf), and if so
  * read in parameters for archive recovery and XLOG streaming.
  *
- * XXX longer term intention is to expand this to
+ * The file is parsed using the main configuration parser.
- * cater for additional parameters and controls
- * possibly use a flex lexer similar to the GUC one
  */
 static void
 readRecoveryCommandFile(void)
 {
 	FILE	   *fd;
-	char		cmdline[MAXPGPATH];
 	TimeLineID	rtli = 0;
 	bool		rtliGiven = false;
-	bool		syntaxError = false;
+	ConfigVariable *item,
+				   *head = NULL,
+				   *tail = NULL;
 
 	fd = AllocateFile(RECOVERY_COMMAND_FILE, "r");
 	if (fd == NULL)
@@ -5146,55 +5051,47 @@ readRecoveryCommandFile(void)
 	}
 
 	/*
-	 * Parse the file...
+	 * Since we're asking ParseConfigFp() to error out at FATAL, there's no
+	 * need to check the return value.
 	 */ 
-	while (fgets(cmdline, sizeof(cmdline), fd) != NULL)
+	ParseConfigFp(fd, RECOVERY_COMMAND_FILE, 0, FATAL, &head, &tail);
+
+	for (item = head; item; item = item->next)
 	{
-		char	   *tok1;
+		if (strcmp(item->name, "restore_command") == 0)
-		char	   *tok2;
-
-		if (!parseRecoveryCommandFileLine(cmdline, &tok1, &tok2))
 		{
-			syntaxError = true;
+			recoveryRestoreCommand = pstrdup(item->value);
-			break;
-		}
-		if (tok1 == NULL)
-			continue;
-
-		if (strcmp(tok1, "restore_command") == 0)
-		{
-			recoveryRestoreCommand = pstrdup(tok2);
 			ereport(DEBUG2,
 					(errmsg("restore_command = '%s'",
 							recoveryRestoreCommand)));
 		}
-		else if (strcmp(tok1, "recovery_end_command") == 0)
+		else if (strcmp(item->name, "recovery_end_command") == 0)
 		{
-			recoveryEndCommand = pstrdup(tok2);
+			recoveryEndCommand = pstrdup(item->value);
 			ereport(DEBUG2,
 					(errmsg("recovery_end_command = '%s'",
 							recoveryEndCommand)));
 		}
-		else if (strcmp(tok1, "archive_cleanup_command") == 0)
+		else if (strcmp(item->name, "archive_cleanup_command") == 0)
 		{
-			archiveCleanupCommand = pstrdup(tok2);
+			archiveCleanupCommand = pstrdup(item->value);
 			ereport(DEBUG2,
 					(errmsg("archive_cleanup_command = '%s'",
 							archiveCleanupCommand)));
 		}
-		else if (strcmp(tok1, "recovery_target_timeline") == 0)
+		else if (strcmp(item->name, "recovery_target_timeline") == 0)
 		{
 			rtliGiven = true;
-			if (strcmp(tok2, "latest") == 0)
+			if (strcmp(item->value, "latest") == 0)
 				rtli = 0;
 			else
 			{
 				errno = 0;
-				rtli = (TimeLineID) strtoul(tok2, NULL, 0);
+				rtli = (TimeLineID) strtoul(item->value, NULL, 0);
 				if (errno == EINVAL || errno == ERANGE)
 					ereport(FATAL,
 							(errmsg("recovery_target_timeline is not a valid number: \"%s\"",
-									tok2)));
+									item->value)));
 			}
 			if (rtli)
 				ereport(DEBUG2,
@@ -5203,20 +5100,20 @@ readRecoveryCommandFile(void)
 				ereport(DEBUG2,
 						(errmsg("recovery_target_timeline = latest")));
 		}
-		else if (strcmp(tok1, "recovery_target_xid") == 0)
+		else if (strcmp(item->name, "recovery_target_xid") == 0)
 		{
 			errno = 0;
-			recoveryTargetXid = (TransactionId) strtoul(tok2, NULL, 0);
+			recoveryTargetXid = (TransactionId) strtoul(item->value, NULL, 0);
 			if (errno == EINVAL || errno == ERANGE)
 				ereport(FATAL,
 				 (errmsg("recovery_target_xid is not a valid number: \"%s\"",
-						 tok2)));
+						 item->value)));
 			ereport(DEBUG2,
 					(errmsg("recovery_target_xid = %u",
 							recoveryTargetXid)));
 			recoveryTarget = RECOVERY_TARGET_XID;
 		}
-		else if (strcmp(tok1, "recovery_target_time") == 0)
+		else if (strcmp(item->name, "recovery_target_time") == 0)
 		{
 			/*
 			 * if recovery_target_xid specified, then this overrides
@@ -5231,44 +5128,44 @@ readRecoveryCommandFile(void)
 			 */
 			recoveryTargetTime =
 				DatumGetTimestampTz(DirectFunctionCall3(timestamptz_in,
-														CStringGetDatum(tok2),
+														CStringGetDatum(item->value),
 												ObjectIdGetDatum(InvalidOid),
 														Int32GetDatum(-1)));
 			ereport(DEBUG2,
 					(errmsg("recovery_target_time = '%s'",
 							timestamptz_to_str(recoveryTargetTime))));
 		}
-		else if (strcmp(tok1, "recovery_target_inclusive") == 0)
+		else if (strcmp(item->name, "recovery_target_inclusive") == 0)
 		{
 			/*
 			 * does nothing if a recovery_target is not also set
 			 */
-			if (!parse_bool(tok2, &recoveryTargetInclusive))
+			if (!parse_bool(item->value, &recoveryTargetInclusive))
 				ereport(ERROR,
 						(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
 						 errmsg("parameter \"%s\" requires a Boolean value", "recovery_target_inclusive")));
 			ereport(DEBUG2,
-					(errmsg("recovery_target_inclusive = %s", tok2)));
+					(errmsg("recovery_target_inclusive = %s", item->value)));
 		}
-		else if (strcmp(tok1, "standby_mode") == 0)
+		else if (strcmp(item->name, "standby_mode") == 0)
 		{
-			if (!parse_bool(tok2, &StandbyMode))
+			if (!parse_bool(item->value, &StandbyMode))
 				ereport(ERROR,
 						(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
 						 errmsg("parameter \"%s\" requires a Boolean value", "standby_mode")));
 			ereport(DEBUG2,
-					(errmsg("standby_mode = '%s'", tok2)));
+					(errmsg("standby_mode = '%s'", item->value)));
 		}
-		else if (strcmp(tok1, "primary_conninfo") == 0)
+		else if (strcmp(item->name, "primary_conninfo") == 0)
 		{
-			PrimaryConnInfo = pstrdup(tok2);
+			PrimaryConnInfo = pstrdup(item->value);
 			ereport(DEBUG2,
 					(errmsg("primary_conninfo = '%s'",
 							PrimaryConnInfo)));
 		}
-		else if (strcmp(tok1, "trigger_file") == 0)
+		else if (strcmp(item->name, "trigger_file") == 0)
 		{
-			TriggerFile = pstrdup(tok2);
+			TriggerFile = pstrdup(item->value);
 			ereport(DEBUG2,
 					(errmsg("trigger_file = '%s'",
 							TriggerFile)));
@@ -5276,17 +5173,9 @@ readRecoveryCommandFile(void)
 		else
 			ereport(FATAL,
 					(errmsg("unrecognized recovery parameter \"%s\"",
-							tok1)));
+							item->name)));
 	}
 
-	FreeFile(fd);
-
-	if (syntaxError)
-		ereport(FATAL,
-				(errmsg("syntax error in recovery command file: %s",
-						cmdline),
-			  errhint("Lines should have the format parameter = 'value'.")));
-
 	/*
 	 * Check for compulsory parameters
 	 */
@@ -5332,6 +5221,9 @@ readRecoveryCommandFile(void)
 			recoveryTargetTLI = findNewestTimeLine(recoveryTargetTLI);
 		}
 	}
+
+	FreeConfigVariables(head);
+	FreeFile(fd);
 }
 
 /*

src/backend/utils/misc/guc-file.l

@@ -35,25 +35,11 @@ enum {
 	GUC_ERROR = 100
 };
 
-struct name_value_pair
-{
-	char       *name;
-	char       *value;
-	char	   *filename;
-	int			sourceline;
-	struct name_value_pair *next;
-};
-
 static unsigned int ConfigFileLineno;
 
 /* flex fails to supply a prototype for yylex, so provide one */
 int GUC_yylex(void);
 
-static bool ParseConfigFile(const char *config_file, const char *calling_file,
-							int depth, int elevel,
-							struct name_value_pair **head_p,
-							struct name_value_pair **tail_p);
-static void free_name_value_list(struct name_value_pair * list);
 static char *GUC_scanstr(const char *s);
 
 %}
@@ -118,7 +104,9 @@ void
 ProcessConfigFile(GucContext context)
 {
 	int			elevel;
-	struct name_value_pair *item, *head, *tail;
+	ConfigVariable *item,
+				   *head,
+				   *tail;
 	char	   *cvc = NULL;
 	struct config_string *cvc_struct;
 	const char *envvar;
@@ -351,50 +339,24 @@ ProcessConfigFile(GucContext context)
 	PgReloadTime = GetCurrentTimestamp();
 
  cleanup_list:
-	free_name_value_list(head);
+	FreeConfigVariables(head);
 	if (cvc)
 		free(cvc);
 }
 
-
 /*
- * Read and parse a single configuration file.  This function recurses
+ * See next function for details. This one will just work with a config_file
- * to handle "include" directives.
+ * name rather than an already opened File Descriptor
- *
- * Input parameters:
- *	config_file: absolute or relative path of file to read
- *	calling_file: absolute path of file containing the "include" directive,
- *		or NULL at outer level (config_file must be absolute at outer level)
- *	depth: recursion depth (used only to prevent infinite recursion)
- *	elevel: error logging level determined by ProcessConfigFile()
- * Output parameters:
- *	head_p, tail_p: head and tail of linked list of name/value pairs
- *
- * *head_p and *tail_p must be initialized to NULL before calling the outer
- * recursion level.  On exit, they contain a list of name-value pairs read
- * from the input file(s).
- *
- * Returns TRUE if successful, FALSE if an error occurred.  The error has
- * already been ereport'd, it is only necessary for the caller to clean up
- * its own state and release the name/value pairs list.
- *
- * Note: if elevel >= ERROR then an error will not return control to the
- * caller, and internal state such as open files will not be cleaned up.
- * This case occurs only during postmaster or standalone-backend startup,
- * where an error will lead to immediate process exit anyway; so there is
- * no point in contorting the code so it can clean up nicely.
  */
-static bool
+bool
 ParseConfigFile(const char *config_file, const char *calling_file,
 				int depth, int elevel,
-				struct name_value_pair **head_p,
+				ConfigVariable **head_p,
-				struct name_value_pair **tail_p)
+				ConfigVariable **tail_p)
 {
 	bool		OK = true;
-	char		abs_path[MAXPGPATH];
 	FILE	   *fp;
-	YY_BUFFER_STATE lex_buffer;
+	char		abs_path[MAXPGPATH];
-	int			token;
 
 	/*
 	 * Reject too-deep include nesting depth.  This is just a safety check
@@ -416,12 +378,23 @@ ParseConfigFile(const char *config_file, const char *calling_file,
 	 */
 	if (!is_absolute_path(config_file))
 	{
-		Assert(calling_file != NULL);
+		if (calling_file != NULL)
-		strlcpy(abs_path, calling_file, sizeof(abs_path));
+		{
-		get_parent_directory(abs_path);
+			strlcpy(abs_path, calling_file, sizeof(abs_path));
-		join_path_components(abs_path, abs_path, config_file);
+			get_parent_directory(abs_path);
-		canonicalize_path(abs_path);
+			join_path_components(abs_path, abs_path, config_file);
-		config_file = abs_path;
+			canonicalize_path(abs_path);
+			config_file = abs_path;
+		}
+		else
+		{
+			/*
+			 * calling_file is NULL, we make an absolute path from $PGDATA
+			 */
+			join_path_components(abs_path, data_directory, config_file);
+			canonicalize_path(abs_path);
+			config_file = abs_path;
+		}
 	}
 
 	fp = AllocateFile(config_file, "r");
@@ -434,6 +407,47 @@ ParseConfigFile(const char *config_file, const char *calling_file,
 		return false;
 	}
 
+	OK = ParseConfigFp(fp, config_file, depth, elevel, head_p, tail_p);
+
+	FreeFile(fp);
+
+	return OK;
+}
+
+/*
+ * Read and parse a single configuration file.  This function recurses
+ * to handle "include" directives.
+ *
+ * Input parameters:
+ *	fp: file pointer from AllocateFile for the configuration file to parse
+ *	config_file: absolute or relative path of file to read
+ *	depth: recursion depth (used only to prevent infinite recursion)
+ *	elevel: error logging level determined by ProcessConfigFile()
+ * Output parameters:
+ *	head_p, tail_p: head and tail of linked list of name/value pairs
+ *
+ * *head_p and *tail_p must be initialized to NULL before calling the outer
+ * recursion level.  On exit, they contain a list of name-value pairs read
+ * from the input file(s).
+ *
+ * Returns TRUE if successful, FALSE if an error occurred.  The error has
+ * already been ereport'd, it is only necessary for the caller to clean up
+ * its own state and release the name/value pairs list.
+ *
+ * Note: if elevel >= ERROR then an error will not return control to the
+ * caller, and internal state such as open files will not be cleaned up.
+ * This case occurs only during postmaster or standalone-backend startup,
+ * where an error will lead to immediate process exit anyway; so there is
+ * no point in contorting the code so it can clean up nicely.
+ */
+bool
+ParseConfigFp(FILE *fp, const char *config_file, int depth, int elevel,
+			  ConfigVariable **head_p, ConfigVariable **tail_p)
+{
+	bool		OK = true;
+	YY_BUFFER_STATE lex_buffer;
+	int			token;
+
 	/*
 	 * Parse
 	 */
@@ -446,7 +460,7 @@ ParseConfigFile(const char *config_file, const char *calling_file,
 	while ((token = yylex()))
 	{
 		char	   *opt_name, *opt_value;
-		struct name_value_pair *item;
+		ConfigVariable *item;
 
 		if (token == GUC_EOL)	/* empty or comment line */
 			continue;
@@ -579,23 +593,22 @@ ParseConfigFile(const char *config_file, const char *calling_file,
 
 cleanup_exit:
 	yy_delete_buffer(lex_buffer);
-	FreeFile(fp);
 	return OK;
 }
 
 
 /*
- * Free a list of name/value pairs, including the names and the values
+ * Free a list of ConfigVariables, including the names and the values
  */
-static void
+void
-free_name_value_list(struct name_value_pair *list)
+FreeConfigVariables(ConfigVariable *list)
 {
-	struct name_value_pair *item;
+	ConfigVariable *item;
 
 	item = list;
 	while (item)
 	{
-		struct name_value_pair *next = item->next;
+		ConfigVariable *next = item->next;
 
 		pfree(item->name);
 		pfree(item->value);

src/backend/utils/misc/guc.c

@@ -389,6 +389,7 @@ int			trace_recovery_messages = LOG;
 
 int			num_temp_buffers = 1000;
 
+char	   *data_directory;
 char	   *ConfigFileName;
 char	   *HbaFileName;
 char	   *IdentFileName;
@@ -426,7 +427,6 @@ static char *timezone_string;
 static char *log_timezone_string;
 static char *timezone_abbreviations_string;
 static char *XactIsoLevel_string;
-static char *data_directory;
 static char *custom_variable_classes;
 static int	max_function_args;
 static int	max_index_keys;

src/include/utils/guc.h

@@ -96,6 +96,26 @@ typedef enum
 	PGC_S_SESSION				/* SET command */
 } GucSource;
 
+/*
+ * Parsing the configuation file will return a list of name-value pairs
+ */
+typedef struct ConfigVariable
+{
+	char       *name;
+	char       *value;
+	char	   *filename;
+	int			sourceline;
+	struct ConfigVariable  *next;
+} ConfigVariable;
+
+extern bool ParseConfigFile(const char *config_file, const char *calling_file,
+				int depth, int elevel,
+				ConfigVariable **head_p, ConfigVariable **tail_p);
+extern bool ParseConfigFp(FILE *fp, const char *config_file,
+			  int depth, int elevel,
+			  ConfigVariable **head_p, ConfigVariable **tail_p);
+extern void FreeConfigVariables(ConfigVariable *list);
+
 /*
  * Enum values are made up of an array of name-value pairs
  */
@@ -176,6 +196,7 @@ extern int	log_temp_files;
 
 extern int	num_temp_buffers;
 
+extern char *data_directory;
 extern char *ConfigFileName;
 extern char *HbaFileName;
 extern char *IdentFileName;