Thread: pg_settings.enumval as array

The attached patch changes pg_settings.enumval to be an array of text
instead of just a string, per previous discussion and the open items list.

Comments?

//Magnus
*** a/src/backend/utils/misc/guc.c
--- b/src/backend/utils/misc/guc.c
***************
*** 168,174 **** static bool assign_maxconnections(int newval, bool doit, GucSource source);
  static const char *assign_pgstat_temp_directory(const char *newval, bool doit, GucSource source);

  static char *config_enum_get_options(struct config_enum *record,
!                                      const char *prefix, const char *suffix);


  /*
--- 168,175 ----
  static const char *assign_pgstat_temp_directory(const char *newval, bool doit, GucSource source);

  static char *config_enum_get_options(struct config_enum *record,
!                                      const char *prefix, const char *suffix,
!                                      const char *separator);


  /*
***************
*** 4427,4433 **** config_enum_lookup_by_name(struct config_enum *record, const char *value, int *r
   * If suffix is non-NULL, it is added to the end of the string.
   */
  static char *
! config_enum_get_options(struct config_enum *record, const char *prefix, const char *suffix)
  {
      const struct config_enum_entry *entry = record->options;
      int        len = 0;
--- 4428,4435 ----
   * If suffix is non-NULL, it is added to the end of the string.
   */
  static char *
! config_enum_get_options(struct config_enum *record, const char *prefix,
!                         const char *suffix, const char *separator)
  {
      const struct config_enum_entry *entry = record->options;
      int        len = 0;
***************
*** 4439,4445 **** config_enum_get_options(struct config_enum *record, const char *prefix, const ch
      while (entry && entry->name)
      {
          if (!entry->hidden)
!             len += strlen(entry->name) + 2; /* string and ", " */

          entry++;
      }
--- 4441,4447 ----
      while (entry && entry->name)
      {
          if (!entry->hidden)
!             len += strlen(entry->name) + strlen(separator);

          entry++;
      }
***************
*** 4454,4460 **** config_enum_get_options(struct config_enum *record, const char *prefix, const ch
          if (!entry->hidden)
          {
              strcat(hintmsg, entry->name);
!             strcat(hintmsg, ", ");
          }

          entry++;
--- 4456,4462 ----
          if (!entry->hidden)
          {
              strcat(hintmsg, entry->name);
!             strcat(hintmsg, separator);
          }

          entry++;
***************
*** 4469,4484 **** config_enum_get_options(struct config_enum *record, const char *prefix, const ch
       * to make sure we don't write to invalid memory instead of actually
       * trying to do something smart with it.
       */
!     if (len > 1)
!         /* Replace final comma/space */
!         hintmsg[len-2] = '\0';

      strcat(hintmsg, suffix);

      return hintmsg;
  }

-
  /*
   * Call a GucStringAssignHook function, being careful to free the
   * "newval" string if the hook ereports.
--- 4471,4485 ----
       * to make sure we don't write to invalid memory instead of actually
       * trying to do something smart with it.
       */
!     if (len > strlen(separator)-1)
!         /* Replace final separator */
!         hintmsg[len-strlen(separator)] = '\0';

      strcat(hintmsg, suffix);

      return hintmsg;
  }

  /*
   * Call a GucStringAssignHook function, being careful to free the
   * "newval" string if the hook ereports.
***************
*** 5044,5050 **** set_config_option(const char *name, const char *value,
                  {
                      if (!config_enum_lookup_by_name(conf, value, &newval))
                      {
!                         char *hintmsg = config_enum_get_options(conf, "Available values: ", ".");

                          ereport(elevel,
                                  (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
--- 5045,5051 ----
                  {
                      if (!config_enum_lookup_by_name(conf, value, &newval))
                      {
!                         char *hintmsg = config_enum_get_options(conf, "Available values: ", ".", ", ");

                          ereport(elevel,
                                  (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
***************
*** 6249,6255 **** GetConfigOptionByNum(int varnum, const char **values, bool *noshow)
                  values[10] = NULL;

                  /* enumvals */
!                 values[11] = config_enum_get_options((struct config_enum *) conf, "", "");

                   /* boot_val */
                  values[12] = pstrdup(config_enum_lookup_by_value(lconf, lconf->boot_val));
--- 6250,6256 ----
                  values[10] = NULL;

                  /* enumvals */
!                 values[11] = config_enum_get_options((struct config_enum *) conf, "{\"", "\"}", "\",\"");

                   /* boot_val */
                  values[12] = pstrdup(config_enum_lookup_by_value(lconf, lconf->boot_val));
***************
*** 6385,6391 **** show_all_settings(PG_FUNCTION_ARGS)
          TupleDescInitEntry(tupdesc, (AttrNumber) 11, "max_val",
                             TEXTOID, -1, 0);
          TupleDescInitEntry(tupdesc, (AttrNumber) 12, "enumvals",
!                            TEXTOID, -1, 0);
           TupleDescInitEntry(tupdesc, (AttrNumber) 13, "boot_val",
                              TEXTOID, -1, 0);
           TupleDescInitEntry(tupdesc, (AttrNumber) 14, "reset_val",
--- 6386,6392 ----
          TupleDescInitEntry(tupdesc, (AttrNumber) 11, "max_val",
                             TEXTOID, -1, 0);
          TupleDescInitEntry(tupdesc, (AttrNumber) 12, "enumvals",
!                            CSTRINGARRAYOID, -1, 0);
           TupleDescInitEntry(tupdesc, (AttrNumber) 13, "boot_val",
                              TEXTOID, -1, 0);
           TupleDescInitEntry(tupdesc, (AttrNumber) 14, "reset_val",
*** a/src/include/catalog/pg_proc.h
--- b/src/include/catalog/pg_proc.h
***************
*** 3166,3172 **** DATA(insert OID = 2077 (  current_setting    PGNSP PGUID 12 1 0 0 f f t f s 1 25 "2
  DESCR("SHOW X as a function");
  DATA(insert OID = 2078 (  set_config        PGNSP PGUID 12 1 0 0 f f f f v 3 25 "25 25 16" _null_ _null_ _null_
set_config_by_name_null_ _null_ _null_ )); 
  DESCR("SET X as a function");
! DATA(insert OID = 2084 (  pg_show_all_settings    PGNSP PGUID 12 1 1000 0 f f t t s 0 2249 ""
"{25,25,25,25,25,25,25,25,25,25,25,25,25,25,25,23}""{o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o}"
"{name,setting,unit,category,short_desc,extra_desc,context,vartype,source,min_val,max_val,enumvals,boot_val,reset_val,sourcefile,sourceline}"
show_all_settings_null_ _null_ _null_ )); 
  DESCR("SHOW ALL as a function");
  DATA(insert OID = 1371 (  pg_lock_status   PGNSP PGUID 12 1 1000 0 f f t t v 0 2249 ""
"{25,26,26,23,21,25,28,26,26,21,25,23,25,16}""{o,o,o,o,o,o,o,o,o,o,o,o,o,o}"
"{locktype,database,relation,page,tuple,virtualxid,transactionid,classid,objid,objsubid,virtualtransaction,pid,mode,granted}"
pg_lock_status_null_ _null_ _null_ )); 
  DESCR("view system lock information");
--- 3166,3172 ----
  DESCR("SHOW X as a function");
  DATA(insert OID = 2078 (  set_config        PGNSP PGUID 12 1 0 0 f f f f v 3 25 "25 25 16" _null_ _null_ _null_
set_config_by_name_null_ _null_ _null_ )); 
  DESCR("SET X as a function");
! DATA(insert OID = 2084 (  pg_show_all_settings    PGNSP PGUID 12 1 1000 0 f f t t s 0 2249 ""
"{25,25,25,25,25,25,25,25,25,25,25,1263,25,25,25,23}""{o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o}"
"{name,setting,unit,category,short_desc,extra_desc,context,vartype,source,min_val,max_val,enumvals,boot_val,reset_val,sourcefile,sourceline}"
show_all_settings_null_ _null_ _null_ )); 
  DESCR("SHOW ALL as a function");
  DATA(insert OID = 1371 (  pg_lock_status   PGNSP PGUID 12 1 1000 0 f f t t v 0 2249 ""
"{25,26,26,23,21,25,28,26,26,21,25,23,25,16}""{o,o,o,o,o,o,o,o,o,o,o,o,o,o}"
"{locktype,database,relation,page,tuple,virtualxid,transactionid,classid,objid,objsubid,virtualtransaction,pid,mode,granted}"
pg_lock_status_null_ _null_ _null_ )); 
  DESCR("view system lock information");

Magnus Hagander <magnus@hagander.net> writes:
> The attached patch changes pg_settings.enumval to be an array of text
> instead of just a string, per previous discussion and the open items list.

> Comments?

Hmmm ... this coding will fail if any enumval contains a double quote.
Which is probably not a big problem, but we ought to document the
restriction somewhere.

Also, this:

> !     if (len > strlen(separator)-1)
> !         /* Replace final separator */
> !         hintmsg[len-strlen(separator)] = '\0';

would read better IMHO as "if (len >= strlen(separator))".

Also, the output datatype should be text[] not cstring[].

Otherwise seems okay.
        regards, tom lane

Tom Lane wrote:
> Magnus Hagander <magnus@hagander.net> writes:
>> The attached patch changes pg_settings.enumval to be an array of text
>> instead of just a string, per previous discussion and the open items list.
> 
>> Comments?
> 
> Hmmm ... this coding will fail if any enumval contains a double quote.
> Which is probably not a big problem, but we ought to document the
> restriction somewhere.

Hmm. Know what, I had a code comment about that in there. Then I
re-factored my patch and lost it!

I think a code comment is enough. It would actually be quite silly to
*have* a double quote in an enumval - I doubt it will happen by misake.

Will add back.

> Also, this:
> 
>> !     if (len > strlen(separator)-1)
>> !         /* Replace final separator */
>> !         hintmsg[len-strlen(separator)] = '\0';
> 
> would read better IMHO as "if (len >= strlen(separator))".

Pfft, result of too much copy/paste. Changed.


> Also, the output datatype should be text[] not cstring[].


Ok, will change. That requires hardcode of 1009? Or should I a #define
to pg_type.h?

//Magnus

Magnus Hagander <magnus@hagander.net> writes:
> Tom Lane wrote:
>> Also, the output datatype should be text[] not cstring[].

> Ok, will change. That requires hardcode of 1009? Or should I a #define
> to pg_type.h?

Add a #define (bit surprising we didn't have one already)
        regards, tom lane

Tom Lane wrote:
> Magnus Hagander <magnus@hagander.net> writes:
>> Tom Lane wrote:
>>> Also, the output datatype should be text[] not cstring[].
> 
>> Ok, will change. That requires hardcode of 1009? Or should I a #define
>> to pg_type.h?
> 
> Add a #define (bit surprising we didn't have one already)

Yeah, that surprised me too - which is why I had to ask (in case there
was some strange reason we didn't want it).

Changed and applied. Thanks for your comments!

//Magnus