Should we get rid of custom_variable_classes altogether?

On Sun, Oct 2, 2011 at 10:05 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

During the discussion of Alexey Klyukin's rewrite of ParseConfigFile,
considerable unhappiness was expressed by various people about the
complexity and relative uselessness of the custom_variable_classes GUC.
While working over his patch just now, I've come around to the side that
was saying that this variable isn't worth its keep.  We don't have any
way to validate whether the second part of a qualified GUC name is
correct, if its associated extension module isn't loaded, so how much
point is there in validating the first part?  And the variable is
certainly a pain in the rear both to DBAs and to the GUC code itself.

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.

--
 Simon Riggs                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Simon Riggs <simon@2ndQuadrant.com> writes:

On Sun, Oct 2, 2011 at 10:05 PM, Tom Lane <tgl@sss.pgh.pa.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.

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.

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.

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?

regards, tom lane

Tom Lane <tgl@sss.pgh.pa.us> writes:

Simon Riggs <simon@2ndQuadrant.com> writes:

On Sun, Oct 2, 2011 at 10:05 PM, Tom Lane <tgl@sss.pgh.pa.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

On Sun, Oct 2, 2011 at 23:05, Tom Lane <tgl@sss.pgh.pa.us> wrote:

During the discussion of Alexey Klyukin's rewrite of ParseConfigFile,
considerable unhappiness was expressed by various people about the
complexity and relative uselessness of the custom_variable_classes GUC.
While working over his patch just now, I've come around to the side that
was saying that this variable isn't worth its keep.  We don't have any
way to validate whether the second part of a qualified GUC name is
correct, if its associated extension module isn't loaded, so how much
point is there in validating the first part?  And the variable is
certainly a pain in the rear both to DBAs and to the GUC code itself.

Don't forget that there are usecases for variables under
custom_variable_classes that aren't actually associated with
extensions (as general session-shared-variables). Though I guess if it
was somehow restricted to extensions, those who needed that could just
rewrap all their code as extensions - though that would make it less
convenient.

The point being that even if you *could* validate them somehow against
a static list, requiring that might not be a good idea.

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.

Seems reasonable to me.

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

Magnus Hagander <magnus@hagander.net> writes:

Don't forget that there are usecases for variables under
custom_variable_classes that aren't actually associated with
extensions (as general session-shared-variables). Though I guess if it
was somehow restricted to extensions, those who needed that could just
rewrap all their code as extensions - though that would make it less
convenient.

Right. Getting rid of custom_variable_classes should actually make
those use-cases easier, since it will eliminate a required setup step.

I tried to think of a security argument for keeping the setting, but
couldn't really. Yeah, not having it will let people clutter their
individual backend's GUC array with lots of useless stuff, but so what?
There's plenty of other ways to run your session out of memory if you're
so inclined.

regards, tom lane

On 10/03/2011 10:17 AM, Tom Lane wrote:

Magnus Hagander<magnus@hagander.net> writes:

Don't forget that there are usecases for variables under
custom_variable_classes that aren't actually associated with
extensions (as general session-shared-variables). Though I guess if it
was somehow restricted to extensions, those who needed that could just
rewrap all their code as extensions - though that would make it less
convenient.

Right. Getting rid of custom_variable_classes should actually make
those use-cases easier, since it will eliminate a required setup step.

I tried to think of a security argument for keeping the setting, but
couldn't really. Yeah, not having it will let people clutter their
individual backend's GUC array with lots of useless stuff, but so what?
There's plenty of other ways to run your session out of memory if you're
so inclined.

So are we going to sanction using this as a poor man's session variable
mechanism?

If so maybe we should at least warn that anything set will be accessible
by all roles, so security definer functions for example should be wary
of trusting such values.

cheers

andrew

Andrew Dunstan <andrew@dunslane.net> writes:

On 10/03/2011 10:17 AM, Tom Lane wrote:

Right. Getting rid of custom_variable_classes should actually make
those use-cases easier, since it will eliminate a required setup step.

So are we going to sanction using this as a poor man's session variable
mechanism?

People already are doing that, sanctioned or not.

If so maybe we should at least warn that anything set will be accessible
by all roles, so security definer functions for example should be wary
of trusting such values.

Since it's not documented anywhere, I'm not sure where we'd put such
a warning. I think anyone bright enough to think of such a hack should
be able to see the potential downsides, anyway.

regards, tom lane

On Mon, Oct 03, 2011 at 10:41:48AM -0400, Tom Lane wrote:

Andrew Dunstan <andrew@dunslane.net> writes:

On 10/03/2011 10:17 AM, Tom Lane wrote:

Right. Getting rid of custom_variable_classes should actually
make those use-cases easier, since it will eliminate a required
setup step.

So are we going to sanction using this as a poor man's session
variable mechanism?

People already are doing that, sanctioned or not.

If so maybe we should at least warn that anything set will be
accessible by all roles, so security definer functions for example
should be wary of trusting such values.

Since it's not documented anywhere, I'm not sure where we'd put such
a warning. I think anyone bright enough to think of such a hack
should be able to see the potential downsides, anyway.

Perhaps it's best to document this usage and include the warning for
those less "bright," as you term them. I'd be less tempted to call
them "not bright" and more tempted to think they might assume
PostgreSQL already takes care of cleaning this up, but whatever.

Cheers,
David.
--
David Fetter <david@fetter.org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david.fetter@gmail.com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

On Mon, Oct 3, 2011 at 10:55 AM, David Fetter <david@fetter.org> wrote:

Perhaps it's best to document this usage and include the warning for
those less "bright," as you term them.   I'd be less tempted to call
them "not bright" and more tempted to think they might assume
PostgreSQL already takes care of cleaning this up, but whatever.

Yeah. custom_variable_classes is a pretty annoying wart, but if it's
set to the default value (namely, empty) then it actually does prevent
people from setting bajillions of completely pointless settings, which
seems like it has some merit. I'm not sure it has enough merit to
justify keeping it around, but it has more than none. We could allow
entering a date of February 31st, too, but we don't.

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

Robert Haas <robertmhaas@gmail.com> writes:

Yeah. custom_variable_classes is a pretty annoying wart, but if it's
set to the default value (namely, empty) then it actually does prevent
people from setting bajillions of completely pointless settings, which
seems like it has some merit. I'm not sure it has enough merit to
justify keeping it around, but it has more than none. We could allow
entering a date of February 31st, too, but we don't.

Well, that argument was essentially why we put it in to begin with.
But I think pretty much everybody agrees that it's more trouble than
it's worth (in fact, weren't you one of the people complaining about
it?)

regards, tom lane

On Mon, Oct 3, 2011 at 11:16 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@gmail.com> writes:

Yeah.  custom_variable_classes is a pretty annoying wart, but if it's
set to the default value (namely, empty) then it actually does prevent
people from setting bajillions of completely pointless settings, which
seems like it has some merit.  I'm not sure it has enough merit to
justify keeping it around, but it has more than none.  We could allow
entering a date of February 31st, too, but we don't.

Well, that argument was essentially why we put it in to begin with.
But I think pretty much everybody agrees that it's more trouble than
it's worth (in fact, weren't you one of the people complaining about
it?)

Well, yes. But I was arguing that we should replace the leaky dam
with one that's watertight, rather than demolishing the dam.

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

Robert Haas <robertmhaas@gmail.com> writes:

On Mon, Oct 3, 2011 at 11:16 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@gmail.com> writes:

Yeah. �custom_variable_classes is a pretty annoying wart, but if it's
set to the default value (namely, empty) then it actually does prevent
people from setting bajillions of completely pointless settings, which
seems like it has some merit.

Well, that argument was essentially why we put it in to begin with.
But I think pretty much everybody agrees that it's more trouble than
it's worth (in fact, weren't you one of the people complaining about
it?)

Well, yes. But I was arguing that we should replace the leaky dam
with one that's watertight, rather than demolishing the dam.

If we had some idea how to do that, I'd probably agree. But we don't.
In any case, custom_variable_classes as currently defined is not the
basis for a solution to that desire, and removing it won't create an
impediment to solving the problem properly, should we come up with
a solution.

(This is, however, a good reason for continuing to not document that
you can create random GUC variables --- we might someday shut that
off again.)

regards, tom lane

On Mon, Oct 3, 2011 at 12:25 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@gmail.com> writes:

On Mon, Oct 3, 2011 at 11:16 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@gmail.com> writes:

Yeah.  custom_variable_classes is a pretty annoying wart, but if it's
set to the default value (namely, empty) then it actually does prevent
people from setting bajillions of completely pointless settings, which
seems like it has some merit.

Well, that argument was essentially why we put it in to begin with.
But I think pretty much everybody agrees that it's more trouble than
it's worth (in fact, weren't you one of the people complaining about
it?)

Well, yes.  But I was arguing that we should replace the leaky dam
with one that's watertight, rather than demolishing the dam.

If we had some idea how to do that, I'd probably agree.  But we don't.
In any case, custom_variable_classes as currently defined is not the
basis for a solution to that desire, and removing it won't create an
impediment to solving the problem properly, should we come up with
a solution.

Yeah, that's why I'm not complaining too loudly. :-)

(This is, however, a good reason for continuing to not document that
you can create random GUC variables --- we might someday shut that
off again.)

Or maybe better still would be to explicitly document the fact that
behavior in this area should not be relied upon.

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

David Fetter <david@fetter.org> writes:

Perhaps it's best to document this usage and include the warning for
those less "bright," as you term them. I'd be less tempted to call
them "not bright" and more tempted to think they might assume
PostgreSQL already takes care of cleaning this up, but whatever.

Who's that dim? D'oh.

Another compromise might be to allow for defining variable in any class
from the configuration files but restrict that to existing classes from
the SET command. Wait, that's exactly what happens as soon as there's
no explicit custom_variable_classes, right?

So we're talking about people with configuration file editing and reload
powers, not about anyone who can connect. I think that's ok.

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

Dimitri Fontaine <dimitri@2ndQuadrant.fr> writes:

Another compromise might be to allow for defining variable in any class
from the configuration files but restrict that to existing classes from
the SET command. Wait, that's exactly what happens as soon as there's
no explicit custom_variable_classes, right?

No, because there are people who do intentionally use placeholder
variables as session-local storage, and that would be taking away
that capability.

regards, tom lane

Tom Lane <tgl@sss.pgh.pa.us> writes:

Dimitri Fontaine <dimitri@2ndQuadrant.fr> writes:

Another compromise might be to allow for defining variable in any class
from the configuration files but restrict that to existing classes from
the SET command. Wait, that's exactly what happens as soon as there's
no explicit custom_variable_classes, right?

No, because there are people who do intentionally use placeholder
variables as session-local storage, and that would be taking away
that capability.

They would have to set the variable to its default value in some
configuration file and reload, just as now. They wouldn't have to also
edit custom_variable_classes, that's about it.

Or do you want to open SET typo.wrogn TO 'foobar' to just work silently?

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

Dimitri Fontaine <dimitri@2ndQuadrant.fr> writes:

Tom Lane <tgl@sss.pgh.pa.us> writes:

No, because there are people who do intentionally use placeholder
variables as session-local storage, and that would be taking away
that capability.

Or do you want to open SET typo.wrogn TO 'foobar' to just work silently?

Well, right at the moment it *does* work silently, as long as the prefix
is one you listed in custom_variable_classes. I don't think we want to
take that away, and in particular I don't want to assume that every
variable will be declared in advance. It's a fairly safe bet that there
are some apps out there that would be broken by such a requirement.

At the same time, I'd kind of like to see a facility for declaring such
variables, if only so you could define them to be bool/int/real not just
strings. But this is getting far afield from the immediate proposal,
and no I'm not volunteering to do it.

regards, tom lane

Tom Lane <tgl@sss.pgh.pa.us> writes:

Or do you want to open SET typo.wrogn TO 'foobar' to just work silently?

Well, right at the moment it *does* work silently, as long as the prefix
is one you listed in custom_variable_classes. I don't think we want to
take that away, and in particular I don't want to assume that every
variable will be declared in advance. It's a fairly safe bet that there
are some apps out there that would be broken by such a requirement.

Fair enough I suppose. But I think we could break that requirement if
we offer a good enough way out.

At the same time, I'd kind of like to see a facility for declaring such
variables, if only so you could define them to be bool/int/real not just
strings. But this is getting far afield from the immediate proposal,
and no I'm not volunteering to do it.

I think we are able to handle that part when dealing with C extension's
GUCs, because those are declared in the .so. We only need to add them
to the control file, the only road block here used to be c_v_c.

What I have in mind for extensions now that c_v_c is out would be to be
able to declare any GUC in the control file, with default values, and
without forcing extension to handle the GUC in its .so — I don't think
we have to change the code beside removing the c_v_c checks here.

In the general case, how far exposing DefineCustomBoolVariable and all
at the SQL level would get us? Then you could allow the session to add
any new GUC by calling that first, SET would bail out if given unknown
variable.

Yes, it would still break some existing applications, but I think it'd
be worth it (and an easy fix too, as I guess most shared variables are
going to be used in PL code, and if you ask me this code should now be
an extension and the control file would then declare the GUCs).

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

Dimitri Fontaine <dimitri@2ndQuadrant.fr> writes:

What I have in mind for extensions now that c_v_c is out would be to be
able to declare any GUC in the control file, with default values, and
without forcing extension to handle the GUC in its .so — I don't think
we have to change the code beside removing the c_v_c checks here.

What's the point of that? A value in an extension control file isn't
particularly easily accessible. You'd basically only see it when
loading the extension, and that's a scenario in which the existing
mechanism works just fine. I see no reason to break existing code
here.

regards, tom lane

Tom Lane <tgl@sss.pgh.pa.us> writes:

Dimitri Fontaine <dimitri@2ndQuadrant.fr> writes:

What I have in mind for extensions now that c_v_c is out would be to be
able to declare any GUC in the control file, with default values, and
without forcing extension to handle the GUC in its .so — I don't think
we have to change the code beside removing the c_v_c checks here.

What's the point of that? A value in an extension control file isn't
particularly easily accessible. You'd basically only see it when
loading the extension, and that's a scenario in which the existing
mechanism works just fine. I see no reason to break existing code
here.

It's not about the code behavior but user support and packaging. That
the code does the right DefineCustom calls is very good, but users
should be able to easily alter defaults after installing an extension.
And you're right, putting the setup into the control file is not
providing that.

We could have some extension.conf file. Appending to postgresql.conf is
not possible from a third-party package per debian's policy, so having
extension/foo.conf instead would make sense here.

But at this point, it's nothing you need to care right now in your patch
I guess, unless you're motivated enough :)

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

Dimitri Fontaine <dimitri@2ndQuadrant.fr> writes:

Tom Lane <tgl@sss.pgh.pa.us> writes:

Dimitri Fontaine <dimitri@2ndQuadrant.fr> writes:

What I have in mind for extensions now that c_v_c is out would be to be
able to declare any GUC in the control file, with default values, and
without forcing extension to handle the GUC in its .so — I don't think
we have to change the code beside removing the c_v_c checks here.

What's the point of that? A value in an extension control file isn't
particularly easily accessible. You'd basically only see it when
loading the extension, and that's a scenario in which the existing
mechanism works just fine. I see no reason to break existing code
here.

It's not about the code behavior but user support and packaging. That
the code does the right DefineCustom calls is very good, but users
should be able to easily alter defaults after installing an extension.
And you're right, putting the setup into the control file is not
providing that.

I still don't see the point. If they want to change the default
setting, they add an entry to postgresql.conf. Problem solved.

We could have some extension.conf file. Appending to postgresql.conf is
not possible from a third-party package per debian's policy, so having
extension/foo.conf instead would make sense here.

This is adding even more complication to solve a non-problem.
May I remind you that a lot of people think that the default entries in
postgresql.conf for the core settings are a bad idea? Why should we
invent even more mechanism (and more conventions for users to remember)
to duplicate something of questionable value?

regards, tom lane

Tom Lane <tgl@sss.pgh.pa.us> writes:

I still don't see the point. If they want to change the default
setting, they add an entry to postgresql.conf. Problem solved.

As you wish. They will have to figure the current defaults in some
other way then edit the file. That's good enough for now anyway.

We could have some extension.conf file. Appending to postgresql.conf is
not possible from a third-party package per debian's policy, so having
extension/foo.conf instead would make sense here.

This is adding even more complication to solve a non-problem.

Mmm. Ok.

May I remind you that a lot of people think that the default entries in
postgresql.conf for the core settings are a bad idea? Why should we
invent even more mechanism (and more conventions for users to remember)
to duplicate something of questionable value?

It could be the timing when I try to sell my idea of one file per GUC,
then extensions would simply add a bunch of files in there. The value
of doing one GUC per file is not having to parse anything, the first non
line is the value, the rest of the file free form comments.

With this model, there's no new setup mechanism.

But anyhow, all that can wait until after you get rid of
custom_variable_classes, I think we're talking about what could happen
next here, if anything.

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

On Mon, Oct 3, 2011 at 00:05, Tom Lane <tgl@sss.pgh.pa.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.

Comments, other proposals?

While working on E.164 telephone numbers datatype contrib module
(https://github.com/commandprompt/e164/commits/guc) I've stumbled
across this problem: how do I add regression tests involving the
module-defined GUC option?

Trying to hack postgresql.conf to include e164 in the
custom_variable_classes then send it a HUP doesn't seem to be an
option. But it seems that you cannot (re)set it otherwise. See:

$ psql -d contrib_regression
psql (9.1.0)
Type "help" for help.

contrib_regression=# SET e164.area_codes_format='';
ERROR: unrecognized configuration parameter "e164.area_codes_format"
contrib_regression=# SET custom_variable_classes='e164';
ERROR: parameter "custom_variable_classes" cannot be changed now

I wonder how/if other contrib modules ever do regression tests on
their GUC options?

At this rate, removing the custom_variable_classes option altogether
is pretty much going to solve my problem.

--
Regards,
Alex