Re: plpgsql GUC variable: custom or built-in?

So I think we're agreed on adding a variable_conflict GUC for plpgsql.
The straight-and-narrow way to do it would be to make this a custom
GUC that's defined only when plpgsql is dynamically loaded into the
backend. However that implies a lot of notational overhead, notably
having to put plpgsql into custom_variable_classes if you want to set
the variable in postgresql.conf. Maybe that's okay, particularly if
you are of the opinion that most people will leave it at default.
But it could also be argued that plpgsql is so widely used that we
should bend the rules for it, and make this a built-in GUC that just
happens to only be consulted by plpgsql.

I'm leaning to the custom GUC approach because it seems a little
cleaner from a code point of view, but I wanted to see if anyone
wishes to argue for the other way.

One reason to argue for the other way is that maybe it wouldn't only
be consulted by plpgsql. In particular I can easily imagine SQL
functions having the same issue as soon as someone gets around to
letting them use names for their parameters.

regards, tom lane

On Thu, Nov 12, 2009 at 11:13 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> One reason to argue for the other way is that maybe it wouldn't only
> be consulted by plpgsql.  In particular I can easily imagine SQL
> functions having the same issue as soon as someone gets around to
> letting them use names for their parameters.

I don't have a strong feeling on the core issue but I don't agree with
this point. AIUI, we are implementing multiple behaviors here for
reasons of backward and competing-product compatibility. Presumably,
if we're starting from scratch, we'll pick a sensible behavior -
probably error in the case of SQL - and stick with it.

...Robert

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Thu, Nov 12, 2009 at 11:13 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> One reason to argue for the other way is that maybe it wouldn't only
>> be consulted by plpgsql. In particular I can easily imagine SQL
>> functions having the same issue as soon as someone gets around to
>> letting them use names for their parameters.

> I don't have a strong feeling on the core issue but I don't agree with
> this point. AIUI, we are implementing multiple behaviors here for
> reasons of backward and competing-product compatibility. Presumably,
> if we're starting from scratch, we'll pick a sensible behavior -
> probably error in the case of SQL - and stick with it.

Fair enough. I'll start writing the custom GUC then.

regards, tom lane

I wrote:
> Fair enough. I'll start writing the custom GUC then.

While testing that, I noticed a pre-existing GUC limitation that maybe
we should do something about now: it does not work very nicely for
custom SUSET GUC variables. Normally, if a variable is SUSET, an
ordinary user can't do ALTER USER SET or ALTER DATABASE SET to modify
it. However, a superuser can do that on his behalf, and then the
variable will default in the desired way for that user or database.
But this logic doesn't work for custom variables: the system doesn't
record whether the option was set by a superuser or not, so it's
afraid to allow the value to be applied when the defining module
gets loaded. (Instead you get a WARNING and the value reverts to
default.)

I think we discussed this once before and came up with the idea that
it wouldn't be a problem if ALTER USER/DATABASE SET disallowed setting
of variables not currently known to the system. Right now, if
plpgsql is in custom_variable_classes, you can do
ALTER USER foouser SET plpgsql.variable_conflict = variable_first;
and the system will take it even if plpgsql isn't loaded. If we
required plpgsql to be loaded then we could be sure that the appropriate
permissions check had been made when the ALTER was done, and then in
subsequent sessions it would be safe to apply the variable value while
loading plpgsql.

One possible objection is that a loadable module couldn't safely upgrade
a USERSET variable to SUSET (except across a major version boundary),
since the permissions check would already have been made implicitly for
any ALTER settings. This doesn't seem like a big problem compared to
the current situation of not being able to use SUSET effectively at all,
though.

Another issue is that pg_dumpall output would fail to reload with such a
restriction, since the dump script would most likely be executed in a
session that hadn't loaded the relevant loadable module. We could get
around that by still allowing superusers to issue ALTER SET for unknown
variables, but that seems a tad weird. OTOH the current rule (must be
in custom_variable_classes) is pretty hazardous for dump reloading, too.

Comments?

regards, tom lane

Tom Lane escribió:
> I wrote:
> > Fair enough. I'll start writing the custom GUC then.
>
> While testing that, I noticed [...]

With all this fallout, I think it would be cleaner to step back and make
it a regular GUC variable, not custom. Pretending that plpgsql is
somehow not an integral part of the system is not completely true
anyway. Yes, we're playing favorites in the PL camp here, but so what?

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> With all this fallout, I think it would be cleaner to step back and make
> it a regular GUC variable, not custom. Pretending that plpgsql is
> somehow not an integral part of the system is not completely true
> anyway. Yes, we're playing favorites in the PL camp here, but so what?

True, but on the other hand, if plpgsql needs this to work nicely, then
$YOURPL probably needs it too.

regards, tom lane

On Thu, Nov 12, 2009 at 6:31 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
>> With all this fallout, I think it would be cleaner to step back and make
>> it a regular GUC variable, not custom.  Pretending that plpgsql is
>> somehow not an integral part of the system is not completely true
>> anyway.  Yes, we're playing favorites in the PL camp here, but so what?
>
> True, but on the other hand, if plpgsql needs this to work nicely, then
> $YOURPL probably needs it too.

This thread is still on the open items list, as:

Improve behavior of SUSET GUC variables added by loadable modules?

Is there something that needs to be done here? If so, what is it?

...Robert

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> This thread is still on the open items list, as:
> Improve behavior of SUSET GUC variables added by loadable modules?
> Is there something that needs to be done here? If so, what is it?

Well, if we knew exactly what to do, it wouldn't still be on the list.
The problem is that making a "custom" variable SUSET doesn't really
work: the system will not accept a value that's assigned before the
loadable module is loaded, even if it was assigned by a superuser.
I suggested a fix in the referenced thread, but it's not exactly an
ideal fix.

regards, tom lane

On Mon, Apr 19, 2010 at 9:51 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> This thread is still on the open items list, as:
>> Improve behavior of SUSET GUC variables added by loadable modules?
>> Is there something that needs to be done here?  If so, what is it?
>
> Well, if we knew exactly what to do, it wouldn't still be on the list.
> The problem is that making a "custom" variable SUSET doesn't really
> work: the system will not accept a value that's assigned before the
> loadable module is loaded, even if it was assigned by a superuser.
> I suggested a fix in the referenced thread, but it's not exactly an
> ideal fix.

Well, at this point the issue is deciding whether we're going to try
to do anything about this before beta. If we don't know what the
right solution is, that sounds to me like "no" - in which case we
should move this from the open items list to the todo list. Does that
sound reasonable to you?

...Robert

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Mon, Apr 19, 2010 at 9:51 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> The problem is that making a "custom" variable SUSET doesn't really
>> work: the system will not accept a value that's assigned before the
>> loadable module is loaded, even if it was assigned by a superuser.
>> I suggested a fix in the referenced thread, but it's not exactly an
>> ideal fix.

> Well, at this point the issue is deciding whether we're going to try
> to do anything about this before beta.

The reason it seems of concern for 9.0 is that now we have a custom
SUSET variable in plpgsql. If we don't fix this then we need to think
hard about the alternative of forcing the variable into the core code
to avoid the gotchas.

regards, tom lane

On Mon, Apr 19, 2010 at 10:29 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Mon, Apr 19, 2010 at 9:51 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> The problem is that making a "custom" variable SUSET doesn't really
>>> work: the system will not accept a value that's assigned before the
>>> loadable module is loaded, even if it was assigned by a superuser.
>>> I suggested a fix in the referenced thread, but it's not exactly an
>>> ideal fix.
>
>> Well, at this point the issue is deciding whether we're going to try
>> to do anything about this before beta.
>
> The reason it seems of concern for 9.0 is that now we have a custom
> SUSET variable in plpgsql.  If we don't fix this then we need to think
> hard about the alternative of forcing the variable into the core code
> to avoid the gotchas.

Well, having reread your proposed solution, it sounds pretty
reasonable to me. You're never going to be able to make totally
sensible decisions about GUCs if the code that defines those GUCs
isn't loaded, so requiring that the code be loaded before any GUCs are
set seems like a sensible thing to do. On the other hand, if forcing
this into core gets a beta out the door sooner, maybe that's the way
to go, even though I wouldn't exactly classify it as an elegant
solution.

Or to put it another way - this thread has been sitting idle for 5
months; it's time to make a decision.

...Robert

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Mon, Apr 19, 2010 at 10:29 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> The reason it seems of concern for 9.0 is that now we have a custom
>> SUSET variable in plpgsql. If we don't fix this then we need to think
>> hard about the alternative of forcing the variable into the core code
>> to avoid the gotchas.

> Well, having reread your proposed solution, it sounds pretty
> reasonable to me. You're never going to be able to make totally
> sensible decisions about GUCs if the code that defines those GUCs
> isn't loaded, so requiring that the code be loaded before any GUCs are
> set seems like a sensible thing to do. On the other hand, if forcing
> this into core gets a beta out the door sooner, maybe that's the way
> to go, even though I wouldn't exactly classify it as an elegant
> solution.

> Or to put it another way - this thread has been sitting idle for 5
> months; it's time to make a decision.

Well, if there are no other comments, I'll push forward with the fix
proposed here:
http://archives.postgresql.org/pgsql-hackers/2009-11/msg00531.php

regards, tom lane

I wrote:
> Well, if there are no other comments, I'll push forward with the fix
> proposed here:
> http://archives.postgresql.org/pgsql-hackers/2009-11/msg00531.php

Done. I did not make the change I speculated about of allowing
completely unknown variables (those that don't even match
custom_variable_classes) to be set by superusers. It would be a very
minor tweak to the committed code to allow that, but I'm not convinced
that making a corner case in dump/restore slightly easier is worth the
loss of error checking. In practice, if you have ALTER ... SETs for
custom variables, you'd better list their modules in
custom_variable_classes, or it won't work nicely. I see no really
strong reason not to fix that parameter before you restore instead of
after.

regards, tom lane