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.

diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 1dbffbfb4825bdb3eadc46af06c2f373728487e8..bca9e3fc384d2a2f995ca98966693d2995055025 100644 (file)
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -10,7 +10,7 @@
  * Written by Peter Eisentraut <peter_e@gmx.net>.
  *
  * IDENTIFICATION
- *       $PostgreSQL: pgsql/src/backend/utils/misc/guc.c,v 1.432 2008/01/30 18:35:55 tgl Exp $
+ *       $PostgreSQL: pgsql/src/backend/utils/misc/guc.c,v 1.432.2.1 2008/05/26 18:54:36 tgl Exp $
  *
  *--------------------------------------------------------------------
  */
@@ -4370,6 +4370,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, action);
+
                                if (conf->assign_hook)
                                        if (!(*conf->assign_hook) (newval, changeVal, source))
                                        {
@@ -4380,32 +4384,26 @@ set_config_option(const char *name, const char *value,
                                                return false;
                                        }
 
-                               if (changeVal || makeDefault)
+                               if (changeVal)
+                               {
+                                       *conf->variable = newval;
+                                       conf->gen.source = source;
+                               }
+                               if (makeDefault)
                                {
-                                       /* Save old value to support transaction abort */
-                                       if (!makeDefault)
-                                               push_old_value(&conf->gen, action);
-                                       if (changeVal)
+                                       GucStack   *stack;
+
+                                       if (conf->gen.reset_source <= source)
                                        {
-                                               *conf->variable = newval;
-                                               conf->gen.source = source;
+                                               conf->reset_val = newval;
+                                               conf->gen.reset_source = source;
                                        }
-                                       if (makeDefault)
+                                       for (stack = conf->gen.stack; stack; stack = stack->prev)
                                        {
-                                               GucStack   *stack;
-
-                                               if (conf->gen.reset_source <= source)
+                                               if (stack->source <= source)
                                                {
-                                                       conf->reset_val = newval;
-                                                       conf->gen.reset_source = source;
-                                               }
-                                               for (stack = conf->gen.stack; stack; stack = stack->prev)
-                                               {
-                                                       if (stack->source <= source)
-                                                       {
-                                                               stack->prior.boolval = newval;
-                                                               stack->source = source;
-                                                       }
+                                                       stack->prior.boolval = newval;
+                                                       stack->source = source;
                                                }
                                        }
                                }
@@ -4447,6 +4445,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, action);
+
                                if (conf->assign_hook)
                                        if (!(*conf->assign_hook) (newval, changeVal, source))
                                        {
@@ -4457,32 +4459,26 @@ set_config_option(const char *name, const char *value,
                                                return false;
                                        }
 
-                               if (changeVal || makeDefault)
+                               if (changeVal)
+                               {
+                                       *conf->variable = newval;
+                                       conf->gen.source = source;
+                               }
+                               if (makeDefault)
                                {
-                                       /* Save old value to support transaction abort */
-                                       if (!makeDefault)
-                                               push_old_value(&conf->gen, action);
-                                       if (changeVal)
+                                       GucStack   *stack;
+
+                                       if (conf->gen.reset_source <= source)
                                        {
-                                               *conf->variable = newval;
-                                               conf->gen.source = source;
+                                               conf->reset_val = newval;
+                                               conf->gen.reset_source = source;
                                        }
-                                       if (makeDefault)
+                                       for (stack = conf->gen.stack; stack; stack = stack->prev)
                                        {
-                                               GucStack   *stack;
-
-                                               if (conf->gen.reset_source <= source)
+                                               if (stack->source <= source)
                                                {
-                                                       conf->reset_val = newval;
-                                                       conf->gen.reset_source = source;
-                                               }
-                                               for (stack = conf->gen.stack; stack; stack = stack->prev)
-                                               {
-                                                       if (stack->source <= source)
-                                                       {
-                                                               stack->prior.intval = newval;
-                                                               stack->source = source;
-                                                       }
+                                                       stack->prior.intval = newval;
+                                                       stack->source = source;
                                                }
                                        }
                                }
@@ -4521,6 +4517,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, action);
+
                                if (conf->assign_hook)
                                        if (!(*conf->assign_hook) (newval, changeVal, source))
                                        {
@@ -4531,32 +4531,26 @@ set_config_option(const char *name, const char *value,
                                                return false;
                                        }
 
-                               if (changeVal || makeDefault)
+                               if (changeVal)
+                               {
+                                       *conf->variable = newval;
+                                       conf->gen.source = source;
+                               }
+                               if (makeDefault)
                                {
-                                       /* Save old value to support transaction abort */
-                                       if (!makeDefault)
-                                               push_old_value(&conf->gen, action);
-                                       if (changeVal)
+                                       GucStack   *stack;
+
+                                       if (conf->gen.reset_source <= source)
                                        {
-                                               *conf->variable = newval;
-                                               conf->gen.source = source;
+                                               conf->reset_val = newval;
+                                               conf->gen.reset_source = source;
                                        }
-                                       if (makeDefault)
+                                       for (stack = conf->gen.stack; stack; stack = stack->prev)
                                        {
-                                               GucStack   *stack;
-
-                                               if (conf->gen.reset_source <= source)
+                                               if (stack->source <= source)
                                                {
-                                                       conf->reset_val = newval;
-                                                       conf->gen.reset_source = source;
-                                               }
-                                               for (stack = conf->gen.stack; stack; stack = stack->prev)
-                                               {
-                                                       if (stack->source <= source)
-                                                       {
-                                                               stack->prior.realval = newval;
-                                                               stack->source = source;
-                                                       }
+                                                       stack->prior.realval = newval;
+                                                       stack->source = source;
                                                }
                                        }
                                }
@@ -4610,6 +4604,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, action);
+
                                if (conf->assign_hook && newval)
                                {
                                        const char *hookresult;
@@ -4647,40 +4645,32 @@ set_config_option(const char *name, const char *value,
                                        }
                                }
 
-                               if (changeVal || makeDefault)
+                               if (changeVal)
+                               {
+                                       set_string_field(conf, conf->variable, newval);
+                                       conf->gen.source = source;
+                               }
+                               if (makeDefault)
                                {
-                                       /* Save old value to support transaction abort */
-                                       if (!makeDefault)
-                                               push_old_value(&conf->gen, action);
-                                       if (changeVal)
+                                       GucStack   *stack;
+
+                                       if (conf->gen.reset_source <= source)
                                        {
-                                               set_string_field(conf, conf->variable, newval);
-                                               conf->gen.source = source;
+                                               set_string_field(conf, &conf->reset_val, newval);
+                                               conf->gen.reset_source = source;
                                        }
-                                       if (makeDefault)
+                                       for (stack = conf->gen.stack; stack; stack = stack->prev)
                                        {
-                                               GucStack   *stack;
-
-                                               if (conf->gen.reset_source <= source)
+                                               if (stack->source <= source)
                                                {
-                                                       set_string_field(conf, &conf->reset_val, newval);
-                                                       conf->gen.reset_source = source;
-                                               }
-                                               for (stack = conf->gen.stack; stack; stack = stack->prev)
-                                               {
-                                                       if (stack->source <= source)
-                                                       {
-                                                               set_string_field(conf, &stack->prior.stringval,
-                                                                                                newval);
-                                                               stack->source = source;
-                                                       }
+                                                       set_string_field(conf, &stack->prior.stringval,
+                                                                                        newval);
+                                                       stack->source = source;
                                                }
-                                               /* Perhaps we didn't install newval anywhere */
-                                               if (newval && !string_field_used(conf, newval))
-                                                       free(newval);
                                        }
                                }
-                               else if (newval)
+                               /* Perhaps we didn't install newval anywhere */
+                               if (newval && !string_field_used(conf, newval))
                                        free(newval);
                                break;
                        }