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
pg_parser_continue_on_error_v4.patch
Description: Binary data
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
