Re: [PATCHES] guc patch: Make variables fall back to default values
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 [EMAIL PROTECTED] http://momjian.us EnterpriseDB http://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. + ---(end of broadcast)--- TIP 3: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
Re: [PATCHES] guc patch: Make variables fall back to default values
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
Index: src/backend/utils/misc/guc-file.l
===
RCS file: /projects/cvsroot/pgsql/src/backend/utils/misc/guc-file.l,v
retrieving revision 1.49
diff -c -r1.49 guc-file.l
*** src/backend/utils/misc/guc-file.l 13 Mar 2007 14:32:25 - 1.49
--- src/backend/utils/misc/guc-file.l 3 Apr 2007 18:56:15 -
***
*** 116,121
--- 116,124
{
int elevel;
struct name_value_pair *item, *head, *tail;
+ int i;
+ bool *in_conffile = NULL;
+ const char *var;
Assert(context == PGC_POSTMASTER || context == PGC_SIGHUP);
***
*** 140,158
/* Check if all options are valid */
for (item = head; item; item = item-next)
{
if (!set_config_option(item-name, item-value, context,
PGC_S_FILE, false, false))
goto cleanup_list;
}
! /* If we got here all the options checked out okay, so apply them. */
for (item = head; item; item = item-next)
{
set_config_option(item-name, item-value, context,
PGC_S_FILE, false, true);
! }
cleanup_list:
free_name_value_list(head);
}
--- 143,300
/* Check if all options are valid */
for (item = head; item; item = item-next)
{
+ char *sep = strchr(item-name, GUC_QUALIFIER_SEPARATOR);
+ if (sep !is_custom_class(item-name, sep - item-name))
+ {
+ ereport(elevel,
+ (errcode(ERRCODE_UNDEFINED_OBJECT),
+ errmsg(unrecognized configuration parameter \%s\,
+ item-name)));
+ goto cleanup_list;
+ }
+
if (!set_config_option(item-name, item-value, context,
PGC_S_FILE, false, false))
goto cleanup_list;
}
!
! /*
! * Mark all variables as not showing up in the config file. The
! * allocation has to take place after ParseConfigFile() since this
! * function can change num_guc_variables due to custom variables.
! * It would be easier to add a new field or status bit to struct
! * conf_generic, but that way we would expose internal information
! * that is just needed here in the following few lines. The price
! * to pay for this separation are a few more loops over the set of
! * configuration options, but those are expected to be rather few
! * and we only have to pay the cost at SIGHUP. We initialize
! * in_conffile only here because set_config_option() makes
! * guc_variables grow with custom variables.
! */
! in_conffile = guc_malloc(elevel, num_guc_variables * sizeof(bool));
! if (!in_conffile)
! goto cleanup_list;
! for (i = 0; i num_guc_variables; i++)
! in_conffile[i] = false;
!
for (item = head; item; item = item-next)
{
+ /*
+ * After set_config_option() the variable name item-name is
+ * known to exist.
+ */
+ Assert(guc_get_index(item-name) = 0);
+ in_conffile[guc_get_index(item-name)] = true;
+ }
+
+ for (i = 0; i num_guc_variables; i++)
+ {
+ struct config_generic *gconf = guc_variables[i];
+ if (!in_conffile[i] gconf-source == PGC_S_FILE)
+ {
+ if (gconf-context PGC_SIGHUP)
+ ereport(elevel,
+ (errcode(ERRCODE_CANT_CHANGE_RUNTIME_PARAM),
+ errmsg(parameter \%s\ cannot be changed after server start; configuration file change ignored,
+ gconf-name)));
+ else
+ {
+ /* prepare */
+ GucStack *stack;
+ if (gconf-reset_source == PGC_S_FILE)
+ gconf-reset_source = PGC_S_DEFAULT;
+ for (stack = gconf-stack; stack; stack = stack-prev)
+ if (stack-source == PGC_S_FILE)
+ stack-source = PGC_S_DEFAULT;
+ /* apply the default */
+ set_config_option(gconf-name, NULL, context,
+ PGC_S_DEFAULT, false, true);
+ }
+ }
+ else if (!in_conffile[i] gconf-reset_source == PGC_S_FILE)
+ {
+ /*--
+ * Change the reset_val to default_val. Here's an
+ * example: In the configuration file we have
+ *
+ * seq_page_cost = 3.00
+ *
+ * Now we execute in a session
+ *
+ * SET seq_page_cost TO 4.00;
+ *
+ * Then we remove this option from the configuration file
+ * and send SIGHUP. Now when you execute
+ *
+ * RESET seq_page_cost;
+ *
+ * it should fall back to 1.00 (the default value for
+ * seq_page_cost) and not to 3.00 (which is the current
+ * reset_val).
+ */
+
+ switch (gconf-vartype)
+ {
+ case PGC_BOOL:
+ {
+ struct config_bool *conf;
+ conf = (struct config_bool *) gconf;
+ conf-reset_val = conf-boot_val;
+ break;
+ }
+ case PGC_INT:
+ {
+ struct config_int *conf;
+ conf = (struct config_int *) gconf;
+ conf-reset_val = conf-boot_val;
+ break;
+ }
+ case
Re: [PATCHES] guc patch: Make variables fall back to default values
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 [EMAIL PROTECTED] http://momjian.us EnterpriseDB http://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. + ---(end of broadcast)--- TIP 7: You can help support the PostgreSQL project by donating at http://www.postgresql.org/about/donate
Re: [PATCHES] guc patch: Make variables fall back to default values
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 [EMAIL PROTECTED] 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 [EMAIL PROTECTED] http://momjian.us EnterpriseDB http://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. + ---(end of broadcast)--- TIP 7: You can help support the PostgreSQL project by donating at http://www.postgresql.org/about/donate
Re: [PATCHES] guc patch: Make variables fall back to default values
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 ---(end of broadcast)--- TIP 5: don't forget to increase your free space map settings
Re: [PATCHES] guc patch: Make variables fall back to default values
Peter Eisentraut [EMAIL PROTECTED] 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 ---(end of broadcast)--- TIP 4: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] guc patch: Make variables fall back to default values
Tom Lane [EMAIL PROTECTED] 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 ---(end of broadcast)--- TIP 3: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
Re: [PATCHES] guc patch: Make variables fall back to default values
On Tue, Mar 13, 2007 at 08:22:17AM +, 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
Index: src/backend/utils/misc/guc.c
===
RCS file: /projects/cvsroot/pgsql/src/backend/utils/misc/guc.c,v
retrieving revision 1.380
diff -c -r1.380 guc.c
*** src/backend/utils/misc/guc.c 12 Mar 2007 22:09:28 - 1.380
--- src/backend/utils/misc/guc.c 13 Mar 2007 10:03:26 -
***
*** 2658,2678
gen = guc_variables[idx];
- /*
- * Even though this function could delete other types of variables as well,
- * at the moment we only call it for custom variables that always have type
- * string.
- */
Assert(gen-group == CUSTOM_OPTIONS);
- Assert(gen-vartype == PGC_STRING);
! conf = (struct config_string *) gen;
! set_string_field(conf, conf-reset_val, NULL);
! set_string_field(conf, conf-tentative_val, NULL);
! for (stack = conf-gen.stack; stack; stack = prev)
{
! set_string_field(conf, stack-tentative_val.stringval, NULL);
! set_string_field(conf, stack-value.stringval, NULL);
prev = stack-prev;
pfree(stack);
}
--- 2658,2678
gen = guc_variables[idx];
Assert(gen-group == CUSTOM_OPTIONS);
! if (gen-vartype == PGC_STRING)
! {
! conf = (struct config_string *) gen;
! set_string_field(conf, conf-reset_val, NULL);
! set_string_field(conf, conf-tentative_val, NULL);
! }
! for (stack = gen-stack; stack; stack = prev)
{
! if (gen-vartype == PGC_STRING)
! {
! set_string_field(conf, stack-tentative_val.stringval, NULL);
! set_string_field(conf, stack-value.stringval, NULL);
! }
prev = stack-prev;
pfree(stack);
}
***
*** 2698,2706
gen = guc_variables[idx];
Assert(gen-group == CUSTOM_OPTIONS);
- Assert(gen-vartype == PGC_STRING);
-
- conf = (struct config_string *) gen;
/*
* Here we check whether it is safe to really delete the variable
--- 2698,2703
***
*** 2723,2731
* then been deleted from the configuration file should behave
* as if it had been introduced in the session.
*/
- Assert(gen-vartype == PGC_STRING);
gen-reset_source = PGC_S_DEFAULT;
! set_string_field(conf, conf-reset_val, NULL);
}
else
guc_delete_variable(name);
--- 2720,2731
* then been deleted from the configuration file should behave
* as if it had been introduced in the session.
*/
gen-reset_source = PGC_S_DEFAULT;
! if (gen-vartype == PGC_STRING)
! {
! conf = (struct config_string *) gen;
! set_string_field(conf, conf-reset_val, NULL);
! }
}
else
guc_delete_variable(name);
---(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
Re: [PATCHES] guc patch: Make variables fall back to default values
Gregory Stark [EMAIL PROTECTED] 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 ---(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
Re: [PATCHES] guc patch: Make variables fall back to default values
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/ ---(end of broadcast)--- TIP 5: don't forget to increase your free space map settings
Re: [PATCHES] guc patch: Make variables fall back to default values
On Tue, Mar 13, 2007 at 10:19:54AM -0400, Tom Lane wrote: Gregory Stark [EMAIL PROTECTED] 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 ---(end of broadcast)--- TIP 7: You can help support the PostgreSQL project by donating at http://www.postgresql.org/about/donate
Re: [PATCHES] guc patch: Make variables fall back to default values
Joachim Wieland [EMAIL PROTECTED] 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 ---(end of broadcast)--- TIP 1: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly
Re: [PATCHES] guc patch: Make variables fall back to default values
On Tue, Mar 13, 2007 at 11:08:52AM -0400, Tom Lane wrote: Joachim Wieland [EMAIL PROTECTED] 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 ---(end of broadcast)--- TIP 1: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly
Re: [PATCHES] guc patch: Make variables fall back to default values
Joachim Wieland [EMAIL PROTECTED] 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 ---(end of broadcast)--- TIP 2: Don't 'kill -9' the postmaster
Re: [PATCHES] guc patch: Make variables fall back to default values
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 ---(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
Re: [PATCHES] guc patch: Make variables fall back to default values
Joachim Wieland [EMAIL PROTECTED] 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
Re: [PATCHES] guc patch: Make variables fall back to default values
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/ ---(end of broadcast)--- TIP 1: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly
Re: [PATCHES] guc patch: Make variables fall back to default values
Peter Eisentraut [EMAIL PROTECTED] 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 ---(end of broadcast)--- TIP 2: Don't 'kill -9' the postmaster
Re: [PATCHES] guc patch: Make variables fall back to default values
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/ ---(end of broadcast)--- TIP 3: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
Re: [PATCHES] guc patch: Make variables fall back to default values
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 ---(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
Re: [PATCHES] guc patch: Make variables fall back to default values
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 [EMAIL PROTECTED] http://momjian.us EnterpriseDB http://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. + ---(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
[PATCHES] 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
Index: src/backend/access/transam/xact.c
===
RCS file: /projects/cvsroot/pgsql/src/backend/access/transam/xact.c,v
retrieving revision 1.234
diff -c -r1.234 xact.c
*** src/backend/access/transam/xact.c 9 Feb 2007 03:35:33 - 1.234
--- src/backend/access/transam/xact.c 19 Feb 2007 12:44:31 -
***
*** 1514,1519
--- 1514,1537
AtCommit_Notify();
/*
+ * GUC can abort the transaction in exactly one case, namely when you
+ * delete a custom variable class while a still-open transaction has
+ * SET a custom variable within this class.
+ *
+ * Consider the following example. In the configuration file we could have:
+ * custom_variable_classes = foo
+ *
+ * begin;
+ * set foo.bar to 1;
+ * delete foo.bar from configuration file and send SIGHUP
+ * commit;
+ *
+ * This will result in an error because foo.bar is no longer available
+ * but commit would have to guarantee that the value is preserved.
+ */
+ AtEOXact_GUC(true, false);
+
+ /*
* Update flat files if we changed pg_database, pg_authid or
* pg_auth_members. This should be the last step before commit.
*/
***
*** 1623,1629
/* Check we've released all catcache entries */
AtEOXact_CatCache(true);
- AtEOXact_GUC(true, false);
AtEOXact_SPI(true);
AtEOXact_on_commit_actions(true);
AtEOXact_Namespace(true);
--- 1641,1646
Index: src/backend/utils/misc/guc-file.l
===
RCS file: /projects/cvsroot/pgsql/src/backend/utils/misc/guc-file.l,v
retrieving revision 1.47
diff -c -r1.47 guc-file.l
*** src/backend/utils/misc/guc-file.l 10 Feb 2007 14:58:55 - 1.47
--- src/backend/utils/misc/guc-file.l 19 Feb 2007 12:44:59 -
***
*** 54,59
--- 54,61
static void free_name_value_list(struct name_value_pair * list);
static char *GUC_scanstr(const char *s);
+ static char *custom_variable_classes_backup;
+
%}
%option 8bit
***
*** 114,121
void
ProcessConfigFile(GucContext context)
{
! int elevel;
struct name_value_pair *item, *head, *tail;
Assert(context == PGC_POSTMASTER || context == PGC_SIGHUP);
--- 116,126
void
ProcessConfigFile(GucContext context)
{
! int elevel, i;
struct name_value_pair *item, *head, *tail;
+ bool *in_conffile = NULL;
+ const char *var;
+ bool success = false;
Assert(context == PGC_POSTMASTER || context == PGC_SIGHUP);
***
*** 132,137
--- 137,153
head = tail = NULL;
+ /* We do not know if we will read the configuration file
+ * without error. If we encounter an error we have to restore the
+ * previous value of the custom_variable_classes variable for
+ * consistency. Hence we have to save its value.
+ */
+ var = GetConfigOption(custom_variable_classes);
+ if (var)
+ custom_variable_classes_backup = pstrdup(var);
+ set_config_option(custom_variable_classes, NULL, context,
+
