Re: postgresql.conf error checking strategy

On Wed, Apr 6, 2011 at 5:17 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I just spent a rather confused half hour while testing my GUC
> assign-hook patch, and when I finally figured out what was happening,
> it made me wonder whether we should redesign the behavior a little bit.
>
> The current behavior of ProcessConfigFile is that it runs through all
> the "name = value" pairs extracted from the file and tries to fully
> verify each value (by seeing whether set_config_option with changeVal
> false likes it).  Only if every one of them checks out does it actually
> apply any of the settings.  Now this is nice and conservative --- the
> aim is to avoid applying settings from a possibly corrupted file, in
> case somebody fat-fingered their edits in a big way.  But there's a
> little problem:
>
> 1. It's possible that not all the backends agree on whether a setting
> is valid.  The case I was testing involved setting client_encoding
> from the config file, so whether it succeeds depends on the database
> encoding (some conversions might exist and others not).  This means
> that some backends might apply the postgresql.conf settings and others
> not.  That's pretty bad in itself, if something that needs to be
> consistent system-wide is changing.
>
> 2. Only the postmaster reports config file problems at elevel LOG;
> backends only complain at DEBUG3, to avoid cluttering the log with
> lots of duplicate messages.  This means that if you do have a few
> backends that fail to adopt a setting, there likely won't be anything
> in the log to tell you so.  (The reason I was so confused is that I'd
> raised log_min_messages to DEBUG5 to try to understand what was
> happening ... but my backend-under-test wasn't adopting that setting,
> and wasn't logging anything to tell me so either ...)
>
> So I'm thinking we should adopt a strategy that's less likely to result
> in divergent behavior among different backends.  The idea I have in mind
> is to have the first "validation" pass only check that each name is a
> legal GUC variable name, and not look at the values at all.  If so, try
> to apply all the values.  Any that fail to apply we log as usual, but
> still apply the others.  ISTM that verifying the names should be enough
> protection against broken files for practical purposes, and it should be
> something that all backends will agree on even if there are individual
> values that are not valid for all.
>
> Comments?

I don't think now is a good time for a major behavior change in this
area, and I'm not convinced this is the best possible design.

There are a number of parameters which are currently PGC_POSTMASTER
rather than PGC_SIGHUP precisely because of the possibility of
backends being out of step with each other.  wal_level is an obvious
example, and one that it would be *really* nice to be able to change
without a server restart.  It would be nice to have a real solution to
that problem, but this isn't it, and I don't want to engineer it right
now.

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