From fd97d48c85132c6b2acc07805009ff81bb76dbd3 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Mon, 26 May 2008 18:54:43 +0000 Subject: [PATCH] Fix an old corner-case bug in set_config_option: push_old_value has to be called before, not after, calling the assign_hook if any. This is because push_old_value might fail (due to palloc out-of-memory), and in that case there would be no stack entry to tell transaction abort to undo the GUC assignment. Of course the actual assignment to the GUC variable hasn't happened yet --- but the assign_hook might have altered subsidiary state. Without a stack entry we won't call it again to make it undo such actions. So this is necessary to make the world safe for assign_hooks with side effects. Per a discussion a couple weeks ago with Magnus. Back-patch to 8.0. 7.x did not have the problem because it did not have allocatable stacks of GUC values. --- src/backend/utils/misc/guc.c | 30 +++++++++++++++++------------- 1 file changed, 17 insertions(+), 13 deletions(-) diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index 656063321ef..98e682b009a 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -10,7 +10,7 @@ * Written by Peter Eisentraut . * * IDENTIFICATION - * $PostgreSQL: pgsql/src/backend/utils/misc/guc.c,v 1.360.2.1 2007/04/23 15:13:30 neilc Exp $ + * $PostgreSQL: pgsql/src/backend/utils/misc/guc.c,v 1.360.2.2 2008/05/26 18:54:43 tgl Exp $ * *-------------------------------------------------------------------- */ @@ -3982,6 +3982,10 @@ set_config_option(const char *name, const char *value, source = conf->gen.reset_source; } + /* Save old value to support transaction abort */ + if (changeVal && !makeDefault) + push_old_value(&conf->gen); + if (conf->assign_hook) if (!(*conf->assign_hook) (newval, changeVal, source)) { @@ -3994,9 +3998,6 @@ set_config_option(const char *name, const char *value, if (changeVal || makeDefault) { - /* Save old value to support transaction abort */ - if (!makeDefault) - push_old_value(&conf->gen); if (changeVal) { *conf->variable = newval; @@ -4066,6 +4067,10 @@ set_config_option(const char *name, const char *value, source = conf->gen.reset_source; } + /* Save old value to support transaction abort */ + if (changeVal && !makeDefault) + push_old_value(&conf->gen); + if (conf->assign_hook) if (!(*conf->assign_hook) (newval, changeVal, source)) { @@ -4078,9 +4083,6 @@ set_config_option(const char *name, const char *value, if (changeVal || makeDefault) { - /* Save old value to support transaction abort */ - if (!makeDefault) - push_old_value(&conf->gen); if (changeVal) { *conf->variable = newval; @@ -4150,6 +4152,10 @@ set_config_option(const char *name, const char *value, source = conf->gen.reset_source; } + /* Save old value to support transaction abort */ + if (changeVal && !makeDefault) + push_old_value(&conf->gen); + if (conf->assign_hook) if (!(*conf->assign_hook) (newval, changeVal, source)) { @@ -4162,9 +4168,6 @@ set_config_option(const char *name, const char *value, if (changeVal || makeDefault) { - /* Save old value to support transaction abort */ - if (!makeDefault) - push_old_value(&conf->gen); if (changeVal) { *conf->variable = newval; @@ -4239,6 +4242,10 @@ set_config_option(const char *name, const char *value, break; } + /* Save old value to support transaction abort */ + if (changeVal && !makeDefault) + push_old_value(&conf->gen); + if (conf->assign_hook) { const char *hookresult; @@ -4278,9 +4285,6 @@ set_config_option(const char *name, const char *value, if (changeVal || makeDefault) { - /* Save old value to support transaction abort */ - if (!makeDefault) - push_old_value(&conf->gen); if (changeVal) { set_string_field(conf, conf->variable, newval);