pg_parser_continue_on_error_v5.diff

application/octet-stream

Filename: pg_parser_continue_on_error_v5.diff
Type: application/octet-stream
Part: 0
Message: Re: REVIEW proposal: a validator for configuration files
*** a/src/backend/access/transam/xlog.c
--- b/src/backend/access/transam/xlog.c
***************
*** 5297,5303 **** readRecoveryCommandFile(void)
  	 * Since we're asking ParseConfigFp() to error out at FATAL, there's no
  	 * need to check the return value.
  	 */
! 	ParseConfigFp(fd, RECOVERY_COMMAND_FILE, 0, FATAL, &head, &tail);
  
  	for (item = head; item; item = item->next)
  	{
--- 5297,5303 ----
  	 * Since we're asking ParseConfigFp() to error out at FATAL, there's no
  	 * need to check the return value.
  	 */
! 	ParseConfigFp(fd, RECOVERY_COMMAND_FILE, 0, FATAL, 0, &head, &tail);
  
  	for (item = head; item; item = item->next)
  	{
*** a/src/backend/commands/extension.c
--- b/src/backend/commands/extension.c
***************
*** 474,480 **** parse_extension_control_file(ExtensionControlFile *control,
  	/*
  	 * Parse the file content, using GUC's file parsing code
  	 */
! 	ParseConfigFp(file, filename, 0, ERROR, &head, &tail);
  
  	FreeFile(file);
  
--- 474,480 ----
  	/*
  	 * Parse the file content, using GUC's file parsing code
  	 */
! 	ParseConfigFp(file, filename, 0, ERROR, 0, &head, &tail);
  
  	FreeFile(file);
  
*** a/src/backend/utils/misc/guc-file.l
--- b/src/backend/utils/misc/guc-file.l
***************
*** 43,48 **** int GUC_yylex(void);
--- 43,51 ----
  
  static char *GUC_scanstr(const char *s);
  
+ /* Maximum number of errors to report */
+ #define MAX_ERROR_REPORTS	100
+ 
  %}
  
  %option 8bit
***************
*** 112,135 **** ProcessConfigFile(GucContext context)
  	char	   *cvc = NULL;
  	struct config_string *cvc_struct;
  	int			i;
  
! 	Assert(context == PGC_POSTMASTER || context == PGC_SIGHUP);
  
! 	if (context == PGC_SIGHUP)
! 	{
! 		/*
! 		 * To avoid cluttering the log, only the postmaster bleats loudly
! 		 * about problems with the config file.
! 		 */
! 		elevel = IsUnderPostmaster ? DEBUG2 : LOG;
! 	}
! 	else
! 		elevel = ERROR;
  
! 	/* Parse the file into a list of option names and values */
  	head = tail = NULL;
  
! 	if (!ParseConfigFile(ConfigFileName, NULL, 0, elevel, &head, &tail))
  		goto cleanup_list;
  
  	/*
--- 115,146 ----
  	char	   *cvc = NULL;
  	struct config_string *cvc_struct;
  	int			i;
+ 	bool		errors,
+ 				apply = false;
  
! 	/*
! 	 * Config files are only processes on startup (by the postmaster)
! 	 * and on SIGHUP (by the postmaster and backends)
! 	 */
! 	Assert((context == PGC_POSTMASTER && !IsUnderPostmaster) ||
! 		   context == PGC_SIGHUP);
  
! 	/*
! 	 * To avoid cluttering the log, only the postmaster bleats loudly
! 	 * about problems with the config file.
! 	 */
! 	elevel = IsUnderPostmaster ? DEBUG2 : LOG;
  
! 	/*
! 	 * Parse the file into a list of option names and values.  We continue even
! 	 * if we hit a parse error, because we also want to report invalid values in
! 	 * the parts we could parse.
! 	 */
  	head = tail = NULL;
  
! 	errors = (ParseConfigFile(ConfigFileName, NULL, 0, elevel, 0, &head, &tail) > 0);
! 	/* bail out early if errors were found during the parse stage */
! 	if (errors)
  		goto cleanup_list;
  
  	/*
***************
*** 162,168 **** ProcessConfigFile(GucContext context)
--- 173,182 ----
  			goto cleanup_list;
  		if (!call_string_check_hook(cvc_struct, &cvc, &extra,
  									PGC_S_FILE, elevel))
+ 		{							
+ 			errors = true;
  			goto cleanup_list;
+ 		}
  		if (extra)
  			free(extra);
  	}
***************
*** 179,184 **** ProcessConfigFile(GucContext context)
--- 193,200 ----
  		gconf->status &= ~GUC_IS_IN_FILE;
  	}
  
+ 	apply = true;
+ 
  	/*
  	 * Check if all options are valid.  As a side-effect, the GUC_IS_IN_FILE
  	 * flag is set on each GUC variable mentioned in the list.
***************
*** 202,208 **** ProcessConfigFile(GucContext context)
  						(errcode(ERRCODE_UNDEFINED_OBJECT),
  						 errmsg("unrecognized configuration parameter \"%s\"",
  								item->name)));
! 				goto cleanup_list;
  			}
  			/*
  			 * 2. There is no GUC entry.  If we called set_config_option then
--- 218,226 ----
  						(errcode(ERRCODE_UNDEFINED_OBJECT),
  						 errmsg("unrecognized configuration parameter \"%s\"",
  								item->name)));
! 				errors = true;
! 				apply = false;
! 				continue;
  			}
  			/*
  			 * 2. There is no GUC entry.  If we called set_config_option then
***************
*** 219,230 **** ProcessConfigFile(GucContext context)
  			 * entry.
  			 */
  		}
! 
  		if (!set_config_option(item->name, item->value, context,
  							   PGC_S_FILE, GUC_ACTION_SET, false))
! 			goto cleanup_list;
  	}
  
  	/*
  	 * Check for variables having been removed from the config file, and
  	 * revert their reset values (and perhaps also effective values) to the
--- 237,268 ----
  			 * entry.
  			 */
  		}
! 		else if (find_option(item->name, false, elevel) == NULL)
! 		{
! 			/* For regular variables require a real GUC entry or bail out. */
! 			ereport(elevel,
! 					(errcode(ERRCODE_UNDEFINED_OBJECT),
! 					 errmsg("unrecognized configuration parameter \"%s\"",
! 					 		item->name)));
! 			errors = true;
! 			apply = false;
! 			continue;
! 		}
! 		/* Note possible problems and continue */
  		if (!set_config_option(item->name, item->value, context,
  							   PGC_S_FILE, GUC_ACTION_SET, false))
! 			errors = true;				   
  	}
  
+ 	/* 
+ 	 * Exit early if errors were detected during postmaster's startup 
+ 	 * or if we can't apply settings at SIGHUP due to parse errors.
+ 	 */
+ 	if (errors && context == PGC_POSTMASTER)
+ 		apply = false;
+ 	if (!apply)
+ 		goto cleanup_list;
+ 
  	/*
  	 * Check for variables having been removed from the config file, and
  	 * revert their reset values (and perhaps also effective values) to the
***************
*** 249,254 **** ProcessConfigFile(GucContext context)
--- 287,293 ----
  					(errcode(ERRCODE_CANT_CHANGE_RUNTIME_PARAM),
  					 errmsg("parameter \"%s\" cannot be changed without restarting the server",
  							gconf->name)));
+ 			errors = true;
  			continue;
  		}
  
***************
*** 285,291 **** ProcessConfigFile(GucContext context)
  	 * particular should be run only after initialization is complete.
  	 *
  	 * XXX this is an unmaintainable crock, because we have to know how
! 	 * to set (or at least what to call to set) every variable that could
  	 * potentially have PGC_S_DYNAMIC_DEFAULT or PGC_S_ENV_VAR source.
  	 * However, there's no time to redesign it for 9.1.
  	 */
--- 324,330 ----
  	 * particular should be run only after initialization is complete.
  	 *
  	 * XXX this is an unmaintainable crock, because we have to know how
! 	 * to set (at least what to call to set) every variable that could
  	 * potentially have PGC_S_DYNAMIC_DEFAULT or PGC_S_ENV_VAR source.
  	 * However, there's no time to redesign it for 9.1.
  	 */
***************
*** 316,321 **** ProcessConfigFile(GucContext context)
--- 355,370 ----
  			pre_value = pstrdup(preval);
  		}
  
+ 		/* 
+ 		 * XXX: We would like to notice the errors from set_config_option by
+ 		 * examining its return value, but, unfortunately, it's not possible.
+ 		 * The reason is that a false result is returned for PGC_POSTMASTER
+ 		 * variables during reload, even unchanged ones. This also means that
+ 		 * actual attempts to change such variables won't contribute to the
+ 		 * errorcount, because the same call returns true during the check
+ 		 * stage (when the changeVal argument is false), and that the invalid
+ 		 * values will be reported twice during reload.
+ 		 */
  		if (set_config_option(item->name, item->value, context,
  			   					 PGC_S_FILE, GUC_ACTION_SET, true))
  		{
***************
*** 346,364 **** ProcessConfigFile(GucContext context)
  	FreeConfigVariables(head);
  	if (cvc)
  		free(cvc);
  }
  
  /*
!  * See next function for details. This one will just work with a config_file
   * name rather than an already opened File Descriptor
   */
! bool
  ParseConfigFile(const char *config_file, const char *calling_file,
! 				int depth, int elevel,
  				ConfigVariable **head_p,
  				ConfigVariable **tail_p)
  {
! 	bool		OK = true;
  	FILE	   *fp;
  	char		abs_path[MAXPGPATH];
  
--- 395,433 ----
  	FreeConfigVariables(head);
  	if (cvc)
  		free(cvc);
+ 
+ 	if (errors)
+ 	{
+ 		 /* If we got errors during postmaster start, complain and bail out. */
+ 		if (context != PGC_SIGHUP)
+ 			elevel = ERROR;
+ 		else if (!IsUnderPostmaster)
+ 			elevel = WARNING;
+ 		else
+ 			elevel = DEBUG2;
+ 		ereport(elevel,
+ 				(errcode(ERRCODE_CONFIG_FILE_ERROR),
+ 				 errmsg("errors detected while parsing configuration files"),
+ 				 apply ? 
+ 				 errdetail("changes were applied.") : 
+ 				 errdetail("changes were not applied.")));		
+ 	}
  }
  
  /*
!  * See ParseConfigFp for details. This one will just work with a config_file
   * name rather than an already opened File Descriptor
+  *
+  * The return convention is the same as ParseConfigFp; additionally, file not
+  * found or file not accessible conditions are considered a single error.
   */
! int
  ParseConfigFile(const char *config_file, const char *calling_file,
! 				int depth, int elevel, int nerrors,
  				ConfigVariable **head_p,
  				ConfigVariable **tail_p)
  {
! 	int			errorcount = 0;
  	FILE	   *fp;
  	char		abs_path[MAXPGPATH];
  
***************
*** 373,379 **** ParseConfigFile(const char *config_file, const char *calling_file,
  				(errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
  				 errmsg("could not open configuration file \"%s\": maximum nesting depth exceeded",
  						config_file)));
! 		return false;
  	}
  
  	/*
--- 442,448 ----
  				(errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
  				 errmsg("could not open configuration file \"%s\": maximum nesting depth exceeded",
  						config_file)));
! 		return 1;
  	}
  
  	/*
***************
*** 408,421 **** ParseConfigFile(const char *config_file, const char *calling_file,
  				(errcode_for_file_access(),
  				 errmsg("could not open configuration file \"%s\": %m",
  						config_file)));
! 		return false;
  	}
  
! 	OK = ParseConfigFp(fp, config_file, depth, elevel, head_p, tail_p);
  
  	FreeFile(fp);
  
! 	return OK;
  }
  
  /*
--- 477,490 ----
  				(errcode_for_file_access(),
  				 errmsg("could not open configuration file \"%s\": %m",
  						config_file)));
! 		return 1;
  	}
  
! 	errorcount = ParseConfigFp(fp, config_file, depth, elevel, nerrors, head_p, tail_p);
  
  	FreeFile(fp);
  
! 	return errorcount;
  }
  
  /*
***************
*** 427,432 **** ParseConfigFile(const char *config_file, const char *calling_file,
--- 496,502 ----
   *	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()
+  *	nerrors: number of errors already seen while parsing previous config files
   * Output parameters:
   *	head_p, tail_p: head and tail of linked list of name/value pairs
   *
***************
*** 434,456 **** ParseConfigFile(const char *config_file, const char *calling_file,
   * 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
--- 504,533 ----
   * recursion level.  On exit, they contain a list of name-value pairs read
   * from the input file(s).
   *
!  * Returns 0 if successful, or the number of parsing errors found if any
!  * occurred.  The error(s) have already been ereport'd, it is only necessary
!  * for the caller to clean up its own state and release the name/value pairs
!  * list.
!  *
!  * If elevel < ERROR then we don't return immediately on the first error,
!  * although we do return after coming across 100 of them. This is to prevent
!  * trashing out logs when parsing a completely bogus file.
   *
   * 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.
+  *
+  * We use nerrors to break out early in case too many errors are found.
   */
! int
  ParseConfigFp(FILE *fp, const char *config_file, int depth, int elevel,
! 			  int nerrors, ConfigVariable **head_p, ConfigVariable **tail_p)
  {
  	YY_BUFFER_STATE lex_buffer;
! 	int			token,
! 				errorcount;
  
  	/*
  	 * Parse
***************
*** 459,464 **** ParseConfigFp(FILE *fp, const char *config_file, int depth, int elevel,
--- 536,542 ----
  	yy_switch_to_buffer(lex_buffer);
  
  	ConfigFileLineno = 1;
+ 	errorcount = 0;
  
  	/* This loop iterates once per logical line */
  	while ((token = yylex()))
***************
*** 510,524 **** ParseConfigFp(FILE *fp, const char *config_file, int depth, int elevel,
  			 */
  			unsigned int save_ConfigFileLineno = ConfigFileLineno;
  
! 			if (!ParseConfigFile(opt_value, config_file,
! 								 depth + 1, elevel,
! 								 head_p, tail_p))
! 			{
! 				pfree(opt_name);
! 				pfree(opt_value);
! 				OK = false;
! 				goto cleanup_exit;
! 			}
  			yy_switch_to_buffer(lex_buffer);
  			ConfigFileLineno = save_ConfigFileLineno;
  			pfree(opt_name);
--- 588,595 ----
  			 */
  			unsigned int save_ConfigFileLineno = ConfigFileLineno;
  
! 			errorcount += ParseConfigFile(opt_value, config_file, depth + 1,
! 										  elevel, errorcount, head_p, tail_p);
  			yy_switch_to_buffer(lex_buffer);
  			ConfigFileLineno = save_ConfigFileLineno;
  			pfree(opt_name);
***************
*** 577,603 **** ParseConfigFp(FILE *fp, const char *config_file, int depth, int elevel,
  		/* break out of loop if read EOF, else loop for next line */
  		if (token == 0)
  			break;
  	}
  
  	/* successful completion of parsing */
- 	goto cleanup_exit;
- 
-  parse_error:
- 	if (token == GUC_EOL || token == 0)
- 		ereport(elevel,
- 				(errcode(ERRCODE_SYNTAX_ERROR),
- 				 errmsg("syntax error in file \"%s\" line %u, near end of line",
- 						config_file, ConfigFileLineno - 1)));
- 	else
- 		ereport(elevel,
- 				(errcode(ERRCODE_SYNTAX_ERROR),
- 				 errmsg("syntax error in file \"%s\" line %u, near token \"%s\"",
- 						config_file, ConfigFileLineno, yytext)));
- 	OK = false;
- 
- cleanup_exit:
  	yy_delete_buffer(lex_buffer);
! 	return OK;
  }
  
  
--- 648,690 ----
  		/* break out of loop if read EOF, else loop for next line */
  		if (token == 0)
  			break;
+ 		/* skip over parse_error if we made it this far without errors */
+ 		continue;
+ 
+ 	parse_error:
+ 		if (token == GUC_EOL || token == 0)
+ 			ereport(elevel,
+ 					(errcode(ERRCODE_SYNTAX_ERROR),
+ 					 errmsg("syntax error in file \"%s\" line %u, near end of line",
+ 							config_file, ConfigFileLineno - 1)));
+ 		else
+ 			ereport(elevel,
+ 					(errcode(ERRCODE_SYNTAX_ERROR),
+ 					 errmsg("syntax error in file \"%s\" line %u, near token \"%s\"",
+ 							config_file, ConfigFileLineno, yytext)));
+ 		errorcount++;
+ 		/* fast forward till the next EOL/EOF */
+ 		while (token != 0 && token != GUC_EOL)
+ 			token = yylex();
+ 		
+ 		/* break out of loop on EOF */
+ 		if (token == 0)
+ 			break;
+ 		
+ 		/* avoid producing too much noise when parsing a bogus file */
+ 		if (errorcount + nerrors >= MAX_ERROR_REPORTS)
+ 		{
+ 			ereport(elevel,
+ 					(errcode(ERRCODE_SYNTAX_ERROR),
+ 					 errmsg("too many errors found, stopped processing file \"%s\"",
+ 							config_file)));
+ 			break;
+ 		}
  	}
  
  	/* successful completion of parsing */
  	yy_delete_buffer(lex_buffer);
! 	return errorcount;
  }
  
  
*** a/src/backend/utils/misc/guc.c
--- b/src/backend/utils/misc/guc.c
***************
*** 5053,5059 **** config_enum_get_options(struct config_enum * record, const char *prefix,
   *
   * If there is an error (non-existing option, invalid value) then an
   * ereport(ERROR) is thrown *unless* this is called in a context where we
!  * don't want to ereport (currently, startup or SIGHUP config file reread).
   * In that case we write a suitable error message via ereport(LOG) and
   * return false. This is working around the deficiencies in the ereport
   * mechanism, so don't blame me.  In all other cases, the function
--- 5053,5059 ----
   *
   * If there is an error (non-existing option, invalid value) then an
   * ereport(ERROR) is thrown *unless* this is called in a context where we
!  * don't want to ereport (currently, settings defaults and config file read).
   * In that case we write a suitable error message via ereport(LOG) and
   * return false. This is working around the deficiencies in the ereport
   * mechanism, so don't blame me.  In all other cases, the function
***************
*** 5072,5078 **** set_config_option(const char *name, const char *value,
  	bool		prohibitValueChange = false;
  	bool		makeDefault;
  
! 	if (context == PGC_SIGHUP || source == PGC_S_DEFAULT)
  	{
  		/*
  		 * To avoid cluttering the log, only the postmaster bleats loudly
--- 5072,5078 ----
  	bool		prohibitValueChange = false;
  	bool		makeDefault;
  
! 	if (source == PGC_S_FILE || source == PGC_S_DEFAULT)
  	{
  		/*
  		 * To avoid cluttering the log, only the postmaster bleats loudly
*** a/src/include/utils/guc.h
--- b/src/include/utils/guc.h
***************
*** 110,120 **** typedef struct ConfigVariable
  	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);
  
--- 110,120 ----
  	struct ConfigVariable *next;
  } ConfigVariable;
  
! extern int ParseConfigFile(const char *config_file, const char *calling_file,
! 				int depth, int elevel, int nerrors,
  				ConfigVariable **head_p, ConfigVariable **tail_p);
! extern int ParseConfigFp(FILE *fp, const char *config_file,
! 			  int depth, int elevel, int nerrors,
  			  ConfigVariable **head_p, ConfigVariable **tail_p);
  extern void FreeConfigVariables(ConfigVariable *list);