Thread: postgresql.conf: patch to have ParseConfigFile report all parsing errors, then bail

ParseConfigFile currently exits on the first parsing error. Changed
guc_file.l to report all parsing errors before exiting:
* Moved parse_error: block inside while() loop
* Removed cleanup_exit: and associated 'goto'
* Added ereport if ParseConfigFile() returns false
* changed OK to ok ;)
* Added comment - TODO: Report bogus variables in addition to parsing
errors before bailing out

-selena

--
Selena Deckelmann
End Point Corporation
selena@endpoint.com
503-282-2512
*** a/src/backend/utils/misc/guc-file.l
--- b/src/backend/utils/misc/guc-file.l
***************
*** 143,149 **** ProcessConfigFile(GucContext context)
--- 143,155 ----
      if (!ParseConfigFile(ConfigFileName, NULL,
                           0, context, elevel,
                           &head, &tail))
+     {
+         ereport(elevel,
+                 (errcode(ERRCODE_CONFIG_FILE_ERROR),
+                  errmsg("Did not reload \"%s\" due to earlier parsing error(s)", ConfigFileName)));
+         /* TODO: Report bogus variables in addition to parsing errors before bailing out */
          goto cleanup_list;
+     }

      /*
       * We need the proposed new value of custom_variable_classes to check
***************
*** 361,367 **** ParseConfigFile(const char *config_file, const char *calling_file,
                  struct name_value_pair **head_p,
                  struct name_value_pair **tail_p)
  {
!     bool        OK = true;
      char        abs_path[MAXPGPATH];
      FILE       *fp;
      YY_BUFFER_STATE lex_buffer;
--- 367,373 ----
                  struct name_value_pair **head_p,
                  struct name_value_pair **tail_p)
  {
!     bool        ok = true;
      char        abs_path[MAXPGPATH];
      FILE       *fp;
      YY_BUFFER_STATE lex_buffer;
***************
*** 461,472 **** ParseConfigFile(const char *config_file, const char *calling_file,
              if (!ParseConfigFile(opt_value, config_file,
                                   depth + 1, context, 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);
--- 467,474 ----
              if (!ParseConfigFile(opt_value, config_file,
                                   depth + 1, context, elevel,
                                   head_p, tail_p))
!                 ok = false;
!
              yy_switch_to_buffer(lex_buffer);
              ConfigFileLineno = save_ConfigFileLineno;
              pfree(opt_name);
***************
*** 525,552 **** ParseConfigFile(const char *config_file, const char *calling_file,
          /* 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);
      FreeFile(fp);
!     return OK;
  }


--- 527,555 ----
          /* 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 error */
!         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)));
!             ok = false;
!     }

!     /* completion of parsing */
      yy_delete_buffer(lex_buffer);
      FreeFile(fp);
!
!     return ok;
  }

Selena Deckelmann wrote:

> !         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)));

Not that this has anything to do with the patch at hand, but I remember
thinking about this sort of error message in the past.  Would it be
appropriate to move the file name and line number to an errcontext()
field?

I know somebody is going to say "but errcontext is not shown when
verbosity is set to terse", to which I say that we should fix that too.

-- 
Alvaro Herrera                                http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera <alvherre@commandprompt.com> writes:
> Not that this has anything to do with the patch at hand, but I remember
> thinking about this sort of error message in the past.  Would it be
> appropriate to move the file name and line number to an errcontext()
> field?

I think the message is fine as is.  If you moved that information then
you'd have nothing left in the base message but "syntax error", which
is useless.

> I know somebody is going to say "but errcontext is not shown when
> verbosity is set to terse", to which I say that we should fix that too.

No, because the whole point of terse is to be terse.  Adding context
to the output would just create a demand for "super terse" mode.
        regards, tom lane

On Sun, 2009-03-08 at 16:27 -0700, Selena Deckelmann wrote:
> ParseConfigFile currently exits on the first parsing error. Changed 
> guc_file.l to report all parsing errors before exiting:
> * Moved parse_error: block inside while() loop
> * Removed cleanup_exit: and associated 'goto'
> * Added ereport if ParseConfigFile() returns false
> * changed OK to ok ;)
> * Added comment - TODO: Report bogus variables in addition to parsing 
> errors before bailing out

These are very good changes, good news.

Is it possible to check for parameters that have been changed, yet will
not be applied at reload? It's a common error to change something like
shared_buffers and then expect that to have changed when you reload. It
would be useful to issue messages when that has occurred.

-- Simon Riggs           www.2ndQuadrant.comPostgreSQL Training, Services and Support

Hi!

Simon Riggs wrote:
> On Sun, 2009-03-08 at 16:27 -0700, Selena Deckelmann wrote:
>> ParseConfigFile currently exits on the first parsing error. Changed
>> guc_file.l to report all parsing errors before exiting:
>> * Moved parse_error: block inside while() loop
>> * Removed cleanup_exit: and associated 'goto'
>> * Added ereport if ParseConfigFile() returns false
>> * changed OK to ok ;)
>> * Added comment - TODO: Report bogus variables in addition to parsing
>> errors before bailing out
>
> These are very good changes, good news.

Thanks :)

> Is it possible to check for parameters that have been changed, yet will
> not be applied at reload?

This was already implemented! :) For example:

LOG:  attempted change of parameter "shared_buffers" ignored
DETAIL:  This parameter cannot be changed after server start.
LOG:  attempted change of parameter "max_prepared_transactions" ignored
DETAIL:  This parameter cannot be changed after server start.

A thing that could be added, however, is reporting of all invalid (as 
opposed to valid, but requires a restart to apply) parameters before 
exiting. This change requires refactoring ProcessConfigFile() more 
significantly, as the parsing and validity checks are done separately, 
and are exited with gotos. :)

I haven't tried to change this yet, but was planning to.

-selena


-- 
Selena Deckelmann
End Point Corporation

On Tue, 2009-03-10 at 07:30 -0700, Selena Deckelmann wrote:

> > Is it possible to check for parameters that have been changed, yet will
> > not be applied at reload?
> 
> This was already implemented! :) For example:
> 
> LOG:  attempted change of parameter "shared_buffers" ignored
> DETAIL:  This parameter cannot be changed after server start.
> LOG:  attempted change of parameter "max_prepared_transactions" ignored
> DETAIL:  This parameter cannot be changed after server start.

OK, had that at the back of my mind for a while I guess.

> A thing that could be added, however, is reporting of all invalid (as 
> opposed to valid, but requires a restart to apply) parameters before 
> exiting. This change requires refactoring ProcessConfigFile() more 
> significantly, as the parsing and validity checks are done separately, 
> and are exited with gotos. :)
> 
> I haven't tried to change this yet, but was planning to.

Can't we just do a reload, but with doit = false?

-- Simon Riggs           www.2ndQuadrant.comPostgreSQL Training, Services and Support

Simon Riggs wrote:
> On Tue, 2009-03-10 at 07:30 -0700, Selena Deckelmann wrote:

>> A thing that could be added, however, is reporting of all invalid (as
>> opposed to valid, but requires a restart to apply) parameters before
>> exiting. This change requires refactoring ProcessConfigFile() more
>> significantly, as the parsing and validity checks are done separately,
>> and are exited with gotos. :)
>>
>> I haven't tried to change this yet, but was planning to.
>
> Can't we just do a reload, but with doit = false?

That would also be cool.  Like an 'apachectl configtest'.

There are five places where a 'goto' is used in guc-file.l needs to be 
changed to make this work (in addition to adding an option to pg_ctl).

It should be pretty straightforward, and I'll have a look at it tonight.

-selena

-- 
Selena Deckelmann
End Point Corporation

Selena Deckelmann <selena@endpoint.com> writes:
> ParseConfigFile currently exits on the first parsing error. Changed 
> guc_file.l to report all parsing errors before exiting:

This seems like basically a good idea, but consider what happens if
you make a really major-league screwup in your postgresql.conf
(say, you accidentally copy the text of "War and Peace" into it).
You'll get megabytes of mostly-useless bleating in your log file.
Multiply that by the number of active backends, if you're unlucky
enough to have done it at log level DEBUG2.  And not only are you
bloating your log, but it's going to take a fair amount of time
for all the backends to read and complain (or not) about the whole
file.

So I think a couple of safety valves would be prudent:

1. If IsUnderPostmaster, fall out after the first error, same as now.

2. Even in the postmaster, count the number of errors reported,
and give up after say 100.  By that point it's much more likely
that you're reading War and Peace than that you're continuing to
contribute to the enlightenment of the DBA.
        regards, tom lane