Thread: 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
Attachment
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@momjian.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@gmx.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@gmx.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@sss.pgh.pa.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
Attachment
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@enterprisedb.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/
Attachment
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@enterprisedb.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@mcknight.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@mcknight.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@mcknight.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@mcknight.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@mcknight.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@momjian.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@mcknight.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@momjian.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
Attachment
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@momjian.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@momjian.us> http://momjian.us EnterpriseDB http://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. +