Thread: fix for new SUSET GUC variables

fix for new SUSET GUC variables

From
Bruce Momjian
Date:
I have applied this patch, which I posted previously.

It adds a new GUC context USERLIMIT which prevents certain options from
being turned off or increased, for security.  This fixes problems with
making some options SUSET.

The first part of the patch is the doc part, the second remembers the
proper context for the postgres -d flag, and the bottom handles the new
USERLIMIT context.

--
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073
Index: doc/src/sgml/runtime.sgml
===================================================================
RCS file: /cvsroot/pgsql-server/doc/src/sgml/runtime.sgml,v
retrieving revision 1.190
diff -c -c -r1.190 runtime.sgml
*** doc/src/sgml/runtime.sgml    4 Jul 2003 16:41:21 -0000    1.190
--- doc/src/sgml/runtime.sgml    9 Jul 2003 06:30:03 -0000
***************
*** 1541,1546 ****
--- 1541,1547 ----
          to the log.  The default is <literal>NOTICE</>.  Note that
          <literal>LOG</> has a different rank here than in
          <literal>CLIENT_MIN_MESSAGES</>.
+         Only superusers can increase this option.
         </para>
        </listitem>
       </varlistentry>
***************
*** 1576,1581 ****
--- 1577,1583 ----
          SQL statements causing errors, fatal errors, or panics will be
          logged. Enabling this option can be helpful in tracking down
          the source of any errors that appear in the server log.
+         Only superusers can increase this option.
         </para>
        </listitem>
       </varlistentry>
***************
*** 1593,1598 ****
--- 1595,1602 ----
           than 250ms will be logged.  Enabling this
           option can be useful in tracking down unoptimized queries in
           your applications.
+          Only superusers can increase this option if it is set to
+          non-zero by the administrator.
          </para>
         </listitem>
        </varlistentry>
***************
*** 1743,1748 ****
--- 1747,1754 ----
          To use this option, enable <varname>LOG_STATEMENT</> and
          <varname>LOG_PID</> so you can link the statement to the
          duration using the process ID.
+         Only superusers can turn off this option if it is enabled by
+         the administrator.
         </para>
        </listitem>
       </varlistentry>
***************
*** 1765,1770 ****
--- 1771,1778 ----
        <listitem>
         <para>
          Causes each SQL statement to be logged.
+         Only superusers can turn off this option if it is enabled by
+         the administrator.
         </para>
        </listitem>
       </varlistentry>
***************
*** 1826,1831 ****
--- 1834,1841 ----
          For each query, write performance statistics of the respective
          module to the server log. This is a crude profiling
          instrument.
+         Only superusers can turn off this option if it is enabled by
+         the administrator.
         </para>
        </listitem>
       </varlistentry>
Index: src/backend/tcop/postgres.c
===================================================================
RCS file: /cvsroot/pgsql-server/src/backend/tcop/postgres.c,v
retrieving revision 1.349
diff -c -c -r1.349 postgres.c
*** src/backend/tcop/postgres.c    4 Jul 2003 16:41:21 -0000    1.349
--- src/backend/tcop/postgres.c    9 Jul 2003 06:30:08 -0000
***************
*** 1943,1949 ****
      bool        secure;
      int            errs = 0;
      int            debug_flag = 0;
!     GucContext    ctx;
      GucSource    gucsource;
      char       *tmp;
      int            firstchar;
--- 1943,1949 ----
      bool        secure;
      int            errs = 0;
      int            debug_flag = 0;
!     GucContext    ctx, debug_context;
      GucSource    gucsource;
      char       *tmp;
      int            firstchar;
***************
*** 2018,2024 ****

      /* all options are allowed until '-p' */
      secure = true;
!     ctx = PGC_POSTMASTER;
      gucsource = PGC_S_ARGV;        /* initial switches came from command line */

      while ((flag = getopt(argc, argv, "A:B:c:CD:d:Eef:FiNOPo:p:S:st:v:W:x:-:")) != -1)
--- 2018,2024 ----

      /* all options are allowed until '-p' */
      secure = true;
!     ctx = debug_context = PGC_POSTMASTER;
      gucsource = PGC_S_ARGV;        /* initial switches came from command line */

      while ((flag = getopt(argc, argv, "A:B:c:CD:d:Eef:FiNOPo:p:S:st:v:W:x:-:")) != -1)
***************
*** 2055,2079 ****

              case 'd':            /* debug level */
                  {
!                     debug_flag = atoi(optarg);
!                     /* Set server debugging level. */
!                     if (atoi(optarg) != 0)
                      {
!                         char       *debugstr = palloc(strlen("debug") + strlen(optarg) + 1);
!
!                         sprintf(debugstr, "debug%s", optarg);
!                         SetConfigOption("log_min_messages", debugstr, ctx, gucsource);
!                         pfree(debugstr);
!
                      }
-                     else
-                         /*
-                          * -d0 allows user to prevent postmaster debug
-                          * from propagating to backend.  It would be nice
-                          * to set it to the postgresql.conf value here.
-                          */
-                         SetConfigOption("log_min_messages", "notice",
-                                         ctx, gucsource);
                  }
                  break;

--- 2055,2088 ----

              case 'd':            /* debug level */
                  {
!                     /*
!                      *    Client option can't decrease debug level.
!                      *    We have to do the test here because we group priv and client
!                      *    set GUC calls below, after we know the final debug value.
!                      */
!                     if (ctx != PGC_BACKEND || atoi(optarg) > debug_flag)
                      {
!                         debug_flag = atoi(optarg);
!                         debug_context = ctx;    /* save context for use below */
!                         /* Set server debugging level. */
!                         if (debug_flag != 0)
!                         {
!                             char       *debugstr = palloc(strlen("debug") + strlen(optarg) + 1);
!
!                             sprintf(debugstr, "debug%s", optarg);
!                             SetConfigOption("log_min_messages", debugstr, ctx, gucsource);
!                             pfree(debugstr);
!
!                         }
!                         else
!                             /*
!                              * -d0 allows user to prevent postmaster debug
!                              * from propagating to backend.  It would be nice
!                              * to set it to the postgresql.conf value here.
!                              */
!                             SetConfigOption("log_min_messages", "notice",
!                                             ctx, gucsource);
                      }
                  }
                  break;

***************
*** 2323,2342 ****


      /*
!      * -d is not the same as setting
!      * log_min_messages because it enables other
!      * output options.
       */
      if (debug_flag >= 1)
!         SetConfigOption("log_connections", "true", ctx, gucsource);
      if (debug_flag >= 2)
!         SetConfigOption("log_statement", "true", ctx, gucsource);
      if (debug_flag >= 3)
!         SetConfigOption("debug_print_parse", "true", ctx, gucsource);
      if (debug_flag >= 4)
!         SetConfigOption("debug_print_plan", "true", ctx, gucsource);
      if (debug_flag >= 5)
!         SetConfigOption("debug_print_rewritten", "true", ctx, gucsource);

      /*
       * Process any additional GUC variable settings passed in startup packet.
--- 2332,2350 ----


      /*
!      * -d is not the same as setting log_min_messages because it enables
!      * other output options.
       */
      if (debug_flag >= 1)
!         SetConfigOption("log_connections", "true", debug_context, gucsource);
      if (debug_flag >= 2)
!         SetConfigOption("log_statement", "true", debug_context, gucsource);
      if (debug_flag >= 3)
!         SetConfigOption("debug_print_parse", "true", debug_context, gucsource);
      if (debug_flag >= 4)
!         SetConfigOption("debug_print_plan", "true", debug_context, gucsource);
      if (debug_flag >= 5)
!         SetConfigOption("debug_print_rewritten", "true", debug_context, gucsource);

      /*
       * Process any additional GUC variable settings passed in startup packet.
Index: src/backend/utils/misc/guc.c
===================================================================
RCS file: /cvsroot/pgsql-server/src/backend/utils/misc/guc.c,v
retrieving revision 1.134
diff -c -c -r1.134 guc.c
*** src/backend/utils/misc/guc.c    4 Jul 2003 16:41:21 -0000    1.134
--- src/backend/utils/misc/guc.c    9 Jul 2003 06:30:15 -0000
***************
*** 449,455 ****
          false, NULL, NULL
      },
      {
!         {"log_statement", PGC_SUSET, LOGGING_WHAT,
              gettext_noop("Causes each SQL statement to be logged"),
              NULL
          },
--- 449,455 ----
          false, NULL, NULL
      },
      {
!         {"log_statement", PGC_USERLIMIT, LOGGING_WHAT,
              gettext_noop("Causes each SQL statement to be logged"),
              NULL
          },
***************
*** 457,463 ****
          false, NULL, NULL
      },
      {
!         {"log_duration", PGC_SUSET, LOGGING_WHAT,
              gettext_noop("Duration of every completed statement is logged"),
              NULL
          },
--- 457,463 ----
          false, NULL, NULL
      },
      {
!         {"log_duration", PGC_USERLIMIT, LOGGING_WHAT,
              gettext_noop("Duration of every completed statement is logged"),
              NULL
          },
***************
*** 497,503 ****
          false, NULL, NULL
      },
      {
!         {"log_parser_stats", PGC_SUSET, STATS_MONITORING,
              gettext_noop("Write parser performance stats to server log"),
              NULL
          },
--- 497,503 ----
          false, NULL, NULL
      },
      {
!         {"log_parser_stats", PGC_USERLIMIT, STATS_MONITORING,
              gettext_noop("Write parser performance stats to server log"),
              NULL
          },
***************
*** 505,511 ****
          false, NULL, NULL
      },
      {
!         {"log_planner_stats", PGC_SUSET, STATS_MONITORING,
              gettext_noop("Write planner performance stats to server log"),
              NULL
          },
--- 505,511 ----
          false, NULL, NULL
      },
      {
!         {"log_planner_stats", PGC_USERLIMIT, STATS_MONITORING,
              gettext_noop("Write planner performance stats to server log"),
              NULL
          },
***************
*** 513,519 ****
          false, NULL, NULL
      },
      {
!         {"log_executor_stats", PGC_SUSET, STATS_MONITORING,
              gettext_noop("Write executor performance stats to server log"),
              NULL
          },
--- 513,519 ----
          false, NULL, NULL
      },
      {
!         {"log_executor_stats", PGC_USERLIMIT, STATS_MONITORING,
              gettext_noop("Write executor performance stats to server log"),
              NULL
          },
***************
*** 521,527 ****
          false, NULL, NULL
      },
      {
!         {"log_statement_stats", PGC_SUSET, STATS_MONITORING,
              gettext_noop("Write statement performance stats to server log"),
              NULL
          },
--- 521,527 ----
          false, NULL, NULL
      },
      {
!         {"log_statement_stats", PGC_USERLIMIT, STATS_MONITORING,
              gettext_noop("Write statement performance stats to server log"),
              NULL
          },
***************
*** 1107,1113 ****
      },

      {
!         {"log_min_duration_statement", PGC_SUSET, LOGGING_WHEN,
              gettext_noop("Min execution time (msec) above which statements will "
                           "be logged"),
              gettext_noop("The default is 0 (turning this feature off).")
--- 1107,1113 ----
      },

      {
!         {"log_min_duration_statement", PGC_USERLIMIT, LOGGING_WHEN,
              gettext_noop("Min execution time (msec) above which statements will "
                           "be logged"),
              gettext_noop("The default is 0 (turning this feature off).")
***************
*** 1228,1234 ****
      },

      {
!         {"log_min_messages", PGC_SUSET, LOGGING_WHEN,
              gettext_noop("Controls which message levels logged"),
              gettext_noop("Valid values are DEBUG5, DEBUG4, DEBUG3, DEBUG2, DEBUG1, "
                           "INFO, NOTICE, WARNING, ERROR, LOG, FATAL, and PANIC. Each level "
--- 1228,1234 ----
      },

      {
!         {"log_min_messages", PGC_USERLIMIT, LOGGING_WHEN,
              gettext_noop("Controls which message levels logged"),
              gettext_noop("Valid values are DEBUG5, DEBUG4, DEBUG3, DEBUG2, DEBUG1, "
                           "INFO, NOTICE, WARNING, ERROR, LOG, FATAL, and PANIC. Each level "
***************
*** 1248,1254 ****
      },

      {
!         {"log_min_error_statement", PGC_SUSET, LOGGING_WHEN,
              gettext_noop("Controls whether the erroneous statement is logged"),
              gettext_noop("All SQL statements that cause an error of the "
                           "specified level, or a higher level, are logged")
--- 1248,1254 ----
      },

      {
!         {"log_min_error_statement", PGC_USERLIMIT, LOGGING_WHEN,
              gettext_noop("Controls whether the erroneous statement is logged"),
              gettext_noop("All SQL statements that cause an error of the "
                           "specified level, or a higher level, are logged")
***************
*** 1714,1719 ****
--- 1714,1722 ----

                      Assert(conf->reset_val >= conf->min);
                      Assert(conf->reset_val <= conf->max);
+                     /* Check to make sure we only have valid PGC_USERLIMITs */
+                     Assert(conf->gen.context != PGC_USERLIMIT ||
+                            strcmp(conf->gen.name, "log_min_duration_statement") == 0);
                      if (conf->assign_hook)
                          if (!(*conf->assign_hook) (conf->reset_val, true, false))
                              fprintf(stderr, "Failed to initialize %s to %d\n",
***************
*** 1728,1733 ****
--- 1731,1737 ----

                      Assert(conf->reset_val >= conf->min);
                      Assert(conf->reset_val <= conf->max);
+                     Assert(conf->gen.context != PGC_USERLIMIT);
                      if (conf->assign_hook)
                          if (!(*conf->assign_hook) (conf->reset_val, true, false))
                              fprintf(stderr, "Failed to initialize %s to %g\n",
***************
*** 1741,1746 ****
--- 1745,1755 ----
                      struct config_string *conf = (struct config_string *) gconf;
                      char       *str;

+                     /* Check to make sure we only have valid PGC_USERLIMITs */
+                     Assert(conf->gen.context != PGC_USERLIMIT ||
+                            conf->assign_hook == assign_log_min_messages ||
+                            conf->assign_hook == assign_client_min_messages ||
+                            conf->assign_hook == assign_min_error_statement);
                      *conf->variable = NULL;
                      conf->reset_val = NULL;
                      conf->session_val = NULL;
***************
*** 1837,1843 ****
          struct config_generic *gconf = guc_variables[i];

          /* Don't reset non-SET-able values */
!         if (gconf->context != PGC_SUSET && gconf->context != PGC_USERSET)
              continue;
          /* Don't reset if special exclusion from RESET ALL */
          if (gconf->flags & GUC_NO_RESET_ALL)
--- 1846,1854 ----
          struct config_generic *gconf = guc_variables[i];

          /* Don't reset non-SET-able values */
!         if (gconf->context != PGC_SUSET &&
!             gconf->context != PGC_USERLIMIT &&
!             gconf->context != PGC_USERSET)
              continue;
          /* Don't reset if special exclusion from RESET ALL */
          if (gconf->flags & GUC_NO_RESET_ALL)
***************
*** 2286,2291 ****
--- 2297,2303 ----
      int            elevel;
      bool        interactive;
      bool        makeDefault;
+     bool        DoIt_orig;

      if (context == PGC_SIGHUP || source == PGC_S_DEFAULT)
          elevel = DEBUG2;
***************
*** 2371,2376 ****
--- 2383,2389 ----
                  return false;
              }
              break;
+         case PGC_USERLIMIT:    /* USERLIMIT permissions checked below */
          case PGC_USERSET:
              /* always okay */
              break;
***************
*** 2393,2398 ****
--- 2406,2412 ----
       * to set the reset/session values even if we can't set the variable
       * itself.
       */
+     DoIt_orig = DoIt;    /* we might have to reverse this later */
      if (record->source > source)
      {
          if (DoIt && !makeDefault)
***************
*** 2422,2427 ****
--- 2436,2459 ----
                               name);
                          return false;
                      }
+                     /* Limit non-super user changes */
+                     if (record->context == PGC_USERLIMIT &&
+                         source > PGC_S_USERSTART &&
+                         newval < conf->session_val &&
+                         !superuser())
+                     {
+                         elog(elevel, "'%s': permission denied\n"
+                                 "Only super-users can set this value to false.",
+                                 name);
+                         return false;
+                     }
+                     /* Allow admin to override non-super user setting */
+                     if (record->context == PGC_USERLIMIT &&
+                         source < PGC_S_USERSTART &&
+                         record->session_source > PGC_S_USERSTART &&
+                         newval > conf->session_val &&
+                         !superuser())
+                             DoIt = DoIt_orig;
                  }
                  else
                  {
***************
*** 2493,2498 ****
--- 2525,2549 ----
                               name, newval, conf->min, conf->max);
                          return false;
                      }
+                     /* Limit non-super user changes */
+                     if (record->context == PGC_USERLIMIT &&
+                         source > PGC_S_USERSTART &&
+                         conf->session_val != 0 &&
+                         newval > conf->session_val &&
+                         !superuser())
+                     {
+                         elog(elevel, "'%s': permission denied\n"
+                                 "Only super-users can increase this value.",
+                                 name);
+                         return false;
+                     }
+                     /* Allow admin to override non-super user setting */
+                     if (record->context == PGC_USERLIMIT &&
+                         source < PGC_S_USERSTART &&
+                         record->session_source > PGC_S_USERSTART &&
+                         newval < conf->session_val &&
+                         !superuser())
+                             DoIt = DoIt_orig;
                  }
                  else
                  {
***************
*** 2564,2569 ****
--- 2615,2638 ----
                               name, newval, conf->min, conf->max);
                          return false;
                      }
+                     /* Limit non-super user changes */
+                     if (record->context == PGC_USERLIMIT &&
+                         source > PGC_S_USERSTART &&
+                         newval > conf->session_val &&
+                         !superuser())
+                     {
+                         elog(elevel, "'%s': permission denied\n"
+                                 "Only super-users can increase this value.",
+                                 name);
+                         return false;
+                     }
+                     /* Allow admin to override non-super user setting */
+                     if (record->context == PGC_USERLIMIT &&
+                         source < PGC_S_USERSTART &&
+                         record->session_source > PGC_S_USERSTART &&
+                         newval < conf->session_val &&
+                         !superuser())
+                             DoIt = DoIt_orig;
                  }
                  else
                  {
***************
*** 2628,2633 ****
--- 2697,2728 ----
                          elog(elevel, "out of memory");
                          return false;
                      }
+
+                     if (*conf->variable)
+                     {
+                         int old_int_value, new_int_value;
+
+                         /* Limit non-super user changes */
+                         assign_msglvl(&old_int_value, conf->reset_val, true, interactive);
+                         assign_msglvl(&new_int_value, newval, true, interactive);
+                         if (record->context == PGC_USERLIMIT &&
+                             source > PGC_S_USERSTART &&
+                             new_int_value > old_int_value &&
+                             !superuser())
+                         {
+                             elog(elevel, "'%s': permission denied\n"
+                                         "Only super-users can increase this value.",
+                                         name);
+                             return false;
+                         }
+                         /* Allow admin to override non-super user setting */
+                         if (record->context == PGC_USERLIMIT &&
+                             source < PGC_S_USERSTART &&
+                             record->session_source > PGC_S_USERSTART &&
+                             newval < conf->session_val &&
+                             !superuser())
+                                 DoIt = DoIt_orig;
+                         }
                  }
                  else if (conf->reset_val)
                  {
Index: src/include/utils/guc.h
===================================================================
RCS file: /cvsroot/pgsql-server/src/include/utils/guc.h,v
retrieving revision 1.32
diff -c -c -r1.32 guc.h
*** src/include/utils/guc.h    11 Jun 2003 18:01:14 -0000    1.32
--- src/include/utils/guc.h    9 Jul 2003 06:30:15 -0000
***************
*** 48,53 ****
--- 48,56 ----
   * be set in the connection startup packet, because when it is processed
   * we don't yet know if the user is a superuser.
   *
+  * USERLIMIT options can only be manipulated in certain ways by
+  * non-super users.
+  *
   * USERSET options can be set by anyone any time.
   */
  typedef enum
***************
*** 57,62 ****
--- 60,66 ----
      PGC_SIGHUP,
      PGC_BACKEND,
      PGC_SUSET,
+     PGC_USERLIMIT,
      PGC_USERSET
  } GucContext;

***************
*** 76,86 ****
      PGC_S_ENV_VAR = 1,            /* postmaster environment variable */
      PGC_S_FILE = 2,                /* postgresql.conf */
      PGC_S_ARGV = 3,                /* postmaster command line */
!     PGC_S_DATABASE = 4,            /* per-database setting */
!     PGC_S_USER = 5,                /* per-user setting */
!     PGC_S_CLIENT = 6,            /* from client connection request */
!     PGC_S_OVERRIDE = 7,            /* special case to forcibly set default */
!     PGC_S_SESSION = 8            /* SET command */
  } GucSource;


--- 80,95 ----
      PGC_S_ENV_VAR = 1,            /* postmaster environment variable */
      PGC_S_FILE = 2,                /* postgresql.conf */
      PGC_S_ARGV = 3,                /* postmaster command line */
!     PGC_S_USERSTART=4,            /*
!                                  *    Settings below are controlled by users.
!                                  *    This is used by PGC_USERLIMT to prevent
!                                  *    non-super users from changing certain settings.
!                                  */
!     PGC_S_DATABASE = 5,            /* per-database setting */
!     PGC_S_USER = 6,                /* per-user setting */
!     PGC_S_CLIENT = 7,            /* from client connection request */
!     PGC_S_OVERRIDE = 8,            /* special case to forcibly set default */
!     PGC_S_SESSION = 9            /* SET command */
  } GucSource;



Re: fix for new SUSET GUC variables

From
Aizaz Ahmed
Date:
On Wed, 2003-07-09 at 02:50, Bruce Momjian wrote:
> I have applied this patch, which I posted previously.
>
> It adds a new GUC context USERLIMIT which prevents certain options from
> being turned off or increased, for security.  This fixes problems with
> making some options SUSET.

> ***************
> *** 57,62 ****
> --- 60,66 ----
>        PGC_SIGHUP,
>        PGC_BACKEND,
>        PGC_SUSET,
>+       PGC_USERLIMIT,
>        PGC_USERSET
>  } GucContext;


I believe when updating the GucContext enum, it is also necessary to
update the GucContext_names [] in backend/utils/misc/help_config.c.

The need to do this was supposed to be added as a comment to the guc.h
file, right about where GucContext is defined, but it seems as if that
part of the patch was not applied.

From the original patch "Patch for listing runtime option details
through server executable (pg_guc)", dated "30 Jun 2003 16:43:13 -0400":


Index: src/include/utils/guc.h
===================================================================
RCS file: /projects/cvsroot/pgsql-server/src/include/utils/guc.h,v
retrieving revision 1.32
diff -c -p -r1.32 guc.h
*** src/include/utils/guc.h     11 Jun 2003 18:01:14 -0000      1.32
--- src/include/utils/guc.h     30 Jun 2003 19:18:44 -0000
***************
*** 50,55 ****
--- 50,60 ----
   *
   * USERSET options can be set by anyone any time.
   */
+
+ /*
+  * When updating the GucContexts, please make sure to update the
corresponding
+  * GucContext_names [] entries in pg_guc.c. The two must correspond
+  */
  typedef enum
  {
        PGC_INTERNAL,


This patch was modified before being applied ... was there a reason that
this part of the patch was not applied? One of the modifications made
when applying the patch was to change the names of some of the files ...
in the above excerpt pg_guc.c would have to change to help_config.c.

Thanks,
Aizaz


Re: fix for new SUSET GUC variables

From
Bruce Momjian
Date:
Good catch.  That help file didn't exist when I wrote the original
patch.

Both fixes you mentioned are attached and applied.

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

Aizaz Ahmed wrote:
> On Wed, 2003-07-09 at 02:50, Bruce Momjian wrote:
> > I have applied this patch, which I posted previously.
> >
> > It adds a new GUC context USERLIMIT which prevents certain options from
> > being turned off or increased, for security.  This fixes problems with
> > making some options SUSET.
>
> > ***************
> > *** 57,62 ****
> > --- 60,66 ----
> >        PGC_SIGHUP,
> >        PGC_BACKEND,
> >        PGC_SUSET,
> >+       PGC_USERLIMIT,
> >        PGC_USERSET
> >  } GucContext;
>
>
> I believe when updating the GucContext enum, it is also necessary to
> update the GucContext_names [] in backend/utils/misc/help_config.c.
>
> The need to do this was supposed to be added as a comment to the guc.h
> file, right about where GucContext is defined, but it seems as if that
> part of the patch was not applied.
>
> >From the original patch "Patch for listing runtime option details
> through server executable (pg_guc)", dated "30 Jun 2003 16:43:13 -0400":
>
>
> Index: src/include/utils/guc.h
> ===================================================================
> RCS file: /projects/cvsroot/pgsql-server/src/include/utils/guc.h,v
> retrieving revision 1.32
> diff -c -p -r1.32 guc.h
> *** src/include/utils/guc.h     11 Jun 2003 18:01:14 -0000      1.32
> --- src/include/utils/guc.h     30 Jun 2003 19:18:44 -0000
> ***************
> *** 50,55 ****
> --- 50,60 ----
>    *
>    * USERSET options can be set by anyone any time.
>    */
> +
> + /*
> +  * When updating the GucContexts, please make sure to update the
> corresponding
> +  * GucContext_names [] entries in pg_guc.c. The two must correspond
> +  */
>   typedef enum
>   {
>         PGC_INTERNAL,
>
>
> This patch was modified before being applied ... was there a reason that
> this part of the patch was not applied? One of the modifications made
> when applying the patch was to change the names of some of the files ...
> in the above excerpt pg_guc.c would have to change to help_config.c.
>
> Thanks,
> Aizaz
>
>

--
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073
Index: src/backend/utils/misc/help_config.c
===================================================================
RCS file: /cvsroot/pgsql-server/src/backend/utils/misc/help_config.c,v
retrieving revision 1.1
diff -c -c -r1.1 help_config.c
*** src/backend/utils/misc/help_config.c    4 Jul 2003 16:41:21 -0000    1.1
--- src/backend/utils/misc/help_config.c    9 Jul 2003 17:56:29 -0000
***************
*** 143,148 ****
--- 143,149 ----
      "SIGHUP",
      "BACKEND",
      "SUSET",
+     "USERLIMIT",
      "USERSET"
  };

Index: src/include/utils/guc.h
===================================================================
RCS file: /cvsroot/pgsql-server/src/include/utils/guc.h,v
retrieving revision 1.33
diff -c -c -r1.33 guc.h
*** src/include/utils/guc.h    9 Jul 2003 06:47:34 -0000    1.33
--- src/include/utils/guc.h    9 Jul 2003 17:56:30 -0000
***************
*** 52,57 ****
--- 52,60 ----
   * non-super users.
   *
   * USERSET options can be set by anyone any time.
+  *
+  * When updating the GucContexts, please make sure to update the
+  * corresponding GucContext_names [] entries in pg_guc.c.
   */
  typedef enum
  {

Re: fix for new SUSET GUC variables

From
Tom Lane
Date:
Aizaz Ahmed <aahmed@redhat.com> writes:
> I believe when updating the GucContext enum, it is also necessary to
> update the GucContext_names [] in backend/utils/misc/help_config.c.
> The need to do this was supposed to be added as a comment to the guc.h
> file, right about where GucContext is defined, but it seems as if that
> part of the patch was not applied.

I did not include that comment because it seemed misleading (that array
is far from the only place that has to be adjusted when adding a new
GucContext value) as well as distracting (GucContext_names is not
exactly the most important thing to know about when trying to understand
GucContext).

We don't normally try to enumerate in comments all the places you'd need
to change when adding to an enum or other widely-used definition.  You're
supposed to find them by searching the source code for references to the
existing values.  Depending on comments for that sort of thing is far
too error-prone --- you can just about guarantee that the comment will
fail to track new uses.

The reason Bruce's patch broke the array is that he applied an old patch
without sufficient checking on what had changed in the meantime.
If the comment had been there, it would not have saved him from this
error, since I doubt it would have occurred to him to look for such a
comment.

            regards, tom lane

Re: fix for new SUSET GUC variables

From
Bruce Momjian
Date:
Tom Lane wrote:
> Aizaz Ahmed <aahmed@redhat.com> writes:
> > I believe when updating the GucContext enum, it is also necessary to
> > update the GucContext_names [] in backend/utils/misc/help_config.c.
> > The need to do this was supposed to be added as a comment to the guc.h
> > file, right about where GucContext is defined, but it seems as if that
> > part of the patch was not applied.
>
> I did not include that comment because it seemed misleading (that array
> is far from the only place that has to be adjusted when adding a new
> GucContext value) as well as distracting (GucContext_names is not
> exactly the most important thing to know about when trying to understand
> GucContext).

Comment removed.

--
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073