Re: proposal: a validator for configuration files

Hi,

I'd like to add an ability to validate the corectness of PostgreSQL
configuration files, i.e. postgresql.conf, pg_hba.conf, pg_ident.conf without
actually applying them. The idea is that it would be a command-line option to
postgres for a stand-alone case, or a user-callable function when postmaster
is running.

Per the former discussion of a validator for PostgreSQL configuration files
(see http://archives.postgresql.org/pgsql-hackers/2008-08/msg00048.php),
here's an implementation proposal. The development plan consists of 2 parts.
The first one is to add new code that would allow running the checks in both a
stand-alone process, when postmaster is not running, and as a function call
from a backend postgres process. Initially the code would simply loads
configuration files, without performing any of the validation checks. The
second part consists of adding specific checks.

I think most of the code related to this feature should be put into the
auxiliary process. The rationale is that we already have a stand-alone
CheckerProcess, which nowadays only parses postgresql.conf, runs BaseInit and
exists. We can easily load pg_hba.conf and pg_ident.conf and run all the
necessary checks there. Moreover, the same process can be used when checks are
launched from a built-in function. In that case, it would save the postgres
backend from reloading postgresql.conf, pg_hba.conf and pg_ident.conf
internally and restoring the previous configuration options when the function
exists. Below is a more detailed description of implementation steps:

1.1. Stand-alone process (postmaster is not running):

- Add a new option (--check-config) to main.c. Run AuxiliaryProcessMain with
auxType= CheckerProcess if this option is specified.
- In CheckerModeMain load pg_hba.conf, pg_ident.conf

1.2. Built-in function, called from a backend process.
- Add a new built-in function
- Add LastCheckState shared flag to denote whether the check has been
successful or failed
- Add a new PMSignalReason
- From the function call SendPostmasterSignal with the reason added on the
previous step.
- In the postmaster add a check for this reason in sigusr1_handler, spawning
a checker process. Set LastCheckStatus to InProgress.
- Store current configuration options in AuxillaryProcessMain before reloading
configuration files to avoid checking for options that haven't changed.
- In AuxillaryProcessMain, modify SelectConfigFiles invocation to call it
if IsUnderPostmaster = true if auxType process type is CheckerProcess.
- In the CheckerModeMain, set LastCheckState to either success or failure at the
end of all checks.
- The built-in function would wait until LastCheckState changes its value from
InProgress to either Succcess or Failure, or until some predefined timeout
expires, in which case it would emit an error.

2. Add following configuration checks:

postgresql.conf:

Check that:
1. postgres can bind to listen address:port
2. unix_socket_directory has proper permissions (n/a on Windows).
3. unix_socket_group group exists on a target system (n/a Windows).
4. unix_socket_permissions are valid (write permission is not revoked from
the owner, or a group, if group is different from the user).
5. server.crt and server.key exist in the data directory if ssl is
set to true (and server is compiled with openssl)
6. krb_server_keyfile is readable by the server (if set)
7. server is not requesting more shared memory than it's available in the system.
8. shared_preload_libraries and local_preload_libraries exist.
9. synchronous_standby_names are not empty and max_wal_senders
are > 0 if synchronous_replication = true
10. all enable_* query planner parameters are on.
11. seq_page_cost <= random_page_cost, and cpu_ costs are lower than page_ costs.
12. effective_cache_size is less than the amount of physical memory available.
13. geqo is turned on
14. default_statistics_target is >= 100
15. logging collector is enabled if log destination is stderr
16. log directory either exists and writable or that the parent
directory allows creating subdris
17. track counts is on if autovacuum is enabled
18. stats_temp_directory is writeable
19. default tablespace exists (if set)
20. temp_tablespace exists (if set)
21. statement_timeout is 0 (disabled)
22. dynamic_library_path exists
23. sql_inheritance is off
24. zero_damaged_pages is off

pg_hba.conf:
Check that:
1. the server is compiled with ssl if hostssl is specified
2. ssl is not required if hostnossl is specified

- Add a Warning value to LastCheckState for cases when configuration
files were loaded successfully, but one or more validation checks have failed.

Note that these are basic checks don't require the checker process to have
access to the database, Also, with this implementation, a client won't receive
an exact error message in case validation is unsuccessful, however, it would
receive a status (success, failure, warnings), and an exact error message
would be available in the server's log. It's possible to address these
shortcomings in the future.

Ideas, suggestions are welcome.

--
Alexey Klyukin
The PostgreSQL Company - Command Prompt, Inc.

Hi!

On Wed, Mar 30, 2011 at 8:40 AM, Alexey Klyukin <alexk(at)commandprompt(dot)com> wrote:

> I'd like to add an ability to validate the corectness of PostgreSQL
> configuration files, i.e. postgresql.conf, pg_hba.conf, pg_ident.conf without
> actually applying them. The idea is that it would be a command-line option to
> postgres for a stand-alone case, or a user-callable function when postmaster
> is running.
>
> Per the former discussion of a validator for PostgreSQL configuration files
> (see http://archives.postgresql.org/pgsql-hackers/2008-08/msg00048.php),
> here's an implementation proposal.

I did a little bit of work on this, and we discussed it here:

http://archives.postgresql.org/pgsql-hackers/2009-03/msg00345.php
http://archives.postgresql.org/pgsql-hackers/2009-03/msg01142.php

Probably there's a bit of bitrot in there.

> The development plan consists of 2 parts.
> The first one is to add new code that would allow running the checks in both a
> stand-alone process, when postmaster is not running, and as a function call
> from a backend postgres process. Initially the code would simply loads
> configuration files, without performing any of the validation checks. The
> second part consists of adding specific checks.

Cool! Mine was only going to work if the system started up or was reloaded.

> I think most of the code related to this feature should be put into the
> auxiliary process. The rationale is that we already have a stand-alone
> CheckerProcess, which nowadays only parses postgresql.conf, runs BaseInit and
> exists. We can easily load pg_hba.conf and pg_ident.conf and run all the
> necessary checks there. Moreover, the same process can be used when checks are
> launched from a built-in function. In that case, it would save the postgres
> backend from reloading postgresql.conf, pg_hba.conf and pg_ident.conf
> internally and restoring the previous configuration options when the function
> exists. Below is a more detailed description of implementation steps:
>
> 1.1. Stand-alone process (postmaster is not running):
>
> - Add a new option (--check-config) to main.c. Run AuxiliaryProcessMain with
>  auxType= CheckerProcess if this option is specified.
> - In CheckerModeMain load pg_hba.conf, pg_ident.conf
>
>
> 1.2. Built-in function, called from a backend process.
> - Add a new built-in function
> - Add LastCheckState shared flag to denote whether the check has been
>  successful or failed
> - Add a new PMSignalReason
> - From the function call SendPostmasterSignal with the reason added on the
>  previous step.
> - In the postmaster add a check for this reason in sigusr1_handler, spawning
>  a checker process. Set LastCheckStatus to InProgress.
> - Store current configuration options in AuxillaryProcessMain before reloading
>  configuration files to avoid checking for options that haven't changed.
> - In AuxillaryProcessMain, modify SelectConfigFiles invocation to call it
>  if IsUnderPostmaster = true if auxType process type is CheckerProcess.
> - In the CheckerModeMain, set LastCheckState to either success or failure at the
>  end of all checks.
> - The built-in function would wait until LastCheckState changes its value from
>  InProgress to either Succcess or Failure, or until some predefined timeout
>  expires, in which case it would emit an error.
>
> 2. Add following configuration checks:
>
> postgresql.conf:
>
> Check that:
>  1. postgres can bind to listen address:port
>  2. unix_socket_directory has proper permissions (n/a on Windows).
>  3. unix_socket_group group exists on a target system (n/a Windows).
>  4. unix_socket_permissions are valid (write permission is not revoked from
>        the owner, or a group, if group is different from the user).
>  5. server.crt and server.key exist in the data directory if ssl is
>        set to true (and server is compiled with openssl)
>  6. krb_server_keyfile is readable by the server (if  set)
>  7. server is not requesting more shared memory than it's available in the system.
>  8. shared_preload_libraries and local_preload_libraries exist.
>  9. synchronous_standby_names are not empty and max_wal_senders
>        are > 0 if synchronous_replication = true
>  10. all enable_*  query planner parameters are on.
>  11. seq_page_cost <= random_page_cost, and cpu_ costs are lower than page_ costs.
>  12. effective_cache_size is less than the amount of physical memory available.
>  13. geqo is turned on
>  14. default_statistics_target is >= 100
>  15. logging collector is enabled if log destination is stderr
>  16. log directory either exists and writable or that the parent
>         directory allows creating subdris
>  17. track counts is on if autovacuum is enabled
>  18. stats_temp_directory is writeable
>  19. default tablespace exists (if set)
>  20. temp_tablespace exists (if set)
>  21. statement_timeout is 0 (disabled)
>  22. dynamic_library_path exists
>  23. sql_inheritance is off
>  24. zero_damaged_pages is off

Some of this seems not like things that postgres itself should be
doing, but more something that an external validation program (like
what Greg Smith maintains) would do.

> pg_hba.conf:
> Check that:
>  1. the server is compiled with ssl if hostssl is specified
>  2. ssl is not required if hostnossl is specified
>
> - Add a Warning value to LastCheckState for cases when configuration
> files were loaded successfully, but one or more validation checks have failed.
>
> Note that these are basic checks don't require the checker process to have
> access to the database, Also, with this implementation, a client won't receive
> an exact error message in case validation is unsuccessful, however, it would
> receive a status (success, failure, warnings), and an exact error message
> would be available in the server's log. It's possible to address these
> shortcomings in the future.
>
> Ideas, suggestions are welcome.

As I said above, some of what you've suggested seems more like a
non-postgres core thing.. maybe an extension? Or maybe offer the
option to read

My idea was to just check that settings were *valid* not that they met
some other, more subjective criteria.

-selena

--
http://chesnok.com

Hi Selena,

On Mar 30, 2011, at 11:42 PM, Selena Deckelmann wrote:

> Hi!
>
> On Wed, Mar 30, 2011 at 8:40 AM, Alexey Klyukin <alexk(at)commandprompt(dot)com> wrote:
>
>
> I did a little bit of work on this, and we discussed it here:
>
> http://archives.postgresql.org/pgsql-hackers/2009-03/msg00345.php
> http://archives.postgresql.org/pgsql-hackers/2009-03/msg01142.php
>
> Probably there's a bit of bitrot in there.

Cool, I was not aware of your work in this direction. I've updated your patch
to apply to the latest HEAD, implementing Tom Lane's suggestions (attached). I
think I'll implement the other part (reporting all invalid parameters, as
opposed to only the first one) tomorrow.

>
>> The development plan consists of 2 parts.
>> The first one is to add new code that would allow running the checks in both a
>> stand-alone process, when postmaster is not running, and as a function call
>> from a backend postgres process. Initially the code would simply loads
>> configuration files, without performing any of the validation checks. The
>> second part consists of adding specific checks.
>
> Cool! Mine was only going to work if the system started up or was reloaded.

Well, I think a stand-alone check is an easy part :)
>
> As I said above, some of what you've suggested seems more like a
> non-postgres core thing.. maybe an extension? Or maybe offer the
> option to read

Well, initially I'm going to start with just a check that configuration files
are valid, and add other checks afterwards. I think it makes sense for them
to be optional.

>
> My idea was to just check that settings were *valid* not that they met
> some other, more subjective criteria.

Well, my definition of valid configuration is not only the one that server
is able to parse and load, but also to actually apply (i.e. can bind to
a listen_address or read SSL certificate files). I agree that's not always
necessary (i.e. when checking configuration on a different server than
the one it should be applied to), so we can add a flag to turn them off.

--
Alexey Klyukin
The PostgreSQL Company - Command Prompt, Inc.

On Apr 1, 2011, at 12:08 AM, Alexey Klyukin wrote:

> Hi Selena,
>
> On Mar 30, 2011, at 11:42 PM, Selena Deckelmann wrote:
>
>> Hi!
>>
>> On Wed, Mar 30, 2011 at 8:40 AM, Alexey Klyukin <alexk(at)commandprompt(dot)com> wrote:
>>
>>
>> I did a little bit of work on this, and we discussed it here:
>>
>> http://archives.postgresql.org/pgsql-hackers/2009-03/msg00345.php
>> http://archives.postgresql.org/pgsql-hackers/2009-03/msg01142.php
>>
>> Probably there's a bit of bitrot in there.
>
> Cool, I was not aware of your work in this direction. I've updated your patch
> to apply to the latest HEAD, implementing Tom Lane's suggestions (attached). I
> think I'll implement the other part (reporting all invalid parameters, as
> opposed to only the first one) tomorrow.

Here's the update of Selena's patch, which also shows all errors in
configuration parameters (as well as parser errors) during reload.

When I talked to Alvaro the other day he suggested starting with a stand-alone
process, which would load the postgresql.conf in a postmaster context, load
other configuration files and do some of the checks I've mentioned in my
initial proposal (particularly it would check that system's shared memory
limit is high enough by trying to allocate a new shared memory segment).
Afterwards, I'd like to implement checks from a user-callable function, and
not all checks would be available from it.

--
Alexey Klyukin
The PostgreSQL Company - Command Prompt, Inc.

On Mon, Apr 4, 2011 at 2:03 PM, Alexey Klyukin <alexk(at)commandprompt(dot)com> wrote:
> Here's the update of Selena's patch, which also shows all errors in
> configuration parameters (as well as parser errors) during reload.

You should add this here:

https://commitfest.postgresql.org/action/commitfest_view/open

On a quick glance, this patch appears to contain some superfluous
hunks where you changed whitespace or variable names. You might want
to remove those and repost before adding to the CF app. Also, some
submission notes would be very helpful - when you send in the revised
version, detail in the email the exact purpose of the changes so that
someone can review the patch without having to read this thread and
all preceding threads in their entirety.

Thanks,

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Hi,

On Apr 14, 2011, at 9:50 PM, Robert Haas wrote:

> On Mon, Apr 4, 2011 at 2:03 PM, Alexey Klyukin <alexk(at)commandprompt(dot)com> wrote:
>> Here's the update of Selena's patch, which also shows all errors in
>> configuration parameters (as well as parser errors) during reload.
>
> You should add this here:
>
> https://commitfest.postgresql.org/action/commitfest_view/open
>
> On a quick glance, this patch appears to contain some superfluous
> hunks where you changed whitespace or variable names. You might want
> to remove those and repost before adding to the CF app. Also, some
> submission notes would be very helpful - when you send in the revised
> version, detail in the email the exact purpose of the changes so that
> someone can review the patch without having to read this thread and
> all preceding threads in their entirety.

Thank you for the feedback, I've updated the patch, attached is a new version.
I'll add it to the commitfest after posting this message.

The patch forces the parser to report all errors (max 100) from the
ProcessConfigFile/ParseConfigFp. Currently, only the first parse error or an
invalid directive is reported. Reporting all of them is crucial to automatic
validation of postgres config files.

This patch is based on the one submitted earlier by Selena Deckelmann:
http://archives.postgresql.org/pgsql-hackers/2009-03/msg00345.php

It incorporates suggestions by Tom Lane for avoiding excessive bloat in logs
in case there is a junk instead of postgresql.conf.
http://archives.postgresql.org/pgsql-hackers/2009-03/msg01142.php

Regards,
--
Alexey Klyukin
The PostgreSQL Company - Command Prompt, Inc.

Hi

On May14, 2011, at 00:49 , Alexey Klyukin wrote:
> The patch forces the parser to report all errors (max 100) from the
> ProcessConfigFile/ParseConfigFp. Currently, only the first parse error or an
> invalid directive is reported. Reporting all of them is crucial to automatic
> validation of postgres config files.
>
> This patch is based on the one submitted earlier by Selena Deckelmann:
> http://archives.postgresql.org/pgsql-hackers/2009-03/msg00345.php
>
> It incorporates suggestions by Tom Lane for avoiding excessive bloat in logs
> in case there is a junk instead of postgresql.conf.
> http://archives.postgresql.org/pgsql-hackers/2009-03/msg01142.php

Here's my review of your patch.

The patch is in context diff format and applies cleanly to HEAD. It doesn't
contain superfluous whitespace changes any more is and quite readable.

First, the behaviour.

The first problem I ran into when I tried to test this is that it *only*
reports multiple errors during config file reload on SIHUP, not during
postmaster startup. I guess it's been done that way because we
ereport(ERROR,..) not ereport(LOG,...) during postmaster startup, so it's
not immediatly clear how to report multiple errors. But that proplem
seems solvable. What if you ereport(LOG,..)ed the individual errors during
postmaster startup, and then emitted an ereport(ERROR) at the end if
errors occurred? The ERROR could either repeat the first error that was
encountered, or simply say "config file contains errors".

Now to the code.

I see that you basically replaced "goto cleanup..." in both ParseConfigFp()
and ProcessConfigFile() with "++errorcount", and arranged for ParseConfigFp()
to return false, and for ProcessConfigFile() to skip the GUC updates if
"errorcount > 0". The actual value of errorcount is never inspected. The value
is also wrong in the case of include files containing more than error, since
ParseConfigFp() simply increments errorcount by one for each failed
ParseConfigFile() of an included file.

I thus suggest that you replace "errorcount" with a boolean "erroroccurred".

You've also introduced a bug in ParseConfigFp(). Previously, if an included
file contained an error, it did "goto cleanup_exit()" and thus didn't
ereport() on it's own. With your patch applied it ereport()s a parse error
at the location of the "include" directive, which seems wrong.

Finally, I believe that ParseConfigFp() should make at least some effort to
resync after hitting a parser error. I suggest that you simply fast-forward
to the next GUC_EOL token after reporting a parser error.

best regards,
Florian Pflug

Florian,

On Jun 16, 2011, at 2:34 PM, Florian Pflug wrote:

> Hi
>
> On May14, 2011, at 00:49 , Alexey Klyukin wrote:
>> The patch forces the parser to report all errors (max 100) from the
>> ProcessConfigFile/ParseConfigFp. Currently, only the first parse error or an
>> invalid directive is reported. Reporting all of them is crucial to automatic
>> validation of postgres config files.
>>
>> This patch is based on the one submitted earlier by Selena Deckelmann:
>> http://archives.postgresql.org/pgsql-hackers/2009-03/msg00345.php
>>
>> It incorporates suggestions by Tom Lane for avoiding excessive bloat in logs
>> in case there is a junk instead of postgresql.conf.
>> http://archives.postgresql.org/pgsql-hackers/2009-03/msg01142.php
>
> Here's my review of your patch.
>
> The patch is in context diff format and applies cleanly to HEAD. It doesn't
> contain superfluous whitespace changes any more is and quite readable.
>
> First, the behaviour.
>
> The first problem I ran into when I tried to test this is that it *only*
> reports multiple errors during config file reload on SIHUP, not during
> postmaster startup. I guess it's been done that way because we
> ereport(ERROR,..) not ereport(LOG,...) during postmaster startup, so it's
> not immediatly clear how to report multiple errors. But that proplem
> seems solvable. What if you ereport(LOG,..)ed the individual errors during
> postmaster startup, and then emitted an ereport(ERROR) at the end if
> errors occurred? The ERROR could either repeat the first error that was
> encountered, or simply say "config file contains errors".

Makes sense. One problem I came across is that set_config_option from guc.c
sets the elevel by itself. I've changed it to emit LOG errors on PGC_S_FILE
source, apparently all the callers of this function with this source are from
guc-file.l, so hopefully I won't break anything with this change.

>
> Now to the code.
>
> I see that you basically replaced "goto cleanup..." in both ParseConfigFp()
> and ProcessConfigFile() with "++errorcount", and arranged for ParseConfigFp()
> to return false, and for ProcessConfigFile() to skip the GUC updates if
> "errorcount > 0". The actual value of errorcount is never inspected. The value
> is also wrong in the case of include files containing more than error, since
> ParseConfigFp() simply increments errorcount by one for each failed
> ParseConfigFile() of an included file.
>
> I thus suggest that you replace "errorcount" with a boolean "erroroccurred".

I can actually pass errorcount down from the ParseConfigFp() to report the total
number of errors (w/ the include files) at the end of ProcessConfigFile if there is
any interest in having the number of errors reported to a user. If not - I'll change
it to boolean.

>
> You've also introduced a bug in ParseConfigFp(). Previously, if an included
> file contained an error, it did "goto cleanup_exit()" and thus didn't
> ereport() on it's own. With your patch applied it ereport()s a parse error
> at the location of the "include" directive, which seems wrong.

Right, I noticed that I skipped switching the buffers and restoring the Lineno
as well. I'll fix it in the next revision.

>
> Finally, I believe that ParseConfigFp() should make at least some effort to
> resync after hitting a parser error. I suggest that you simply fast-forward
> to the next GUC_EOL token after reporting a parser error.

Makes sense.

Thank you for the review.

I'll post an updated patch, addressing you concerns, shortly.

Alexey.
--
Command Prompt, Inc. http://www.CommandPrompt.com
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

On Jun16, 2011, at 17:23 , Alexey Klyukin wrote:
> On Jun 16, 2011, at 2:34 PM, Florian Pflug wrote:
>> The first problem I ran into when I tried to test this is that it *only*
>> reports multiple errors during config file reload on SIHUP, not during
>> postmaster startup. I guess it's been done that way because we
>> ereport(ERROR,..) not ereport(LOG,...) during postmaster startup, so it's
>> not immediatly clear how to report multiple errors. But that proplem
>> seems solvable. What if you ereport(LOG,..)ed the individual errors during
>> postmaster startup, and then emitted an ereport(ERROR) at the end if
>> errors occurred? The ERROR could either repeat the first error that was
>> encountered, or simply say "config file contains errors".
>
> Makes sense. One problem I came across is that set_config_option from guc.c
> sets the elevel by itself. I've changed it to emit LOG errors on PGC_S_FILE
> source, apparently all the callers of this function with this source are from
> guc-file.l, so hopefully I won't break anything with this change.

Hm, wouldn't a test for "context == PGC_POSTMASTER" be more appropriate?

>> I see that you basically replaced "goto cleanup..." in both ParseConfigFp()
>> and ProcessConfigFile() with "++errorcount", and arranged for ParseConfigFp()
>> to return false, and for ProcessConfigFile() to skip the GUC updates if
>> "errorcount > 0". The actual value of errorcount is never inspected. The value
>> is also wrong in the case of include files containing more than error, since
>> ParseConfigFp() simply increments errorcount by one for each failed
>> ParseConfigFile() of an included file.
>>
>> I thus suggest that you replace "errorcount" with a boolean "erroroccurred".
>
> I can actually pass errorcount down from the ParseConfigFp() to report the total
> number of errors (w/ the include files) at the end of ProcessConfigFile if there is
> any interest in having the number of errors reported to a user. If not - I'll change
> it to boolean.

Nah, just use a boolean, unless you have concrete plans to actually use the errorcount
for something other than test a la "errorcount > 0".

best regards,
Florian Pflug

On Jun 16, 2011, at 6:49 PM, Florian Pflug wrote:

> On Jun16, 2011, at 17:23 , Alexey Klyukin wrote:
>> On Jun 16, 2011, at 2:34 PM, Florian Pflug wrote:
>>> The first problem I ran into when I tried to test this is that it *only*
>>> reports multiple errors during config file reload on SIHUP, not during
>>> postmaster startup. I guess it's been done that way because we
>>> ereport(ERROR,..) not ereport(LOG,...) during postmaster startup, so it's
>>> not immediatly clear how to report multiple errors. But that proplem
>>> seems solvable. What if you ereport(LOG,..)ed the individual errors during
>>> postmaster startup, and then emitted an ereport(ERROR) at the end if
>>> errors occurred? The ERROR could either repeat the first error that was
>>> encountered, or simply say "config file contains errors".
>>
>> Makes sense. One problem I came across is that set_config_option from guc.c
>> sets the elevel by itself. I've changed it to emit LOG errors on PGC_S_FILE
>> source, apparently all the callers of this function with this source are from
>> guc-file.l, so hopefully I won't break anything with this change.
>
> Hm, wouldn't a test for "context == PGC_POSTMASTER" be more appropriate?

In such a case the errors caused by command-line arguments won't stop the postmaster.
PGC_S_FILE seems to handle this correctly. I'm not sure whether it is appropriate to use
there though.

>
>>> I see that you basically replaced "goto cleanup..." in both ParseConfigFp()
>>> and ProcessConfigFile() with "++errorcount", and arranged for ParseConfigFp()
>>> to return false, and for ProcessConfigFile() to skip the GUC updates if
>>> "errorcount > 0". The actual value of errorcount is never inspected. The value
>>> is also wrong in the case of include files containing more than error, since
>>> ParseConfigFp() simply increments errorcount by one for each failed
>>> ParseConfigFile() of an included file.
>>>
>>> I thus suggest that you replace "errorcount" with a boolean "erroroccurred".
>>
>> I can actually pass errorcount down from the ParseConfigFp() to report the total
>> number of errors (w/ the include files) at the end of ProcessConfigFile if there is
>> any interest in having the number of errors reported to a user. If not - I'll change
>> it to boolean.
>
> Nah, just use a boolean, unless you have concrete plans to actually use the errorcount
> for something other than test a la "errorcount > 0".

I just recalled a reason for counting the total number of errors. There is a condition that
checks that the total number of errors is less than 100 and bails out if it's more than that
(100 is arbitrary). The reason is to avoid bloating the logs w/ something totally unrelated
to postgresql.conf. That was suggested by Tom Lane here:
http://archives.postgresql.org/pgsql-hackers/2009-03/msg01142.php

Thank you,
Alexey.

--
Command Prompt, Inc. http://www.CommandPrompt.com
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

On Jun16, 2011, at 18:46 , Alexey Klyukin wrote:
> On Jun 16, 2011, at 6:49 PM, Florian Pflug wrote:
>> Hm, wouldn't a test for "context == PGC_POSTMASTER" be more appropriate?
>
> In such a case the errors caused by command-line arguments won't stop the postmaster.
> PGC_S_FILE seems to handle this correctly. I'm not sure whether it is appropriate to use
> there though.

Ah, yeah, you're right. PGC_S_FILE sounds fine, then. I guess this means you can
drop the check for "context == PGC_SIGHUP" though, because surely the source must
be PGC_S_DEFAULT or PGC_S_FILE if context == PGC_SIGHUP, right? So the check would
become
if (source == PGC_S_FILE || source == PGC_S_DEFAULT)
where it now says
if (context == PGC_SIGHUP || source == PGC_S_DEFAULT)

>>>> I see that you basically replaced "goto cleanup..." in both ParseConfigFp()
>>>> and ProcessConfigFile() with "++errorcount", and arranged for ParseConfigFp()
>>>> to return false, and for ProcessConfigFile() to skip the GUC updates if
>>>> "errorcount > 0". The actual value of errorcount is never inspected. The value
>>>> is also wrong in the case of include files containing more than error, since
>>>> ParseConfigFp() simply increments errorcount by one for each failed
>>>> ParseConfigFile() of an included file.
>>>>
>>>> I thus suggest that you replace "errorcount" with a boolean "erroroccurred".
>>>
>>> I can actually pass errorcount down from the ParseConfigFp() to report the total
>>> number of errors (w/ the include files) at the end of ProcessConfigFile if there is
>>> any interest in having the number of errors reported to a user. If not - I'll change
>>> it to boolean.
>>
>> Nah, just use a boolean, unless you have concrete plans to actually use the errorcount
>> for something other than test a la "errorcount > 0".
>
> I just recalled a reason for counting the total number of errors. There is a condition that
> checks that the total number of errors is less than 100 and bails out if it's more than that
> (100 is arbitrary). The reason is to avoid bloating the logs w/ something totally unrelated
> to postgresql.conf. That was suggested by Tom Lane here:
> http://archives.postgresql.org/pgsql-hackers/2009-03/msg01142.php

Ah, right, I missed that. Guess it'll have to stay a counter, then. Still, I don't think it's
worth the effort to make the count correct in case of included files, so I'd say just add
a comment explaining that the count isn't totally accurate.

best regards,
Florian Pflug

On Jun 16, 2011, at 8:01 PM, Florian Pflug wrote:

> On Jun16, 2011, at 18:46 , Alexey Klyukin wrote:
>> On Jun 16, 2011, at 6:49 PM, Florian Pflug wrote:
>>> Hm, wouldn't a test for "context == PGC_POSTMASTER" be more appropriate?
>>
>> In such a case the errors caused by command-line arguments won't stop the postmaster.
>> PGC_S_FILE seems to handle this correctly. I'm not sure whether it is appropriate to use
>> there though.
>
> Ah, yeah, you're right. PGC_S_FILE sounds fine, then. I guess this means you can
> drop the check for "context == PGC_SIGHUP" though, because surely the source must
> be PGC_S_DEFAULT or PGC_S_FILE if context == PGC_SIGHUP, right? So the check would
> become
> if (source == PGC_S_FILE || source == PGC_S_DEFAULT)
> where it now says
> if (context == PGC_SIGHUP || source == PGC_S_DEFAULT)

Yes, AFAIK PGC_SIGHUP is redundant, thank you for the suggestion.

>>
>> I just recalled a reason for counting the total number of errors. There is a condition that
>> checks that the total number of errors is less than 100 and bails out if it's more than that
>> (100 is arbitrary). The reason is to avoid bloating the logs w/ something totally unrelated
>> to postgresql.conf. That was suggested by Tom Lane here:
>> http://archives.postgresql.org/pgsql-hackers/2009-03/msg01142.php
>
> Ah, right, I missed that. Guess it'll have to stay a counter, then. Still, I don't think it's
> worth the effort to make the count correct in case of included files, so I'd say just add
> a comment explaining that the count isn't totally accurate.

Well, while thinking about this I decided to leave the counter for the ParseConfigFp, but
drop it in ProcessConfigFile. The case we are protecting against is a single file full of junk.
It's unlikely that this junk would contain include directives with valid file paths, neither it's
likely to find a file with a correct syntax, but full of invalid directives.

Thank you,
Alexey.

--
Command Prompt, Inc. http://www.CommandPrompt.com
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

On Jun16, 2011, at 20:14 , Alexey Klyukin wrote:
> On Jun 16, 2011, at 8:01 PM, Florian Pflug wrote:
>> On Jun16, 2011, at 18:46 , Alexey Klyukin wrote:
>>> I just recalled a reason for counting the total number of errors. There is a condition that
>>> checks that the total number of errors is less than 100 and bails out if it's more than that
>>> (100 is arbitrary). The reason is to avoid bloating the logs w/ something totally unrelated
>>> to postgresql.conf. That was suggested by Tom Lane here:
>>> http://archives.postgresql.org/pgsql-hackers/2009-03/msg01142.php
>>
>> Ah, right, I missed that. Guess it'll have to stay a counter, then. Still, I don't think it's
>> worth the effort to make the count correct in case of included files, so I'd say just add
>> a comment explaining that the count isn't totally accurate.
>
> Well, while thinking about this I decided to leave the counter for the ParseConfigFp, but
> drop it in ProcessConfigFile. The case we are protecting against is a single file full of junk.
> It's unlikely that this junk would contain include directives with valid file paths, neither it's
> likely to find a file with a correct syntax, but full of invalid directives.

Sounds good.

best regards,
Florian Pflug

Hi,

On Jun 16, 2011, at 9:18 PM, Florian Pflug wrote:

> On Jun16, 2011, at 20:14 , Alexey Klyukin wrote:
>>
>> Well, while thinking about this I decided to leave the counter for the ParseConfigFp, but
>> drop it in ProcessConfigFile. The case we are protecting against is a single file full of junk.
>> It's unlikely that this junk would contain include directives with valid file paths, neither it's
>> likely to find a file with a correct syntax, but full of invalid directives.
>
> Sounds good.

Attached is the v2 of the patch to show all parse errors at postgresql.conf.
Changes (per review and suggestions from Florian):

- do not stop on the first error during postmaster's start.
- fix errors in processing files from include directives.
- show only a single syntax error per line, i.e. fast forward to the EOL after coming across the first one.
- additional comments/error messages, code cleanup

Questions:

- Should we add a comment for the changes in guc.c? I think the existing ones are still valid, but they might be harder go grasp, given that we've removed PGC_SIGHUP from the condition.
- The error message that we emit when the parsing is unsuccessful, will it cause incompatibility w/ 3rd party tools, which may, in theory, show only one error message (would it better to show the first error instead, as proposed by Florian?).

I'd appreciate your comments and suggestions.

Thank you,
Alexey.

--
Command Prompt, Inc. http://www.CommandPrompt.com
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

On Jun16, 2011, at 22:34 , Alexey Klyukin wrote:
> Attached is the v2 of the patch to show all parse errors at postgresql.conf.
> Changes (per review and suggestions from Florian):
>
> - do not stop on the first error during postmaster's start.
> - fix errors in processing files from include directives.
> - show only a single syntax error per line, i.e. fast forward to the EOL after coming across the first one.
> - additional comments/error messages, code cleanup

Looks mostly good.

I found one issue which I missed earlier. As it stands, the behaviour
of ProcessConfigFile() changes subtly when IsUnderPostmaster because of
the early abort on errors. Now, in theory the outcome should still be
the same, since we don't apply any settings if there's an error in one
of them. But still, there's a risk that this code isn't 100% waterproof,
and then we'd end up with different settings in the postmaster compared
to the backends. The benefit seems also quite small - since the backends
emit their messages at DEBUG2, you usually won't see the difference
anyway.

The elevel setting at the start of ProcessConfigFile() also seemed
needlessly complex, since we cannot have IsUnderPostmaster and
context == PGCS_POSTMASTER at the same time.

I figured it'd be harder to explain the fixes I have in mind than
simply do them and let the code speak. Attached you'll find an updated
version of your v2 patch (called v2a) as well as an incremental patch
against your v2 (called v2a_delta).

The main changes are the removal of the early aborts when
IsUnderPostmaster and the simplification of the elevel setting
logic in ProcessConfigFile().

The updated patch also adds a few comments. If you agree with my changes,
feel free to mark this patch "Ready for Committer".

best regards,
Florian Pflug

Florian,

On Jun 18, 2011, at 5:40 PM, Florian Pflug wrote:

> On Jun16, 2011, at 22:34 , Alexey Klyukin wrote:
>> Attached is the v2 of the patch to show all parse errors at postgresql.conf.
>> Changes (per review and suggestions from Florian):
>>
>> - do not stop on the first error during postmaster's start.
>> - fix errors in processing files from include directives.
>> - show only a single syntax error per line, i.e. fast forward to the EOL after coming across the first one.
>> - additional comments/error messages, code cleanup
>
> Looks mostly good.
>
> I found one issue which I missed earlier. As it stands, the behaviour
> of ProcessConfigFile() changes subtly when IsUnderPostmaster because of
> the early abort on errors. Now, in theory the outcome should still be
> the same, since we don't apply any settings if there's an error in one
> of them. But still, there's a risk that this code isn't 100% waterproof,
> and then we'd end up with different settings in the postmaster compared
> to the backends. The benefit seems also quite small - since the backends
> emit their messages at DEBUG2, you usually won't see the difference
> anyway.

I don't think it has changed at all. Previously, we did goto cleanup_list (or
cleanup_exit in ParseConfigFp) right after the first error, no matter whether
that was a postmaster or its child. What I did in my patch is removing the
goto for the postmaster's case. It was my intention to exit after the initial
error for the postmaster's child, to avoid complaining about all errors both
in the postmaster and in the normal backend (imagine seeing 100 errors from
the postmaster and the same 100 from each of the backends if your log level is
DEBUG2). I think the postmaster's child case won't cause any problems, since
we do exactly what we used to do before.

>
> The elevel setting at the start of ProcessConfigFile() also seemed
> needlessly complex, since we cannot have IsUnderPostmaster and
> context == PGCS_POSTMASTER at the same time.

Agreed.

>
> I figured it'd be harder to explain the fixes I have in mind than
> simply do them and let the code speak. Attached you'll find an updated
> version of your v2 patch (called v2a) as well as an incremental patch
> against your v2 (called v2a_delta).
>
> The main changes are the removal of the early aborts when
> IsUnderPostmaster and the simplification of the elevel setting
> logic in ProcessConfigFile().

Attached is the new patch and the delta. It includes some of the changes from
your patch, while leaving the early abort stuff for postmaster's children.

Thank you,
Alexey.

--
Command Prompt, Inc. http://www.CommandPrompt.com
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

On Jun20, 2011, at 17:02 , Alexey Klyukin wrote:
> On Jun 18, 2011, at 5:40 PM, Florian Pflug wrote:
>> On Jun16, 2011, at 22:34 , Alexey Klyukin wrote:
>>> Attached is the v2 of the patch to show all parse errors at postgresql.conf.
>>> Changes (per review and suggestions from Florian):
>>>
>>> - do not stop on the first error during postmaster's start.
>>> - fix errors in processing files from include directives.
>>> - show only a single syntax error per line, i.e. fast forward to the EOL after coming across the first one.
>>> - additional comments/error messages, code cleanup
>>
>> Looks mostly good.
>>
>> I found one issue which I missed earlier. As it stands, the behaviour
>> of ProcessConfigFile() changes subtly when IsUnderPostmaster because of
>> the early abort on errors. Now, in theory the outcome should still be
>> the same, since we don't apply any settings if there's an error in one
>> of them. But still, there's a risk that this code isn't 100% waterproof,
>> and then we'd end up with different settings in the postmaster compared
>> to the backends. The benefit seems also quite small - since the backends
>> emit their messages at DEBUG2, you usually won't see the difference
>> anyway.
>
> I don't think it has changed at all. Previously, we did goto cleanup_list (or
> cleanup_exit in ParseConfigFp) right after the first error, no matter whether
> that was a postmaster or its child. What I did in my patch is removing the
> goto for the postmaster's case. It was my intention to exit after the initial
> error for the postmaster's child, to avoid complaining about all errors both
> in the postmaster and in the normal backend (imagine seeing 100 errors from
> the postmaster and the same 100 from each of the backends if your log level is
> DEBUG2). I think the postmaster's child case won't cause any problems, since
> we do exactly what we used to do before.

Hm, I think you miss-understood what I was trying to say, probably because I
explained it badly. Let me try again.

I fully agree that there *shouldn't* be any difference in behaviour, because
it *shouldn't* matter whether we abort early or not - we won't have applied
any of the settings anway.

But.

The code the actually implements the "check settings first, apply later" logic
isn't easy to read. Now, assume that this code has a bug. Then, with your
patch applied, we might end up with the postmaster applying a setting (because
it didn't abort early) but the backend ignoring it (because they did abort early).
This is obviously bad. Depending on the setting, the consequences may range
from slightly confusing behaviour to outright crashes I guess...

Now, the risk of that happening might be very small. But on the other hand,
the benefit is pretty small also - you get a little less output for log level
DEBUG2, that's it. A level that people probably don't use for the production
databases anyway. This convinced me that the risk/benefit ratio isn't high enough
to warrant the early abort.

Another benefit of removing the check is that it reduces code complexity. Maybe
not when measured in line counts, but it's one less outside factor that changes
ProcessConfigFiles()'s behaviour and thus one thing less you need to think when
you modify that part again in the future. Again, it's a small benefit, but IMHO
it still outweights the benefit.

Having said that, this is my personal opinion and whoever will eventually
commit this may very will assess the cost/benefit ratio differently. So, if
after this more detailed explanations of my reasoning, you still feel that
it makes sense to keep the early abort, then feel free to mark the
patch "Ready for Committer" nevertheless.

best regards,
Florian Pflug

Florian Pflug <fgp(at)phlo(dot)org> writes:
> The code the actually implements the "check settings first, apply later" logic
> isn't easy to read. Now, assume that this code has a bug. Then, with your
> patch applied, we might end up with the postmaster applying a setting (because
> it didn't abort early) but the backend ignoring it (because they did abort early).
> This is obviously bad. Depending on the setting, the consequences may range
> from slightly confusing behaviour to outright crashes I guess...

This is already known to happen: there are cases where the postmaster
and a backend can come to different conclusions about whether a setting
is valid (eg, because it depends on database encoding). Whether that's
a bug or not isn't completely clear, but if this patch is critically
dependent on the situation never happening, I don't think we can accept
it.

regards, tom lane

On Jun20, 2011, at 18:16 , Tom Lane wrote:
> Florian Pflug <fgp(at)phlo(dot)org> writes:
>> The code the actually implements the "check settings first, apply later" logic
>> isn't easy to read. Now, assume that this code has a bug. Then, with your
>> patch applied, we might end up with the postmaster applying a setting (because
>> it didn't abort early) but the backend ignoring it (because they did abort early).
>> This is obviously bad. Depending on the setting, the consequences may range
>> from slightly confusing behaviour to outright crashes I guess...
>
> This is already known to happen: there are cases where the postmaster
> and a backend can come to different conclusions about whether a setting
> is valid (eg, because it depends on database encoding). Whether that's
> a bug or not isn't completely clear, but if this patch is critically
> dependent on the situation never happening, I don't think we can accept
> it.

Does that mean that some backends might currently choose to ignore an
updated config file wholesale on SIGUP (because some settings are invalid)
while others happily apply it? Meaning that they'll afterwards disagree
even on modified settings which *would* be valid for both backends?

Or do these kinds of setting rejections happen late enough to fall out
of the all-or-nothing logic in ProcessConfigFile?

Anyway, the patch *doesn't* depend on all backends's setting being in sync.
The issue we were discussion was whether it's OK to add another small risk
of them getting out of sync by aborting early on errors in backends but
not in the postmaster. I was arguing against that, while Alexey was in favour
of it, on the grounds that it reduces log traffic (but only at DEBUG2 or
beyond).

best regards,
Florian Pflug

Florian Pflug <fgp(at)phlo(dot)org> writes:
> On Jun20, 2011, at 18:16 , Tom Lane wrote:
>> This is already known to happen: there are cases where the postmaster
>> and a backend can come to different conclusions about whether a setting
>> is valid (eg, because it depends on database encoding). Whether that's
>> a bug or not isn't completely clear, but if this patch is critically
>> dependent on the situation never happening, I don't think we can accept
>> it.

> Does that mean that some backends might currently choose to ignore an
> updated config file wholesale on SIGUP (because some settings are invalid)
> while others happily apply it? Meaning that they'll afterwards disagree
> even on modified settings which *would* be valid for both backends?

Yes. I complained about that before:
http://archives.postgresql.org/pgsql-hackers/2011-04/msg00330.php
but we didn't come to any consensus about fixing it. This patch might
be a good vehicle for revisiting the issue, though.

regards, tom lane

On Jun 20, 2011, at 6:22 PM, Florian Pflug wrote:

> On Jun20, 2011, at 17:02 , Alexey Klyukin wrote:
>>
>> I don't think it has changed at all. Previously, we did goto cleanup_list (or
>> cleanup_exit in ParseConfigFp) right after the first error, no matter whether
>> that was a postmaster or its child. What I did in my patch is removing the
>> goto for the postmaster's case. It was my intention to exit after the initial
>> error for the postmaster's child, to avoid complaining about all errors both
>> in the postmaster and in the normal backend (imagine seeing 100 errors from
>> the postmaster and the same 100 from each of the backends if your log level is
>> DEBUG2). I think the postmaster's child case won't cause any problems, since
>> we do exactly what we used to do before.
>
> Hm, I think you miss-understood what I was trying to say, probably because I
> explained it badly. Let me try again.
>
> I fully agree that there *shouldn't* be any difference in behaviour, because
> it *shouldn't* matter whether we abort early or not - we won't have applied
> any of the settings anway.
>
> But.
>
> The code the actually implements the "check settings first, apply later" logic
> isn't easy to read. Now, assume that this code has a bug. Then, with your
> patch applied, we might end up with the postmaster applying a setting (because
> it didn't abort early) but the backend ignoring it (because they did abort early).
> This is obviously bad. Depending on the setting, the consequences may range
> from slightly confusing behaviour to outright crashes I guess...
>
> Now, the risk of that happening might be very small. But on the other hand,
> the benefit is pretty small also - you get a little less output for log level
> DEBUG2, that's it. A level that people probably don't use for the production
> databases anyway. This convinced me that the risk/benefit ratio isn't high enough
> to warrant the early abort.
>
> Another benefit of removing the check is that it reduces code complexity. Maybe
> not when measured in line counts, but it's one less outside factor that changes
> ProcessConfigFiles()'s behaviour and thus one thing less you need to think when
> you modify that part again in the future. Again, it's a small benefit, but IMHO
> it still outweights the benefit.

While I agree that making the code potentially less bug prone is a good idea,
I don't agree with the statement that since DEBUG2 output gets rarely turned
on we can make it less usable, hoping that nobody would use in production.

>
> Having said that, this is my personal opinion and whoever will eventually
> commit this may very will assess the cost/benefit ratio differently. So, if
> after this more detailed explanations of my reasoning, you still feel that
> it makes sense to keep the early abort, then feel free to mark the
> patch "Ready for Committer" nevertheless.

I'd say that this issue is a judgement call. Depending on a point of view, both
arguments are valid. I've marked this patch as 'Ready for Committer' w/o
removing the early abort stuff, but if there will be more people willing to remove
them - I'll do that.

Thank you,
Alexey.

--
Command Prompt, Inc. http://www.CommandPrompt.com
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Excerpts from Alexey Kluykin's message of mar jun 21 07:43:02 -0400 2011:

> > Another benefit of removing the check is that it reduces code complexity. Maybe
> > not when measured in line counts, but it's one less outside factor that changes
> > ProcessConfigFiles()'s behaviour and thus one thing less you need to think when
> > you modify that part again in the future. Again, it's a small benefit, but IMHO
> > it still outweights the benefit.
>
> While I agree that making the code potentially less bug prone is a good idea,
> I don't agree with the statement that since DEBUG2 output gets rarely turned
> on we can make it less usable, hoping that nobody would use in production.

I would have sided with Florian on this issue, but Tom commented
otherwise somewhere, and I think his rationale makes sense, so I left
the patch as Alexey had it.

I also tweaked the patch so that it really stops processing after 100
errors, say if you include file A which has 50 errors and then file B
which has another 50 errors; instead of looking for 98 more errors, we
just give up at that point. It makes more sense to me anyway, and it
doesn't seem to add any code complexity. Also, if you have already seen
98 errors, it doesn't make sense to let another file contain 100 errors
more, so with this version, that 98 is passed down so that only 2 more
errors are allowed. I had to touch the other callers of ParseConfigFile
but it is clean enough.

I also made the code barf when bailing out of one of those loops. One
strange thing here is that you could get two such messages; say if a
file has 100 parse errors and there are also valid lines that contain
bogus settings (foo = bar). I don't find this to be too problematic,
and I think fixing it would be excessively annoying.

For example, a bogus run would end like this:

95 LOG: syntax error in file "/pgsql/install/HEAD/data/postgresql.conf" line 4, near end of line
96 LOG: syntax error in file "/pgsql/install/HEAD/data/postgresql.conf" line 41, near end of line
97 LOG: syntax error in file "/pgsql/install/HEAD/data/postgresql.conf" line 104, near end of line
98 LOG: syntax error in file "/pgsql/install/HEAD/data/postgresql.conf" line 156, near end of line
99 LOG: syntax error in file "/pgsql/install/HEAD/data/postgresql.conf" line 208, near end of line
100 LOG: syntax error in file "/pgsql/install/HEAD/data/postgresql.conf" line 260, near end of line
101 LOG: too many errors found, stopped processing file "/pgsql/install/HEAD/data/postgresql.conf"
102 LOG: unrecognized configuration parameter "plperl.err"
103 LOG: unrecognized configuration parameter "this1"
104 LOG: too many errors found, stopped processing file "/pgsql/install/HEAD/data/postgresql.conf"
105 FATAL: errors detected while parsing configuration files

One thing I don't like is that those "unrecognized configuration
parameter" messages don't say what file those parameters were found in.
That's material for a different patch however.

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

On Jul14, 2011, at 01:38 , Alvaro Herrera wrote:
> One strange thing here is that you could get two such messages; say if a
> file has 100 parse errors and there are also valid lines that contain
> bogus settings (foo = bar). I don't find this to be too problematic,
> and I think fixing it would be excessively annoying.
>
> For example, a bogus run would end like this:
>
> 95 LOG: syntax error in file "/pgsql/install/HEAD/data/postgresql.conf" line 4, near end of line
> 96 LOG: syntax error in file "/pgsql/install/HEAD/data/postgresql.conf" line 41, near end of line
> 97 LOG: syntax error in file "/pgsql/install/HEAD/data/postgresql.conf" line 104, near end of line
> 98 LOG: syntax error in file "/pgsql/install/HEAD/data/postgresql.conf" line 156, near end of line
> 99 LOG: syntax error in file "/pgsql/install/HEAD/data/postgresql.conf" line 208, near end of line
> 100 LOG: syntax error in file "/pgsql/install/HEAD/data/postgresql.conf" line 260, near end of line
> 101 LOG: too many errors found, stopped processing file "/pgsql/install/HEAD/data/postgresql.conf"
> 102 LOG: unrecognized configuration parameter "plperl.err"
> 103 LOG: unrecognized configuration parameter "this1"
> 104 LOG: too many errors found, stopped processing file "/pgsql/install/HEAD/data/postgresql.conf"
> 105 FATAL: errors detected while parsing configuration files

How about changing ParseConfigFile to say "too many *syntax* error found"
instead? It'd be more precise, and we wouldn't emit exactly the
same message twice.

Do you want me to take a closer look at your modified version of the
patch before you commit, or did you post it more as a "FYI, this is
how it's going to look like"?

best regards,
Florian Pflug

Excerpts from Florian Pflug's message of mié jul 13 20:12:28 -0400 2011:
> On Jul14, 2011, at 01:38 , Alvaro Herrera wrote:
> > One strange thing here is that you could get two such messages; say if a
> > file has 100 parse errors and there are also valid lines that contain
> > bogus settings (foo = bar). I don't find this to be too problematic,
> > and I think fixing it would be excessively annoying.
> >
> > For example, a bogus run would end like this:
> >
> > 95 LOG: syntax error in file "/pgsql/install/HEAD/data/postgresql.conf" line 4, near end of line
> > 96 LOG: syntax error in file "/pgsql/install/HEAD/data/postgresql.conf" line 41, near end of line
> > 97 LOG: syntax error in file "/pgsql/install/HEAD/data/postgresql.conf" line 104, near end of line
> > 98 LOG: syntax error in file "/pgsql/install/HEAD/data/postgresql.conf" line 156, near end of line
> > 99 LOG: syntax error in file "/pgsql/install/HEAD/data/postgresql.conf" line 208, near end of line
> > 100 LOG: syntax error in file "/pgsql/install/HEAD/data/postgresql.conf" line 260, near end of line
> > 101 LOG: too many errors found, stopped processing file "/pgsql/install/HEAD/data/postgresql.conf"
> > 102 LOG: unrecognized configuration parameter "plperl.err"
> > 103 LOG: unrecognized configuration parameter "this1"
> > 104 LOG: too many errors found, stopped processing file "/pgsql/install/HEAD/data/postgresql.conf"
> > 105 FATAL: errors detected while parsing configuration files
>
> How about changing ParseConfigFile to say "too many *syntax* error found"
> instead? It'd be more precise, and we wouldn't emit exactly the
> same message twice.

Yeah, I thought about doing it that way but refrained because it'd be
one more string to translate. That's a poor reason, I admit :-) I'll
change it.

> Do you want me to take a closer look at your modified version of the
> patch before you commit, or did you post it more as a "FYI, this is
> how it's going to look like"?

I know I'd feel more comfortable if you (and Alexey, and Selena) gave it
another look :-)

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

On Jul 14, 2011, at 4:38 AM, Alvaro Herrera wrote:

> Excerpts from Florian Pflug's message of mié jul 13 20:12:28 -0400 2011:
>> On Jul14, 2011, at 01:38 , Alvaro Herrera wrote:
>>> One strange thing here is that you could get two such messages; say if a
>>> file has 100 parse errors and there are also valid lines that contain
>>> bogus settings (foo = bar). I don't find this to be too problematic,
>>> and I think fixing it would be excessively annoying.
>>>
>>> For example, a bogus run would end like this:
>>>
>>> 95 LOG: syntax error in file "/pgsql/install/HEAD/data/postgresql.conf" line 4, near end of line
>>> 96 LOG: syntax error in file "/pgsql/install/HEAD/data/postgresql.conf" line 41, near end of line
>>> 97 LOG: syntax error in file "/pgsql/install/HEAD/data/postgresql.conf" line 104, near end of line
>>> 98 LOG: syntax error in file "/pgsql/install/HEAD/data/postgresql.conf" line 156, near end of line
>>> 99 LOG: syntax error in file "/pgsql/install/HEAD/data/postgresql.conf" line 208, near end of line
>>> 100 LOG: syntax error in file "/pgsql/install/HEAD/data/postgresql.conf" line 260, near end of line
>>> 101 LOG: too many errors found, stopped processing file "/pgsql/install/HEAD/data/postgresql.conf"
>>> 102 LOG: unrecognized configuration parameter "plperl.err"
>>> 103 LOG: unrecognized configuration parameter "this1"
>>> 104 LOG: too many errors found, stopped processing file "/pgsql/install/HEAD/data/postgresql.conf"
>>> 105 FATAL: errors detected while parsing configuration files
>>
>> How about changing ParseConfigFile to say "too many *syntax* error found"
>> instead? It'd be more precise, and we wouldn't emit exactly the
>> same message twice.
>
> Yeah, I thought about doing it that way but refrained because it'd be
> one more string to translate. That's a poor reason, I admit :-) I'll
> change it.

This is happening because a check for total number of errors so far is happening only after coming across at least one non-recognized configuration option. What about adding one more check right after ParseConfigFile, so we can bail out early when overwhelmed with syntax errors? This would save a line in translation :).

>
>> Do you want me to take a closer look at your modified version of the
>> patch before you commit, or did you post it more as a "FYI, this is
>> how it's going to look like"?
>
> I know I'd feel more comfortable if you (and Alexey, and Selena) gave it
> another look :-)

I have checked it here and don't see any more problems with it.

--
Command Prompt, Inc. http://www.CommandPrompt.com
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Excerpts from Alexey Kluykin's message of jue jul 14 09:18:15 -0400 2011:
>
> On Jul 14, 2011, at 4:38 AM, Alvaro Herrera wrote:
>

> >> On Jul14, 2011, at 01:38 , Alvaro Herrera wrote:

> This is happening because a check for total number of errors so far is happening only after coming across at least one non-recognized configuration option. What about adding one more check right after ParseConfigFile, so we can bail out early when overwhelmed with syntax errors? This would save a line in translation :).

Actually I think it would make sense to do it completely the other way
around: reset the number of errors to zero before starting to apply the
values. The rationale is that all the settings that made it past the
tokenisation are completely different lines for which the errors were
reported earlier.

> > I know I'd feel more comfortable if you (and Alexey, and Selena) gave it
> > another look :-)
>
> I have checked it here and don't see any more problems with it.

Thanks.

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> Excerpts from Alexey Kluykin's message of jue jul 14 09:18:15 -0400 2011:
>> This is happening because a check for total number of errors so far
>> is happening only after coming across at least one non-recognized
>> configuration option. What about adding one more check right after
>> ParseConfigFile, so we can bail out early when overwhelmed with
>> syntax errors? This would save a line in translation :).

> Actually I think it would make sense to do it completely the other way
> around: reset the number of errors to zero before starting to apply the
> values. The rationale is that all the settings that made it past the
> tokenisation are completely different lines for which the errors were
> reported earlier.

I'd like to re-open the discussion from here
http://archives.postgresql.org/pgsql-hackers/2011-06/msg01741.php
http://archives.postgresql.org/pgsql-hackers/2011-04/msg00330.php
specifically about this concern:

>>> Does that mean that some backends might currently choose to ignore an
>>> updated config file wholesale on SIGUP (because some settings are invalid)
>>> while others happily apply it? Meaning that they'll afterwards disagree
>>> even on modified settings which *would* be valid for both backends?

I complained about exactly that point back in April, but at the time
people (quite reasonably) didn't want to touch the behavior. Now that
we are entering a new development cycle, it's time to reconsider.
Particularly so if we're considering a patch that touches the behavior
already.

I think that it might be sensible to have the following behavior:

1. Parse the file, where "parse" means collect all the name = value
pairs. Bail out if we find any syntax errors at that level of detail.
(With this patch, we could report some or all of the syntax errors
first.)

2. Tentatively apply the new custom_variable_classes setting if any.

3. Check to see whether all the "name"s are valid. If not, report
the ones that aren't and bail out.

4. Apply each "value". If some of them aren't valid, report that,
but continue, and apply all the ones that are valid.

We can expect that the postmaster and all backends will agree on the
results of steps 1 through 3. They might differ as to the validity
of individual values in step 4 (as per my example of a setting that
depends on database_encoding), but we should never end up with a
situation where a globally correct value is not globally applied.

The original argument for the current behavior was to avoid applying
settings from a thoroughly munged config file, but I think that the
checks involved in steps 1-3 would be sufficient to reject files that
had major problems. It's possible that step 1 is really sufficient to
cover the issue, in which case we could drop the separate step-3 pass
and just treat invalid GUC names as a reason to ignore the particular
line rather than the whole file. That would make things simpler and
faster, and maybe less surprising too.

Comments?

regards, tom lane

I wrote:
> I think that it might be sensible to have the following behavior:

> 1. Parse the file, where "parse" means collect all the name = value
> pairs. Bail out if we find any syntax errors at that level of detail.
> (With this patch, we could report some or all of the syntax errors
> first.)

> 2. Tentatively apply the new custom_variable_classes setting if any.

> 3. Check to see whether all the "name"s are valid. If not, report
> the ones that aren't and bail out.

> 4. Apply each "value". If some of them aren't valid, report that,
> but continue, and apply all the ones that are valid.

> We can expect that the postmaster and all backends will agree on the
> results of steps 1 through 3. They might differ as to the validity
> of individual values in step 4 (as per my example of a setting that
> depends on database_encoding), but we should never end up with a
> situation where a globally correct value is not globally applied.

I thought some more about this, and it occurred to me that it's not that
hard to foresee a situation where different backends might have
different opinions about the results of step 3, ie, different ideas
about the set of valid GUC names. This could arise as a result of some
of them having a particular extension module loaded and others not.

Right now, whether or not you have say plpgsql loaded will not affect
your ability to do "SET plpgsql.junk = foobar" --- as long as "plpgsql"
is listed in custom_variable_classes, we'll accept the command and
create a placeholder variable for plpgsql.junk. But it seems perfectly
plausible that we might someday try to tighten that up so that once a
module has done EmitWarningsOnPlaceholders("plpgsql"), we'll no longer
allow creation of new placeholders named plpgsql.something. If we did
that, we could no longer assume that all backends agree on the set of
legal GUC variable names.

So that seems like an argument --- not terribly strong, but still an
argument --- for doing what I suggested next:

> The original argument for the current behavior was to avoid applying
> settings from a thoroughly munged config file, but I think that the
> checks involved in steps 1-3 would be sufficient to reject files that
> had major problems. It's possible that step 1 is really sufficient to
> cover the issue, in which case we could drop the separate step-3 pass
> and just treat invalid GUC names as a reason to ignore the particular
> line rather than the whole file. That would make things simpler and
> faster, and maybe less surprising too.

IOW, I'm now pretty well convinced that so long as the configuration
file is syntactically valid, we should go ahead and attempt to apply
each name = value setting individually, without allowing the invalidity
of any one name or value to prevent others from being applied.

regards, tom lane

On Jul16, 2011, at 20:55 , Tom Lane wrote:
>> The original argument for the current behavior was to avoid applying
>> settings from a thoroughly munged config file, but I think that the
>> checks involved in steps 1-3 would be sufficient to reject files that
>> had major problems. It's possible that step 1 is really sufficient to
>> cover the issue, in which case we could drop the separate step-3 pass
>> and just treat invalid GUC names as a reason to ignore the particular
>> line rather than the whole file. That would make things simpler and
>> faster, and maybe less surprising too.
>
> IOW, I'm now pretty well convinced that so long as the configuration
> file is syntactically valid, we should go ahead and attempt to apply
> each name = value setting individually, without allowing the invalidity
> of any one name or value to prevent others from being applied.

One benefit of this would be that it'd make the logic of ProcessConfigFile
and its interaction with set_config_option() much simpler, and the behaviour
more predictable. Given that it took me a while to work through the
interactions of these two functions, I all for that.

On the downside, the current behaviour prevents problems if someone changes
two interrelated GUCs, but makes a mistake at one of them. For example,
someone might drastically lower bgwriter_delay but might botch the matching
adjustment of bgwriter_lru_maxpages.

Not sure if that out-weights the benefits, but I thought I'd bring it up.

best regards,
Florian Pflug

Florian Pflug <fgp(at)phlo(dot)org> writes:
> On the downside, the current behaviour prevents problems if someone changes
> two interrelated GUCs, but makes a mistake at one of them. For example,
> someone might drastically lower bgwriter_delay but might botch the matching
> adjustment of bgwriter_lru_maxpages.

That's a fair point, but the current behavior only saves you if the
botch is such that the new value is detectably invalid, as opposed to
say just a factor of 100 off from what you meant. Not sure that that's
all that helpful.

regards, tom lane

On Jul16, 2011, at 21:23 , Tom Lane wrote:

> Florian Pflug <fgp(at)phlo(dot)org> writes:
>> On the downside, the current behaviour prevents problems if someone changes
>> two interrelated GUCs, but makes a mistake at one of them. For example,
>> someone might drastically lower bgwriter_delay but might botch the matching
>> adjustment of bgwriter_lru_maxpages.
>
> That's a fair point, but the current behavior only saves you if the
> botch is such that the new value is detectably invalid, as opposed to
> say just a factor of 100 off from what you meant. Not sure that that's
> all that helpful.

True. And a forgotten zero or wrong unit probably is even more likely than
a totally botched value. So +1 from me.

Btw, if we touch that, I think we should think about providing some way
to detect when a backend fails to apply a value. Showing the precise
option that caused the trouble is probably hard, but could we add a flag to
PGPROC that gets set once a backend fails to apply some setting on SIGUP?
If we showed the state of such a flag in pg_stat_activity, it'd give an
admin a quick way of verifying that all is well after a SIGUP. We might also
want to record the timestamp of the last processed file so that backends
which haven't yet processed the SIHUP can also be detected.

best regards,
Florian Pflug

Florian Pflug <fgp(at)phlo(dot)org> writes:
> Btw, if we touch that, I think we should think about providing some way
> to detect when a backend fails to apply a value.

Hm, maybe, but keep in mind that there are valid reasons for a backend
to ignore a postgresql.conf setting --- in particular, it might have a
local override from some flavor of SET command. So I don't think we'd
want the flag to have the semantics of "this backend is actually *using*
the value"; and yet, if that's not what it means, people could still be
confused. There might be some implementation gotchas as well. I'm not
sure offhand how thoroughly the GUC code checks a value that is being
overridden.

regards, tom lane

On Jul16, 2011, at 22:55 , Tom Lane wrote:
> Florian Pflug <fgp(at)phlo(dot)org> writes:
>> Btw, if we touch that, I think we should think about providing some way
>> to detect when a backend fails to apply a value.
>
> Hm, maybe, but keep in mind that there are valid reasons for a backend
> to ignore a postgresql.conf setting --- in particular, it might have a
> local override from some flavor of SET command. So I don't think we'd
> want the flag to have the semantics of "this backend is actually *using*
> the value";

Yeah, the flag would simply indicate whether a particular backend
encountered an error during config file reload or not.

Actually being able to inspect other backend's GUCs would be nice, but
is way beyond the scope of this of course.

> and yet, if that's not what it means, people could still be
> confused.

Hm, if it's called "cfgfile_valid" or a prettier version thereof I
think the risk is small.

> There might be some implementation gotchas as well. I'm not
> sure offhand how thoroughly the GUC code checks a value that is being
> overridden.

If it doesn't, then what happens when the overriding scope ends, and
the value reverts (or attempts to revert) to its default?

best regards,
Florian Pflug

On Fri, Jul 15, 2011 at 8:02 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> 2. Tentatively apply the new custom_variable_classes setting if any.

Is there any way that we could get *rid* of custom_variable_classes?
The idea of using a GUC to define the set of valid GUCs seems
intrinsically problematic.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Fri, Jul 15, 2011 at 8:02 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> 2. Tentatively apply the new custom_variable_classes setting if any.

> Is there any way that we could get *rid* of custom_variable_classes?
> The idea of using a GUC to define the set of valid GUCs seems
> intrinsically problematic.

Well, we could just drop it and say you can set any dotted-name GUC
you feel like. The only reason to have it is to have some modicum of
error checking ... but I'm not sure why we should bother if there's no
checking on the second half of the name. Not sure if that's going too
far in the laissez-faire direction, though. I can definitely imagine
people complaining "I set plpqsql.variable_conflict in postgresql.conf
and it didn't do anything, how come?"

regards, tom lane

On Sat, Jul 16, 2011 at 10:04 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Fri, Jul 15, 2011 at 8:02 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> 2. Tentatively apply the new custom_variable_classes setting if any.
>
>> Is there any way that we could get *rid* of custom_variable_classes?
>> The idea of using a GUC to define the set of valid GUCs seems
>> intrinsically problematic.
>
> Well, we could just drop it and say you can set any dotted-name GUC
> you feel like.  The only reason to have it is to have some modicum of
> error checking ... but I'm not sure why we should bother if there's no
> checking on the second half of the name.  Not sure if that's going too
> far in the laissez-faire direction, though.  I can definitely imagine
> people complaining "I set plpqsql.variable_conflict in postgresql.conf
> and it didn't do anything, how come?"

I'm not sure that's really making anything any better. Maybe I'm
misunderstanding, but if that's going to be a problem, then presumably
this will create the same problem:

custom_variable_classes='plpgsql'
plpgsql.variable_conflict='on'

...and the fact that we've made them set an extra GUC to shoot
themselves in the foot hardly seems like an improvement. I was more
thinking along the lines of having loadable modules register custom
variable classes at load time, through some sort of C API provided for
that purpose, rather than having the user declare a list that may or
may not match what the .so files really care about.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Sat, Jul 16, 2011 at 10:04 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>>> Is there any way that we could get *rid* of custom_variable_classes?

>> Well, we could just drop it and say you can set any dotted-name GUC
>> you feel like.

> ...and the fact that we've made them set an extra GUC to shoot
> themselves in the foot hardly seems like an improvement. I was more
> thinking along the lines of having loadable modules register custom
> variable classes at load time, through some sort of C API provided for
> that purpose, rather than having the user declare a list that may or
> may not match what the .so files really care about.

Well, we *do* have a C API for that, of a sort. The problem is, what do
you do in processes that have not loaded the relevant extension? (And
no, I don't like the answer of "let's force the postmaster to load every
extension we want to set any parameters for".)

I agree custom_variable_classes is conceptually messy, but it's a
reasonably lightweight compromise that gives some error checking without
requiring a lot of possibly-irrelevant extensions to be loaded into
every postgres process.

regards, tom lane

On Sun, Jul 17, 2011 at 12:59 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Sat, Jul 16, 2011 at 10:04 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>>>> Is there any way that we could get *rid* of custom_variable_classes?
>
>>> Well, we could just drop it and say you can set any dotted-name GUC
>>> you feel like.
>
>> ...and the fact that we've made them set an extra GUC to shoot
>> themselves in the foot hardly seems like an improvement.  I was more
>> thinking along the lines of having loadable modules register custom
>> variable classes at load time, through some sort of C API provided for
>> that purpose, rather than having the user declare a list that may or
>> may not match what the .so files really care about.
>
> Well, we *do* have a C API for that, of a sort.  The problem is, what do
> you do in processes that have not loaded the relevant extension?  (And
> no, I don't like the answer of "let's force the postmaster to load every
> extension we want to set any parameters for".)
>
> I agree custom_variable_classes is conceptually messy, but it's a
> reasonably lightweight compromise that gives some error checking without
> requiring a lot of possibly-irrelevant extensions to be loaded into
> every postgres process.

Hmm. Maybe what we need is a mechanism that allows the configuration
to be associated a loadable module, and whenever that module is
loaded, we also load the associated configuration settings. This is
probably terribly syntax, but something like:

ALTER LOAD 'plpgsql' SET plpgsql.variable_conflict = 'whatever';

AFAICS, that would remove the need to set variables in postgresql.conf
that can't be validated, and so we could just disallow it.

OTOH, maybe that's more trouble than can be justified by the size of
the problem.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Jul18, 2011, at 01:29 , Robert Haas wrote:
> Hmm. Maybe what we need is a mechanism that allows the configuration
> to be associated a loadable module, and whenever that module is
> loaded, we also load the associated configuration settings. This is
> probably terribly syntax, but something like:
>
> ALTER LOAD 'plpgsql' SET plpgsql.variable_conflict = 'whatever';

A variant of this would be to allow extensions (in the CREATE EXTENSION
sense) to declare custom GUCs in their control file. Then we'd only
need to load those files, which seems better than loading a shared
library.

We do expect people to wrap their loadable modules in extensions
anyway nowadays, do we?

best regards,
Florian Pflug

2011/7/18 Florian Pflug <fgp(at)phlo(dot)org>:
> On Jul18, 2011, at 01:29 , Robert Haas wrote:
>> Hmm.  Maybe what we need is a mechanism that allows the configuration
>> to be associated a loadable module, and whenever that module is
>> loaded, we also load the associated configuration settings.  This is
>> probably terribly syntax, but something like:
>>
>> ALTER LOAD 'plpgsql' SET plpgsql.variable_conflict = 'whatever';
>
> A variant of this would be to allow extensions (in the CREATE EXTENSION
> sense) to declare custom GUCs in their control file. Then we'd only
> need to load those files, which seems better than loading a shared
> library.

+1

Pavel

>
> We do expect people to wrap their loadable modules in extensions
> anyway nowadays, do we?
>
> best regards,
> Florian Pflug
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Sun, Jul 17, 2011 at 12:59 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> I agree custom_variable_classes is conceptually messy, but it's a
>> reasonably lightweight compromise that gives some error checking without
>> requiring a lot of possibly-irrelevant extensions to be loaded into
>> every postgres process.

> Hmm. Maybe what we need is a mechanism that allows the configuration
> to be associated a loadable module, and whenever that module is
> loaded, we also load the associated configuration settings. This is
> probably terribly syntax, but something like:

> ALTER LOAD 'plpgsql' SET plpgsql.variable_conflict = 'whatever';

> AFAICS, that would remove the need to set variables in postgresql.conf
> that can't be validated, and so we could just disallow it.

No, that only fixes things for the case of setting a variable in the
control file. It isn't useful for ALTER ROLE/DATABASE SET. And it
still has the problem of forcing every process to load every extension,
and as written it would also require the postmaster to read catalogs.

> OTOH, maybe that's more trouble than can be justified by the size of
> the problem.

Yeah.

regards, tom lane

Tom, Florian,

>> On the downside, the current behaviour prevents problems if someone changes
>> two interrelated GUCs, but makes a mistake at one of them. For example,
>> someone might drastically lower bgwriter_delay but might botch the matching
>> adjustment of bgwriter_lru_maxpages.
>
> That's a fair point, but the current behavior only saves you if the
> botch is such that the new value is detectably invalid, as opposed to
> say just a factor of 100 off from what you meant. Not sure that that's
> all that helpful.

Hmmm. As someone who often deploys pg.conf changes as part of a
production code rollout, I actually like the "atomic" nature of updating
postgresql.conf -- that is, all your changes succeed, or they all fail.

If we add this feature, I'd want there to be an option which allows
getting the current all-or-none behavior.

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

On sön, 2011-07-17 at 00:59 -0400, Tom Lane wrote:
> Well, we *do* have a C API for that, of a sort. The problem is, what
> do you do in processes that have not loaded the relevant extension?

Those processes that have the extension loaded check the parameter
settings in their namespace, those that don't ignore them.

Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> On sn, 2011-07-17 at 00:59 -0400, Tom Lane wrote:
>> Well, we *do* have a C API for that, of a sort. The problem is, what
>> do you do in processes that have not loaded the relevant extension?

> Those processes that have the extension loaded check the parameter
> settings in their namespace, those that don't ignore them.

Then you don't have any meaningful reporting of whether you have entered
valid values --- particularly not with the policy that only the
postmaster makes logfile entries about bad values. It'd work but I
don't think it's tremendously user-friendly.

regards, tom lane

Josh Berkus <josh(at)agliodbs(dot)com> writes:
> Hmmm. As someone who often deploys pg.conf changes as part of a
> production code rollout, I actually like the "atomic" nature of updating
> postgresql.conf -- that is, all your changes succeed, or they all fail.

If we actually *had* that, I'd agree with you. The problem is that it
appears that we have such a behavior, but it fails to work that way in
corner cases. My proposal is aimed at making the corner cases less
corner-y, by adopting a uniform rule that each backend adopts all the
changes it can.

regards, tom lane

Florian Pflug <fgp(at)phlo(dot)org> writes:
> A variant of this would be to allow extensions (in the CREATE EXTENSION
> sense) to declare custom GUCs in their control file. Then we'd only
> need to load those files, which seems better than loading a shared
> library.

The original patch for the extensions had that feature. It had to be
removed, though. The control file value has to be stored in the
catalogs and now only backends can look that up once connected to a
database. This part worked well.

http://git.postgresql.org/gitweb/?p=postgresql-extension.git;a=commit;h=480fa10f4ec7b495cf4f434e6f44a50b94487326
http://git.postgresql.org/gitweb/?p=postgresql-extension.git;a=commit;h=98d802aa1ee12c77c5270c7bc9ed7479360f35b9

IIRC the problem is that now, the postmaster is not seeing cvc at all,
and you can have there some custom parameters to set shared memory
segments, which only postmaster will take care of. An example of that
is the pg_stat_statements contrib.

I would love that we find a way around that, btw.

Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support

On Jul 16, 2011, at 9:55 PM, Tom Lane wrote:

> I wrote:
>> I think that it might be sensible to have the following behavior:
>
>> 1. Parse the file, where "parse" means collect all the name = value
>> pairs. Bail out if we find any syntax errors at that level of detail.
>> (With this patch, we could report some or all of the syntax errors
>> first.)
>
>> 2. Tentatively apply the new custom_variable_classes setting if any.
>
>> 3. Check to see whether all the "name"s are valid. If not, report
>> the ones that aren't and bail out.
>
>> 4. Apply each "value". If some of them aren't valid, report that,
>> but continue, and apply all the ones that are valid.
>
>> We can expect that the postmaster and all backends will agree on the
>> results of steps 1 through 3. They might differ as to the validity
>> of individual values in step 4 (as per my example of a setting that
>> depends on database_encoding), but we should never end up with a
>> situation where a globally correct value is not globally applied.
>

Attached is my first attempt to implement your plan. Basically, I've
reshuffled pieces of the ProcessConfigFile on top of my previous patch,
dropped verification calls of set_config_option and moved the check for
custom_variable_class existence right inside the loop that assigns new values
to GUC variables.

I'd think that removal of custom_variable_classes or setting it from the
extensions could be a separate patch.

I appreciate your comments and suggestions.

> I thought some more about this, and it occurred to me that it's not that
> hard to foresee a situation where different backends might have
> different opinions about the results of step 3, ie, different ideas
> about the set of valid GUC names. This could arise as a result of some
> of them having a particular extension module loaded and others not.
>
> Right now, whether or not you have say plpgsql loaded will not affect
> your ability to do "SET plpgsql.junk = foobar" --- as long as "plpgsql"
> is listed in custom_variable_classes, we'll accept the command and
> create a placeholder variable for plpgsql.junk. But it seems perfectly
> plausible that we might someday try to tighten that up so that once a
> module has done EmitWarningsOnPlaceholders("plpgsql"), we'll no longer
> allow creation of new placeholders named plpgsql.something. If we did
> that, we could no longer assume that all backends agree on the set of
> legal GUC variable names.
>
> So that seems like an argument --- not terribly strong, but still an
> argument --- for doing what I suggested next:
>
>> The original argument for the current behavior was to avoid applying
>> settings from a thoroughly munged config file, but I think that the
>> checks involved in steps 1-3 would be sufficient to reject files that
>> had major problems. It's possible that step 1 is really sufficient to
>> cover the issue, in which case we could drop the separate step-3 pass
>> and just treat invalid GUC names as a reason to ignore the particular
>> line rather than the whole file. That would make things simpler and
>> faster, and maybe less surprising too.
>
> IOW, I'm now pretty well convinced that so long as the configuration
> file is syntactically valid, we should go ahead and attempt to apply
> each name = value setting individually, without allowing the invalidity
> of any one name or value to prevent others from being applied.

--
Command Prompt, Inc. http://www.CommandPrompt.com
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Hi Alexey, I was taking a quick look at this patch, and have a question for ya.

I have a default config from initdb, there is a new setting at the end but its commented out.

root(at)storm: /db/pg92
# /etc/rc.d/postgresql start
Starting PostgreSQL:

root(at)storm: /db/pg92
# more serverlog
LOG: database system was shut down at 2011-09-06 22:30:17 CDT
LOG: database system is ready to accept connections
LOG: autovacuum launcher started

root(at)storm: /db/pg92
# /etc/rc.d/postgresql reload
Reload PostgreSQL: No directory, logging in with HOME=/

root(at)storm: /db/pg92
# more serverlog
LOG: database system was shut down at 2011-09-06 22:30:17 CDT
LOG: database system is ready to accept connections
LOG: autovacuum launcher started
LOG: received SIGHUP, reloading configuration files
WARNING: errors detected while parsing configuration files
WARNING: errors detected while parsing configuration files
WARNING: errors detected while parsing configuration files
WARNING: errors detected while parsing configuration files
WARNING: errors detected while parsing configuration files

I didn't edit the config, it was fine at startup, so why does reload upset it so? Also, what line is the warning for?

If I edit postgresql.conf and just add bob at the last line, then reload:

LOG: received SIGHUP, reloading configuration files
LOG: syntax error in file "/db/pg92/postgresql.conf" line 570, near end of line
FATAL: errors detected while parsing configuration files

Where did the other warnings go? Its right though, line 570 is bad. It also seems to have killed the server. I have not gotten through the history of messages regarding this patch, but is it supposed to kill the server if there is a syntax error in the config file?

-Andy

Andy Colson <andy(at)squeakycode(dot)net> writes:
> Where did the other warnings go? Its right though, line 570 is bad. It also seems to have killed the server. I have not gotten through the history of messages regarding this patch, but is it supposed to kill the server if there is a syntax error in the config file?

The historical behavior is that a configuration file error detected
during postmaster startup should prevent the server from starting, but
an error detected during reload should only result in a LOG message and
the reload not occurring. I don't believe anyone will accept a patch
that causes the server to quit on a failed reload. There has however
been some debate about the exact extent of ignoring bad values during
reload --- currently the theory is "ignore the whole file if anything is
wrong", but there's some support for applying all non-bad values as long
as the overall file syntax is okay.

regards, tom lane

Hello,

On Sep 7, 2011, at 5:00 PM, Tom Lane wrote:

> Andy Colson <andy(at)squeakycode(dot)net> writes:
>> Where did the other warnings go? Its right though, line 570 is bad. It also seems to have killed the server. I have not gotten through the history of messages regarding this patch, but is it supposed to kill the server if there is a syntax error in the config file?
>
> The historical behavior is that a configuration file error detected
> during postmaster startup should prevent the server from starting, but
> an error detected during reload should only result in a LOG message and
> the reload not occurring. I don't believe anyone will accept a patch
> that causes the server to quit on a failed reload. There has however
> been some debate about the exact extent of ignoring bad values during
> reload --- currently the theory is "ignore the whole file if anything is
> wrong", but there's some support for applying all non-bad values as long
> as the overall file syntax is okay.

This patch actually aims to do the latter, applying all good values and reporting the bad ones. It removes the calls to set_config_option with changeVal = false from ProcessConfigFile, trying to apply every option at once and incrementing the warnings counter if set_config_option returns false. After some investigation I've discovered that set_config_option returns false even if a variable value is unchanged, but is applied in the wrong GucContext. The particular case is trying to reload the configuration file variables during SIGHUP: if, say, shared_buffers are commented out, the call to set_config_option with changeVal = true will return false due to this code:

> if (prohibitValueChange)
> {
> if (*conf->variable != newval)
> ereport(elevel,
> (errcode(ERRCODE_CANT_CHANGE_RUNTIME_PARAM),
> errmsg("parameter \"%s\" cannot be changed without restarting the server",
> name)));
> return false;
> }
>

Due to the above, set_config_option returns false for unchanged PGC_POSTMASTER variables during SIGHUP, so it's impossible to distinguish between valid and non valid values and report the latter ones with a single call of this function per var. What do you think about changing the code above to return true if the variable is actually unchanged?

This explains the WARNINGs emitted during reload even for a pristine configuration file right after the installation. I'm looking into why the server gets killed during reload if there is a parse error, it might be as well related to the problem above.

--
Alexey Klyukin http://www.commandprompt.com
The PostgreSQL Company – Command Prompt, Inc.

Hi Andy,

On Sep 7, 2011, at 6:40 AM, Andy Colson wrote:

> Hi Alexey, I was taking a quick look at this patch, and have a question for ya.
>
...

> Where did the other warnings go? Its right though, line 570 is bad. It also seems to have killed the server. I have not gotten through the history of messages regarding this patch, but is it supposed to kill the server if there is a syntax error in the config file?

Thank you for looking at this patch. v4 was more a "what if" concept that took a lot of time for somebody to look at. There were a lot of problems with it, but I think I've nailed down most of them.

Attached is v5. It should fix both problems you've experienced with v4. As with the current code, the startup process gets interrupted on any error detected in the configuration file. Unlike the current code, the patch tries to report all of them before bailing out. The behavior during configuration reload has changed significantly. Instead of ignoring all changes after the first error, the code reports the problematic value and continues. It only skips applying new values completely on syntax errors and invalid configuration option names. In no cases should it bring the server down during reload.

One problem I'm not sure how to address is the fact that we require 2 calls of set_config_option for each option, one to verify the new value and another to actually apply it. Normally, this function returns true for a valid value and false if it is unable to set the new value for whatever reason (either the value is invalid, or the value cannot be changed in the context of the caller). However, calling it once per value in the 'apply' mode during reload produces false for every unchanged value that can only be changed during startup (i.e. shared_buffers, or max_connections). If we ignore its return value, we'll lose the ability to detect whether invalid values were encountered during the reload and report this fact at the end of the function. I think the function should be changed, as described in my previous email (http://archives.postgresql.org/message-id/97A66029-9D3E-43AF-95AA-46FE1B447447@commandprompt.com) and I'd like to hear other opinions on that. Meanwhile, due to 2 calls to set_config_option, it currently reports all invalid values twice. If others will be opposed to changing the set_config_option, I'll fix this by removing the first, verification call and final 'errors were detected' warning to avoid 'false positives' on that (i.e. the WARNING you saw with the previous version for the valid .conf).

I'd appreciate your further comments and suggestions.

Thank you.

--
Alexey Klyukin http://www.commandprompt.com
The PostgreSQL Company – Command Prompt, Inc.

On 09/10/2011 11:39 AM, Alexey Klyukin wrote:
> Hi Andy,
>
> On Sep 7, 2011, at 6:40 AM, Andy Colson wrote:
>
>> Hi Alexey, I was taking a quick look at this patch, and have a question for ya.
>>
> ...
>
>> Where did the other warnings go? Its right though, line 570 is bad. It also seems to have killed the server. I have not gotten through the history of messages regarding this patch, but is it supposed to kill the server if there is a syntax error in the config file?
>
>
> Thank you for looking at this patch. v4 was more a "what if" concept that took a lot of time for somebody to look at. There were a lot of problems with it, but I think I've nailed down most of them.
>
> Attached is v5. It should fix both problems you've experienced with v4. As with the current code, the startup process gets interrupted on any error detected in the configuration file. Unlike the current code, the patch tries to report all of them before bailing out. The behavior during configuration reload has changed significantly. Instead of ignoring all changes after the first error, the code reports the problematic value and continues. It only skips applying new values completely on syntax errors and invalid configuration option names. In no cases should it bring the server down during reload.
>
> One problem I'm not sure how to address is the fact that we require 2 calls of set_config_option for each option, one to verify the new value and another to actually apply it. Normally, this function returns true for a valid value and false if it is unable to set the new value for whatever reason (either the value is invalid, or the value cannot be changed in the context of the caller). However, calling it once per value in the 'apply' mode during reload produces false for every unchanged value that can only be changed during startup (i.e. shared_buffers, or max_connections). If we ignore its return value, we'll lose the ability to detect whether invalid values were encountered during the reload and report this fact at the end of the function. I think the function should be changed, as described in my previous email (http://archives.postgresql.org/message-id/97A66029-9D3E-43AF-95AA-46FE1B447447@commandprompt.com) and I'd like to hear other opinions on that. Meanwhile, due t
o 2 calls to set_config_option, it currently reports all invalid values twice. If others will be opposed to changing the set_config_option, I'll fix this by removing the first, verification call and final 'errors were detected' warning to avoid 'false positives' on that (i.e. the WARNING you saw with the previous version for the valid .conf).
>
> I'd appreciate your further comments and suggestions.
>
> Thank you.
>
> --
> Alexey Klyukin http://www.commandprompt.com
> The PostgreSQL Company – Command Prompt, Inc.
>

After a quick two minute test, this patch seems to work well. It does just what I think it should. I'll add it to the commitfest page for ya.

-Andy

On ons, 2011-09-07 at 10:00 -0400, Tom Lane wrote:
> There has however
> been some debate about the exact extent of ignoring bad values during
> reload --- currently the theory is "ignore the whole file if anything is
> wrong", but there's some support for applying all non-bad values as long
> as the overall file syntax is okay.

That could be a problem if you have some values that depend on each
other, and then you change both of them, but because of an error only
one gets applied. I think ACID(-like) changes is a feature, also on
this level.

On Sep 12, 2011, at 10:24 PM, Peter Eisentraut wrote:

> On ons, 2011-09-07 at 10:00 -0400, Tom Lane wrote:
>> There has however
>> been some debate about the exact extent of ignoring bad values during
>> reload --- currently the theory is "ignore the whole file if anything is
>> wrong", but there's some support for applying all non-bad values as long
>> as the overall file syntax is okay.
>
> That could be a problem if you have some values that depend on each
> other, and then you change both of them, but because of an error only
> one gets applied. I think ACID(-like) changes is a feature, also on
> this level.
>

I think exactly this argument has already been discussed earlier in this thread:
http://archives.postgresql.org/message-id/21310D95-EB8D-4B15-A8BC-0F05505C6A34@phlo.org

--
Alexey Klyukin http://www.commandprompt.com
The PostgreSQL Company – Command Prompt, Inc.

Alexey Klyukin <alexk(at)commandprompt(dot)com> writes:
> Attached is v5. It should fix both problems you've experienced with v4.

I've applied this patch after some additional hacking.

> One problem I'm not sure how to address is the fact that we require 2
> calls of set_config_option for each option, one to verify the new
> value and another to actually apply it. Normally, this function
> returns true for a valid value and false if it is unable to set the
> new value for whatever reason (either the value is invalid, or the
> value cannot be changed in the context of the caller). However,
> calling it once per value in the 'apply' mode during reload produces
> false for every unchanged value that can only be changed during
> startup (i.e. shared_buffers, or max_connections).

I thought that the most reasonable solution here was to extend
set_config_option to provide a three-valued result: applied, error,
or not-applied-for-some-non-error-reason. So I did that, and then
ripped out the first set of calls to set_config_option. That should
make things a bit faster, as well as eliminating a set of corner cases
where the "checking" call succeeds but then the real apply step fails
anyway.

I also got rid of the changes to ParseConfigFile's API. I thought those
were messy and unnecessary, because there's no good reason not to define
the parse-error limit as being so many errors per file. It's really
only there to prevent the "config file contains War and Peace" syndrome
anyway.

regards, tom lane