Thread

Re: proposal: a validator for configuration files

Alexey Klyukin <alexk@commandprompt.com> — 2011-07-25T18:55:09Z

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