fix for new SUSET GUC variables

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

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

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

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