Re: guc patch: Make variables fall back to default values

Attached is the long-awaited guc patch that makes values fall back to their
default values when they got removed (or commented) from the configuration
file. This has always been a source of confusion.

There are three not-so-obvious cases that I'd like to comment:

First one:

In the configuration file you have:

seq_page_cost = 3 (the default for this option is 1)

You start the database and issue "SET seq_page_cost TO 4".

Then you remove the seq_page_cost definition from the configuration file and
send SIGHUP.

If you now do a "RESET seq_page_cost" it will fall back to 1 and not to 3.

Second one:

You have custom_variable_classes = "foo"

You start a transaction and do "SET foo.bar to 4".

Now you remove the custom_variable_classes definition and it falls back to
being empty. Hence all foo.* variables become invalid. You cannot COMMIT the
transaction and COMMIT results in a transaction abort.

Third one:

In the configuration file you have

custom_variable_classes = "foo"
foo.bar = 3

You start a transaction and do "SET foo.bar to 4". Then you remove the
definition of "foo.bar" but you keep the custom_variable_classes definition.
COMMITting the transaction succeeds but since foo.bar does not exist in the
configuration file anymore, your SET command is considered to define a new
variable and executing "RESET foo.bar" does not change the variable (without
any change to the configuration file it would remove your setting and
restore the setting from the configuration file for "foo.bar").

Everything else should be quite straightforward. It is also intended that if
you have changed (or commented) a variable in the configuration file that
cannot be applied (because a parameter can only be changed at server start)
you will get this message every time you send a SIGHUP. That way you can see
if your configuration file matches your current server configuration.

Comments welcome,

Joachim

Your patch has been added to the PostgreSQL unapplied patches list at:

http://momjian.postgresql.org/cgi-bin/pgpatches

It will be applied as soon as one of the PostgreSQL committers reviews
and approves it.

---------------------------------------------------------------------------

Joachim Wieland wrote:
> Attached is the long-awaited guc patch that makes values fall back to their
> default values when they got removed (or commented) from the configuration
> file. This has always been a source of confusion.
>
> There are three not-so-obvious cases that I'd like to comment:
>
> First one:
>
> In the configuration file you have:
>
> seq_page_cost = 3 (the default for this option is 1)
>
> You start the database and issue "SET seq_page_cost TO 4".
>
> Then you remove the seq_page_cost definition from the configuration file and
> send SIGHUP.
>
> If you now do a "RESET seq_page_cost" it will fall back to 1 and not to 3.
>
>
>
> Second one:
>
> You have custom_variable_classes = "foo"
>
> You start a transaction and do "SET foo.bar to 4".
>
> Now you remove the custom_variable_classes definition and it falls back to
> being empty. Hence all foo.* variables become invalid. You cannot COMMIT the
> transaction and COMMIT results in a transaction abort.
>
>
>
> Third one:
>
> In the configuration file you have
>
> custom_variable_classes = "foo"
> foo.bar = 3
>
> You start a transaction and do "SET foo.bar to 4". Then you remove the
> definition of "foo.bar" but you keep the custom_variable_classes definition.
> COMMITting the transaction succeeds but since foo.bar does not exist in the
> configuration file anymore, your SET command is considered to define a new
> variable and executing "RESET foo.bar" does not change the variable (without
> any change to the configuration file it would remove your setting and
> restore the setting from the configuration file for "foo.bar").
>
>
>
> Everything else should be quite straightforward. It is also intended that if
> you have changed (or commented) a variable in the configuration file that
> cannot be applied (because a parameter can only be changed at server start)
> you will get this message every time you send a SIGHUP. That way you can see
> if your configuration file matches your current server configuration.
>
>
>
> Comments welcome,
>
> Joachim
>

[ Attachment, skipping... ]

>
> ---------------------------(end of broadcast)---------------------------
> TIP 3: Have you checked our extensive FAQ?
>
> http://www.postgresql.org/docs/faq

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Joachim Wieland wrote:
> Attached is the long-awaited guc patch that makes values fall back to
> their default values when they got removed (or commented) from the
> configuration file. This has always been a source of confusion.

This patch makes, in its source code comments and error messages, overly
enthusiastic references to the fact that a parameter setting was
supposedly "commented". The only information that is really available,
however, is that the parameter setting disappeared from the
configuration file, and we should not be making other claims.

Another issue that strikes me is that the GUC code apparently makes
mixed used of palloc and guc_malloc, and this patch continues that.
I'm not sure if this is really well thought out.

--
Peter Eisentraut
http://developer.postgresql.org/~petere/

On Sat, Mar 10, 2007 at 09:35:38PM +0100, Peter Eisentraut wrote:
> This patch makes, in its source code comments and error messages, overly
> enthusiastic references to the fact that a parameter setting was
> supposedly "commented". The only information that is really available,
> however, is that the parameter setting disappeared from the
> configuration file, and we should not be making other claims.

Okay, this is an easy fix.

> Another issue that strikes me is that the GUC code apparently makes
> mixed used of palloc and guc_malloc, and this patch continues that.

True, there is some confusion in the GUC code about what allocation routine
should be used. I tried to use the same allocation method as an
already-existing similar allocation.
To lessen this confusion, can you do some cleanup work on the current GUC
code in this area that I can use as a basis for a revised version of the
patch?

Did you also do tests on the functional aspects of the patch?

Joachim

Joachim Wieland wrote:
> Attached is the long-awaited guc patch that makes values fall back to
> their default values when they got removed (or commented) from the
> configuration file. This has always been a source of confusion.

I have applied your patch with some of the discussed corrections. The
use of memory allocation in the GUC code might still need some review.

--
Peter Eisentraut
http://developer.postgresql.org/~petere/

"Peter Eisentraut" <peter_e(at)gmx(dot)net> writes:

> Joachim Wieland wrote:
>> Attached is the long-awaited guc patch that makes values fall back to
>> their default values when they got removed (or commented) from the
>> configuration file. This has always been a source of confusion.
>
> I have applied your patch with some of the discussed corrections. The
> use of memory allocation in the GUC code might still need some review.

This is a pretty major user-visible change. While I'm strongly in favour of it
it seems like there ought to be some documentation file touched by this, no?
Or have I missed it?

--
Gregory Stark
EnterpriseDB http://www.enterprisedb.com

Gregory Stark wrote:
> This is a pretty major user-visible change. While I'm strongly in
> favour of it it seems like there ought to be some documentation file
> touched by this, no? Or have I missed it?

In my opinion, and possibly that of others who have worked on this
issue, the old behavior was a pretty much a bug and now it works as
expected. Not sure how to document that.

--
Peter Eisentraut
http://developer.postgresql.org/~petere/

Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> Gregory Stark wrote:
>> This is a pretty major user-visible change. While I'm strongly in
>> favour of it it seems like there ought to be some documentation file
>> touched by this, no? Or have I missed it?

> In my opinion, and possibly that of others who have worked on this
> issue, the old behavior was a pretty much a bug and now it works as
> expected. Not sure how to document that.

It's a release-note item ... assuming that it doesn't get reverted in
the near future. Could we have some attention to the all-red buildfarm?

regards, tom lane

"Tom Lane" <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:

> It's a release-note item ... assuming that it doesn't get reverted in
> the near future. Could we have some attention to the all-red buildfarm?

It's not just a bug. There's code missing.

The code seems to assume that all custom variables are strings. There are
about half a dozen Assert(variable->vartype == PGC_STRING) throughout the
patch. That's not true, plperl's use_strict is a boolean and we have
DefineCustome*Variable functions for each type of variable. Perl bombs because
plperl.use_strict is a boolean.

--
Gregory Stark
EnterpriseDB http://www.enterprisedb.com

On Tue, Mar 13, 2007 at 08:22:17AM +0000, Gregory Stark wrote:
> The code seems to assume that all custom variables are strings. There are
> about half a dozen Assert(variable->vartype == PGC_STRING) throughout the
> patch. That's not true, plperl's use_strict is a boolean and we have
> DefineCustome*Variable functions for each type of variable. Perl bombs
> because plperl.use_strict is a boolean.

The attached patch removes those Asserts.

But this is not the whole story. I wonder why setting "plperl.use_strict"
is supposed to work at all? Where is the corresponding definition of
"plperl" as a custom variable class? I can add it manually to
postgresql.conf and make the regression tests work but is this the intended
way?

Joachim

Joachim Wieland wrote:
> On Tue, Mar 13, 2007 at 08:22:17AM +0000, Gregory Stark wrote:
>
>> The code seems to assume that all custom variables are strings. There are
>> about half a dozen Assert(variable->vartype == PGC_STRING) throughout the
>> patch. That's not true, plperl's use_strict is a boolean and we have
>> DefineCustome*Variable functions for each type of variable. Perl bombs
>> because plperl.use_strict is a boolean.
>>
>
> The attached patch removes those Asserts.
>
> But this is not the whole story. I wonder why setting "plperl.use_strict"
> is supposed to work at all? Where is the corresponding definition of
> "plperl" as a custom variable class? I can add it manually to
> postgresql.conf and make the regression tests work but is this the intended
> way?
>
>
>

The whole custom variable gadget is a mess, IMNSHO, and almost
completely undocumented. If anyone comes up with a sane, workable and
documented design I'll be happy.

As I understand the way custom variable class works (or is supposed to
work), you need it if the variable is set in advance of the call to
define it (e.g. in postgresql.conf) but not if you set if after.

cheers

andrew

Gregory Stark <stark(at)enterprisedb(dot)com> writes:
> It's not just a bug. There's code missing.

> The code seems to assume that all custom variables are strings. There are
> about half a dozen Assert(variable->vartype == PGC_STRING) throughout the
> patch. That's not true, plperl's use_strict is a boolean and we have
> DefineCustome*Variable functions for each type of variable.

Well, they *are* strings as long as they're "custom". Once a
DefineCustomFoo has been executed, there (should be) no difference
between a "custom" variable and a hard-wired one.

The thing that I was wondering about is the same Joachim mentioned: how
is it that the regression test ever worked? The answer is that it's
not really testing custom variables, because it doesn't try to set
plperl.use_strict until after plperl has been loaded into the current
session. So by that time the variable exists and should look like a
perfectly ordinary boolean GUC variable. The fact that it doesn't look
like that says to me that there's something wrong with the patch logic,
over and above the question of what it should be Asserting.

regards, tom lane

Peter Eisentraut wrote:
> Joachim Wieland wrote:
> > Attached is the long-awaited guc patch that makes values fall back
> > to their default values when they got removed (or commented) from
> > the configuration file. This has always been a source of confusion.
>
> I have applied your patch with some of the discussed corrections.
> The use of memory allocation in the GUC code might still need some
> review.

Reverted for further fixes. Attached is the latest patch I was working
with.

--
Peter Eisentraut
http://developer.postgresql.org/~petere/

Tom Lane wrote:
> The thing that I was wondering about is the same Joachim mentioned:
> how is it that the regression test ever worked? The answer is that
> it's not really testing custom variables, because it doesn't try to
> set plperl.use_strict until after plperl has been loaded into the
> current session.

I think that the sole purpose of c_v_c is to allow custom variables in
the configuration file, because that is possibly read before modules
are loaded. Basically it just means that "prefix.*" is not rejected.
In a session, it doesn't make a difference what c_v_c is set to; the
variable needs to be registered period. However, if the registration
code runs only when the module is invoked for the first time rather
than at the start of the session (as in the case of plperl), then it's
apparently impossible to set a variable in a session before the first
call. It's all very weird.

--
Peter Eisentraut
http://developer.postgresql.org/~petere/

On Tue, Mar 13, 2007 at 10:19:54AM -0400, Tom Lane wrote:
> Gregory Stark <stark(at)enterprisedb(dot)com> writes:
> > It's not just a bug. There's code missing.

> > The code seems to assume that all custom variables are strings. There are
> > about half a dozen Assert(variable->vartype == PGC_STRING) throughout the
> > patch. That's not true, plperl's use_strict is a boolean and we have
> > DefineCustome*Variable functions for each type of variable.

> Well, they *are* strings as long as they're "custom". Once a
> DefineCustomFoo has been executed, there (should be) no difference
> between a "custom" variable and a hard-wired one.

The code in question is the only place that calls one of the
DefineCustom*Variable functions. But those functions set
var->group = CUSTOM_OPTIONS what makes variables look like custom variables
defined via SQL or the config file but in reality they aren't. Hence the
confusion of the type assertion.

> The thing that I was wondering about is the same Joachim mentioned: how
> is it that the regression test ever worked? The answer is that it's
> not really testing custom variables, because it doesn't try to set
> plperl.use_strict until after plperl has been loaded into the current
> session. So by that time the variable exists and should look like a
> perfectly ordinary boolean GUC variable. The fact that it doesn't look
> like that says to me that there's something wrong with the patch logic,
> over and above the question of what it should be Asserting.

What is wrong is that plperl defines a variable that is a mix of a guc
variable and a custom variable. It claims being a custom variable by setting
var->group = CUSTOM_OPTIONS but it does not set the respective
custom_variable_class and so by definition it can't be a custom variable.

Joachim

Joachim Wieland <joe(at)mcknight(dot)de> writes:
> On Tue, Mar 13, 2007 at 10:19:54AM -0400, Tom Lane wrote:
>> Well, they *are* strings as long as they're "custom". Once a
>> DefineCustomFoo has been executed, there (should be) no difference
>> between a "custom" variable and a hard-wired one.

> The code in question is the only place that calls one of the
> DefineCustom*Variable functions. But those functions set
> var->group = CUSTOM_OPTIONS what makes variables look like custom variables
> defined via SQL or the config file but in reality they aren't. Hence the
> confusion of the type assertion.

My point here that you shouldn't be using var->group to make any
semantic choices. That's supposed to be a label for user convenience,
nothing else.

regards, tom lane

On Tue, Mar 13, 2007 at 11:08:52AM -0400, Tom Lane wrote:
> Joachim Wieland <joe(at)mcknight(dot)de> writes:
> > On Tue, Mar 13, 2007 at 10:19:54AM -0400, Tom Lane wrote:
> >> Well, they *are* strings as long as they're "custom". Once a
> >> DefineCustomFoo has been executed, there (should be) no difference
> >> between a "custom" variable and a hard-wired one.

> > The code in question is the only place that calls one of the
> > DefineCustom*Variable functions. But those functions set
> > var->group = CUSTOM_OPTIONS what makes variables look like custom variables
> > defined via SQL or the config file but in reality they aren't. Hence the
> > confusion of the type assertion.

> My point here that you shouldn't be using var->group to make any
> semantic choices. That's supposed to be a label for user convenience,
> nothing else.

Then what is the criterion to tell what is a custom variable and what isn't?
If it contains a dot in the name it is? This wouldn't resolve the problem at
hand either... :-(

We might have to think about custom variables as a whole, what we have now
seems like a very unclear definition and everybody has his own opinion about
what it is and how it works (and I'm not excluding myself here :-)).

Joachim

Joachim Wieland <joe(at)mcknight(dot)de> writes:
> On Tue, Mar 13, 2007 at 11:08:52AM -0400, Tom Lane wrote:
>> My point here that you shouldn't be using var->group to make any
>> semantic choices. That's supposed to be a label for user convenience,
>> nothing else.

> Then what is the criterion to tell what is a custom variable and what isn't?

Why do you need to tell that? IMHO, once the DefineCustomFoo function
has been executed, it should be exactly like any other variable (other
than having a funny name).

regards, tom lane

On Tue, Mar 13, 2007 at 11:52:38AM -0400, Tom Lane wrote:
> > Then what is the criterion to tell what is a custom variable and what isn't?
> Why do you need to tell that? IMHO, once the DefineCustomFoo function
> has been executed, it should be exactly like any other variable (other
> than having a funny name).

For example for the fall-back-to-default patch. I might not need to tell if
it has been introduced by one of the DefineCustomFoo functions but for the
"other" custom variables. Imagine that we have defined a custom variable via
the configuration file, remove it and send SIGHUP. My understanding is that
this variable should then be deleted from the pool of valid variables
because it falls back to its default value and the default value of a custom
variable is its non-existance.

Joachim

Joachim Wieland <joe(at)mcknight(dot)de> writes:
> On Tue, Mar 13, 2007 at 11:52:38AM -0400, Tom Lane wrote:
>> Why do you need to tell that? IMHO, once the DefineCustomFoo function
>> has been executed, it should be exactly like any other variable (other
>> than having a funny name).

> For example for the fall-back-to-default patch. I might not need to tell if
> it has been introduced by one of the DefineCustomFoo functions but for the
> "other" custom variables. Imagine that we have defined a custom variable via
> the configuration file, remove it and send SIGHUP. My understanding is that
> this variable should then be deleted from the pool of valid variables
> because it falls back to its default value and the default value of a custom
> variable is its non-existance.

Once DefineCustomFoo has been executed, you have a reset value to fall
back to. I think what you really want is to recognize variables that
are in the placeholder state, and have them go away as above.
For that you check the GUC_CUSTOM_PLACEHOLDER flag. In any case there
must never be any use of var->group for decision making; that's simply
wrong.

However, ISTM that forcing variables to go away is useless extra code.
What purpose does it serve? Not error checking, because you already
accepted the variable before. Surely you wouldn't argue that, say,
reverting to a prior setting of check_function_bodies should cause the
system to go back and validate a CREATE FUNCTION command it has already
accepted. Moreover, while you could perhaps argue that the "principle
of least surprise" cuts either way here, it seems to me there's a good
argument for not throwing away variables: you might be discarding data
the user needed. So I'd vote for just leaving them there.

regards, tom lane

Is there a new version of this patch being worked on?

---------------------------------------------------------------------------

Tom Lane wrote:
> Joachim Wieland <joe(at)mcknight(dot)de> writes:
> > On Tue, Mar 13, 2007 at 11:52:38AM -0400, Tom Lane wrote:
> >> Why do you need to tell that? IMHO, once the DefineCustomFoo function
> >> has been executed, it should be exactly like any other variable (other
> >> than having a funny name).
>
> > For example for the fall-back-to-default patch. I might not need to tell if
> > it has been introduced by one of the DefineCustomFoo functions but for the
> > "other" custom variables. Imagine that we have defined a custom variable via
> > the configuration file, remove it and send SIGHUP. My understanding is that
> > this variable should then be deleted from the pool of valid variables
> > because it falls back to its default value and the default value of a custom
> > variable is its non-existance.
>
> Once DefineCustomFoo has been executed, you have a reset value to fall
> back to. I think what you really want is to recognize variables that
> are in the placeholder state, and have them go away as above.
> For that you check the GUC_CUSTOM_PLACEHOLDER flag. In any case there
> must never be any use of var->group for decision making; that's simply
> wrong.
>
> However, ISTM that forcing variables to go away is useless extra code.
> What purpose does it serve? Not error checking, because you already
> accepted the variable before. Surely you wouldn't argue that, say,
> reverting to a prior setting of check_function_bodies should cause the
> system to go back and validate a CREATE FUNCTION command it has already
> accepted. Moreover, while you could perhaps argue that the "principle
> of least surprise" cuts either way here, it seems to me there's a good
> argument for not throwing away variables: you might be discarding data
> the user needed. So I'd vote for just leaving them there.
>
> regards, tom lane
>
> ---------------------------(end of broadcast)---------------------------
> TIP 9: In versions below 8.0, the planner will ignore your desire to
> choose an index scan if your joining column's datatypes do not
> match

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

On Thu, Mar 22, 2007 at 04:58:09PM -0400, Bruce Momjian wrote:
> Is there a new version of this patch being worked on?

Yes, I will submit a new version next week.

Joachim

I assume this patch is not ready for 8.3, so I added a URL to the TODO
list for it.

---------------------------------------------------------------------------

Tom Lane wrote:
> Joachim Wieland <joe(at)mcknight(dot)de> writes:
> > On Tue, Mar 13, 2007 at 11:52:38AM -0400, Tom Lane wrote:
> >> Why do you need to tell that? IMHO, once the DefineCustomFoo function
> >> has been executed, it should be exactly like any other variable (other
> >> than having a funny name).
>
> > For example for the fall-back-to-default patch. I might not need to tell if
> > it has been introduced by one of the DefineCustomFoo functions but for the
> > "other" custom variables. Imagine that we have defined a custom variable via
> > the configuration file, remove it and send SIGHUP. My understanding is that
> > this variable should then be deleted from the pool of valid variables
> > because it falls back to its default value and the default value of a custom
> > variable is its non-existance.
>
> Once DefineCustomFoo has been executed, you have a reset value to fall
> back to. I think what you really want is to recognize variables that
> are in the placeholder state, and have them go away as above.
> For that you check the GUC_CUSTOM_PLACEHOLDER flag. In any case there
> must never be any use of var->group for decision making; that's simply
> wrong.
>
> However, ISTM that forcing variables to go away is useless extra code.
> What purpose does it serve? Not error checking, because you already
> accepted the variable before. Surely you wouldn't argue that, say,
> reverting to a prior setting of check_function_bodies should cause the
> system to go back and validate a CREATE FUNCTION command it has already
> accepted. Moreover, while you could perhaps argue that the "principle
> of least surprise" cuts either way here, it seems to me there's a good
> argument for not throwing away variables: you might be discarding data
> the user needed. So I'd vote for just leaving them there.
>
> regards, tom lane
>
> ---------------------------(end of broadcast)---------------------------
> TIP 9: In versions below 8.0, the planner will ignore your desire to
> choose an index scan if your joining column's datatypes do not
> match

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

On Mon, Apr 02, 2007 at 07:25:46PM -0400, Bruce Momjian wrote:
> I assume this patch is not ready for 8.3, so I added a URL to the TODO
> list for it.

I have reworked it such that it ignores custom variable templates as Tom
suggested. Attached is the new version.

Joachim

Your patch has been added to the PostgreSQL unapplied patches list at:

http://momjian.postgresql.org/cgi-bin/pgpatches

It will be applied as soon as one of the PostgreSQL committers reviews
and approves it.

---------------------------------------------------------------------------

Joachim Wieland wrote:
> On Mon, Apr 02, 2007 at 07:25:46PM -0400, Bruce Momjian wrote:
> > I assume this patch is not ready for 8.3, so I added a URL to the TODO
> > list for it.
>
> I have reworked it such that it ignores custom variable templates as Tom
> suggested. Attached is the new version.
>
>
> Joachim

[ Attachment, skipping... ]

>
> ---------------------------(end of broadcast)---------------------------
> TIP 2: Don't 'kill -9' the postmaster

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Patch applied by Peter. Thanks.

---------------------------------------------------------------------------

Joachim Wieland wrote:
> On Mon, Apr 02, 2007 at 07:25:46PM -0400, Bruce Momjian wrote:
> > I assume this patch is not ready for 8.3, so I added a URL to the TODO
> > list for it.
>
> I have reworked it such that it ignores custom variable templates as Tom
> suggested. Attached is the new version.
>
>
> Joachim

[ Attachment, skipping... ]

>
> ---------------------------(end of broadcast)---------------------------
> TIP 2: Don't 'kill -9' the postmaster

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +