Re: pg_get_functiondef forgets about most GUC_LIST_INPUT GUCs - Mailing list pgsql-hackers

At Mon, 19 Mar 2018 23:07:13 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote in <10037.1521515233@sss.pgh.pa.us>
> Michael Paquier <michael@paquier.xyz> writes:
> > On Mon, Mar 19, 2018 at 07:15:36PM -0400, Tom Lane wrote:
> >> This is a good thing not least because all the GUC_LIST_QUOTE variables
> >> are in core.  I've been trying to think of a way that we could have
> >> correct behavior for variables that the core backend doesn't know about
> >> and whose extension shlibs aren't currently loaded, and I can't come up
> >> with one.  So I think that in practice, we have to document that
> >> GUC_LIST_QUOTE is unsupported for extension variables
> 
> > I would propose to add a sentence on the matter at the bottom of the
> > CREATE FUNCTION page.  Even with that, the handling of items in the list
> > is incorrect with any patches on this thread: the double quotes should
> > be applied to each element of the list, not the whole list.
> 
> No, because all the places we are worried about are interpreting the
> contents of proconfig or setconfig columns, and we know that that info
> has been through flatten_set_variable_args().  If that function thought
> that GUC_LIST_QUOTE was applicable, it already did the appropriate
> quoting, and we can't quote over that without making a mess.  But if it
> did not think that GUC_LIST_QUOTE was applicable, then its output can
> just be treated as a single string, and that will work fine.
> 
> Our problem basically is that if we don't know the variable that was
> being processed, we can't be sure whether GUC_LIST_QUOTE was used.
> I don't currently see a solution to that other than insisting that
> GUC_LIST_QUOTE only be used for known core GUCs.

The documentation of SET command says as the follows. (CREATE
FUNCTION says a bit different but seems working in the same way)

https://www.postgresql.org/docs/10/static/sql-set.html

> SET [ SESSION | LOCAL ] configuration_parameter { TO | = } { value | 'value' | DEFAULT }
and

> value
> 
>   New value of parameter. Values can be specified as string
>   constants, identifiers, numbers, or comma-separated lists of
>   these, as appropriate for the particular parameter. DEFAULT can
>   be written to specify resetting the parameter to its default
>   value (that is, whatever value it would have had if no SET had
>   been executed in the current session).

According to this description, a comma-separated list enclosed
with single quotes is a valid list value. (I remenber I was
trapped by the description.)

I should be stupid but couldn't we treat quoted comma-separated
list as a bare list if it is the only value in the argument? I
think no one sets such values containing commas as
temp_tablespaces, *_preload_libraries nor search_path.

But of course it may break something and may break some
extensions.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
*** a/src/backend/utils/misc/guc.c
--- b/src/backend/utils/misc/guc.c
***************
*** 6859,6864 **** GetConfigOptionResetString(const char *name)
--- 6859,6891 ----
      return NULL;
  }
  
+ /*
+  * quote_list_identifiers
+  *        quote each element in the given list string
+  */
+ static const char *
+ quote_list_identifiers(char *str)
+ {
+     StringInfoData buf;
+     List           *namelist;
+     ListCell       *lc;
+ 
+     /* Just quote the whole if this is not a list */
+     if (!SplitIdentifierString(str, ',', &namelist))
+         return quote_identifier(str);
+ 
+     initStringInfo(&buf);
+     foreach (lc, namelist)
+     {
+         char *elmstr = (char *) lfirst(lc);
+ 
+         if (lc != list_head(namelist))
+             appendStringInfoString(&buf, ", ");
+         appendStringInfoString(&buf, quote_identifier(elmstr));
+     }
+ 
+     return buf.data;
+ }
  
  /*
   * flatten_set_variable_args
***************
*** 6973,6979 **** flatten_set_variable_args(const char *name, List *args)
                       * quote it if it's not a vanilla identifier.
                       */
                      if (flags & GUC_LIST_QUOTE)
!                         appendStringInfoString(&buf, quote_identifier(val));
                      else
                          appendStringInfoString(&buf, val);
                  }
--- 7000,7018 ----
                       * quote it if it's not a vanilla identifier.
                       */
                      if (flags & GUC_LIST_QUOTE)
!                     {
!                         if (list_length(args) > 1)
!                             appendStringInfoString(&buf, quote_identifier(val));
!                         else
!                         {
!                             /*
!                              * If we have only one elment, it may be a list
!                              * that is quoted as a whole.
!                              */
!                             appendStringInfoString(&buf,
!                                                    quote_list_identifiers(val));
!                         }
!                     }
                      else
                          appendStringInfoString(&buf, val);
                  }