Re: Should we get rid of custom_variable_classes altogether?

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:
> Simon Riggs <simon(at)2ndQuadrant(dot)com> writes:
>> On Sun, Oct 2, 2011 at 10:05 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> So at this point I'd vote for just dropping it and always allowing
>>> custom (that is, qualified) GUC names to be set, whether the prefix
>>> corresponds to any loaded module or not.
>
>> Sounds sensible. One less thing to configure is a good thing.
>
> Attached is a draft patch for that.

I fiddled with custom_variable_classes for the extension's patch, the
idea was to be able to append to it from the control file. Removing it
entirely makes it even simpler.

I think we should load any qualified entry in the control file as a
custom GUC, or allow a new extension.conf file to be given containing
the default values.

> While working on this I got annoyed at our cheesy handling of the
> situation where a "placeholder" value has to be turned into a real
> setting, which happens when the corresponding extension gets loaded.
> There are several issues:
>
> 1. If it's a SUSET variable, a preceding attempt to set the value via
> SET will fail even if you're a superuser, for example
>
> regression=# set plpgsql.variable_conflict = use_variable;
> SET
> regression=# load 'plpgsql';
> ERROR: permission denied to set parameter "plpgsql.variable_conflict"
>
> The reason for that is that define_custom_variable doesn't know whether
> the pre-existing value was set by a superuser, so it must assume the
> worst. Seems like we could easily fix that by having set_config_option
> set a flag in the GUC variable noting whether a SET was done by a
> superuser or not.

I managed to do that by having another specific GUC array so that I
could call the GUC validation code (assign hooks) at module loading
time. I guess a new flag would provide same capabilities.

> 2. If you do get an error while re-assigning the pre-existing value of
> the variable, it's thrown as an ERROR. This is really pretty nasty
> because it'll abort execution of the extension module's init function;
> for example, a likely consequence is that other custom variables of
> the module don't set set up correctly, and it could easily be a lot
> worse if there are other things the init function hasn't done yet.
>
> I think we need to arrange that set_config_option only reports failures
> to apply such values as WARNING, not ERROR. There isn't anything in its
> present API that could be used for that, but perhaps we could add a new
> enum variant for "action" that commands it.

I think this behavior only makes sense when we had a previous default
value before loading the module (set in the main postgresql.conf file),
and that we should still ERROR out otherwise (default provided by the
extension's code itself). Or maybe I'm confused now.

> 3. We aren't very careful about preserving the reset value of the
> variable, in case it's different from the active value (which could
> happen if the user did a SET and there's also a value from the
> postgresql.conf file).
>
> This seems like it just requires working a bit harder in
> define_custom_variable, to reapply the reset value as well as the
> current value of the variable.
>
> Any objections to fixing that stuff, while I'm looking at it?

Please do :)

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