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

Hello,

At Wed, 14 Mar 2018 17:46:21 +0900, Michael Paquier <michael@paquier.xyz> wrote in <20180314084621.GA617@paquier.xyz>
> On Wed, Mar 14, 2018 at 05:30:59PM +0900, Kyotaro HORIGUCHI wrote:
> > Doesn't it make sense if we provide a buildtime-script that
> > collects the function names and builds a .h file containing a
> > function using the list?
> > 
> > The attached perl script is a rush work of such script, which
> > works at the top of the source tree. It just prints the function
> > definition, does not generate a .h file.
> > 
> > I haven't confirmed anything about it but I had the following
> > output from the current master.
> 
> I quite like that idea actually.  Please note that pl_handler.c declares
> two more of them, so we may want a smarter parsing which takes into
> account declarations of DefineCustom*Variable.

Only DefineCustomStringVariable can accept a list argument even
if flagged GUC_LIST_INPUT. (I'm not sure this should be
explicitly rejected.) I'm not sure this is smart enough but it
works. It would fail if description for variables contain the
same character sequence to the lexical structure around but it is
rare to happen.

The attached patch is the result. It adds a script to generate
include/common/check_listvars.h. It won't be updated even if
related files are modifed (since we cannot know it before
scanning the whole source.) but "make clean" removes it. Regtests
is a copy of Michael's v1 patch.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
From 17cadb17279c802c79e4afa0d7fcd7f63ed38d03 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horiguchi.kyotaro@lab.ntt.co.jp>
Date: Thu, 15 Mar 2018 13:12:17 +0900
Subject: [PATCH] Autogenerating function to detect list-formatted GUC
 variables

Some features require to know whether a GUC variables is list or not
but the set of such functions can be modified through development. We
tried to check it on-the-fly but found that that makes things more
complex. Instead, this generates the list of such variables by
scanning the whole source tree, instead of manually maintaining it.
---
 src/backend/Makefile                |   4 +-
 src/backend/utils/adt/ruleutils.c   |   4 +-
 src/bin/pg_dump/Makefile            |   4 +-
 src/bin/pg_dump/dumputils.c         |   5 +-
 src/bin/pg_dump/pg_dump.c           |   4 +-
 src/include/Makefile                |   7 +-
 src/include/gen_listvars.pl         | 145 ++++++++++++++++++++++++++++++++++++
 src/test/regress/expected/rules.out |  24 ++++++
 src/test/regress/sql/rules.sql      |  12 +++
 9 files changed, 197 insertions(+), 12 deletions(-)
 create mode 100755 src/include/gen_listvars.pl

diff --git a/src/backend/Makefile b/src/backend/Makefile
index 4a28267339..10656c53e6 100644
--- a/src/backend/Makefile
+++ b/src/backend/Makefile
@@ -169,7 +169,7 @@ submake-schemapg:
 
 .PHONY: generated-headers
 
-generated-headers: $(top_builddir)/src/include/parser/gram.h $(top_builddir)/src/include/catalog/schemapg.h
$(top_builddir)/src/include/storage/lwlocknames.h$(top_builddir)/src/include/utils/errcodes.h
$(top_builddir)/src/include/utils/fmgroids.h$(top_builddir)/src/include/utils/fmgrprotos.h
$(top_builddir)/src/include/utils/probes.h
+generated-headers: $(top_builddir)/src/include/parser/gram.h $(top_builddir)/src/include/catalog/schemapg.h
$(top_builddir)/src/include/storage/lwlocknames.h$(top_builddir)/src/include/utils/errcodes.h
$(top_builddir)/src/include/utils/fmgroids.h$(top_builddir)/src/include/utils/fmgrprotos.h
$(top_builddir)/src/include/utils/probes.h$(top_builddir)/src/include/common/check_listvars.h
 
 
 $(top_builddir)/src/include/parser/gram.h: parser/gram.h
     prereqdir=`cd '$(dir $<)' >/dev/null && pwd` && \
@@ -205,6 +205,8 @@ $(top_builddir)/src/include/utils/probes.h: utils/probes.h
     cd '$(dir $@)' && rm -f $(notdir $@) && \
         $(LN_S) "../../../$(subdir)/utils/probes.h" .
 
+$(top_builddir)/src/include/common/check_listvars.h:
+    $(MAKE) -C $(dir $@).. common/check_listvars.h
 
 utils/probes.o: utils/probes.d $(SUBDIROBJS)
     $(DTRACE) $(DTRACEFLAGS) -C -G -s $(call expand_subsys,$^) -o $@
diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index b58ee3c387..d2ad034e06 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -41,6 +41,7 @@
 #include "catalog/pg_type.h"
 #include "commands/defrem.h"
 #include "commands/tablespace.h"
+#include "common/check_listvars.h"
 #include "common/keywords.h"
 #include "executor/spi.h"
 #include "funcapi.h"
@@ -2599,8 +2600,7 @@ pg_get_functiondef(PG_FUNCTION_ARGS)
                  * Some GUC variable names are 'LIST' type and hence must not
                  * be quoted.
                  */
-                if (pg_strcasecmp(configitem, "DateStyle") == 0
-                    || pg_strcasecmp(configitem, "search_path") == 0)
+                if (IsConfigOptionAList(configitem))
                     appendStringInfoString(&buf, pos);
                 else
                     simple_quote_literal(&buf, pos);
diff --git a/src/bin/pg_dump/Makefile b/src/bin/pg_dump/Makefile
index e3bfc95f16..d79ac49a83 100644
--- a/src/bin/pg_dump/Makefile
+++ b/src/bin/pg_dump/Makefile
@@ -25,13 +25,13 @@ OBJS=    pg_backup_archiver.o pg_backup_db.o pg_backup_custom.o \
 
 all: pg_dump pg_restore pg_dumpall
 
-pg_dump: pg_dump.o common.o pg_dump_sort.o $(OBJS) | submake-libpq submake-libpgport submake-libpgfeutils
+pg_dump: pg_dump.o common.o pg_dump_sort.o $(OBJS) | submake-libpq submake-libpgport submake-libpgfeutils
submake-generated-headers
     $(CC) $(CFLAGS) pg_dump.o common.o pg_dump_sort.o $(OBJS) $(LDFLAGS) $(LDFLAGS_EX) $(LIBS) -o $@$(X)
 
 pg_restore: pg_restore.o $(OBJS) | submake-libpq submake-libpgport submake-libpgfeutils
     $(CC) $(CFLAGS) pg_restore.o $(OBJS) $(LDFLAGS) $(LDFLAGS_EX) $(LIBS) -o $@$(X)
 
-pg_dumpall: pg_dumpall.o dumputils.o | submake-libpq submake-libpgport submake-libpgfeutils
+pg_dumpall: pg_dumpall.o dumputils.o | submake-libpq submake-libpgport submake-libpgfeutils submake-generated-headers
     $(CC) $(CFLAGS) pg_dumpall.o dumputils.o $(WIN32RES) $(LDFLAGS) $(LDFLAGS_EX) $(LIBS) -o $@$(X)
 
 install: all installdirs
diff --git a/src/bin/pg_dump/dumputils.c b/src/bin/pg_dump/dumputils.c
index 7f5bb1343e..0c4d648969 100644
--- a/src/bin/pg_dump/dumputils.c
+++ b/src/bin/pg_dump/dumputils.c
@@ -14,6 +14,7 @@
  */
 #include "postgres_fe.h"
 
+#include "common/check_listvars.h"
 #include "dumputils.h"
 #include "fe_utils/string_utils.h"
 
@@ -888,10 +889,8 @@ makeAlterConfigCommand(PGconn *conn, const char *configitem,
 
     /*
      * Some GUC variable names are 'LIST' type and hence must not be quoted.
-     * XXX this list is incomplete ...
      */
-    if (pg_strcasecmp(mine, "DateStyle") == 0
-        || pg_strcasecmp(mine, "search_path") == 0)
+    if (IsConfigOptionAList(mine))
         appendPQExpBufferStr(buf, pos);
     else
         appendStringLiteralConn(buf, pos, conn);
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 566cbf2cda..6446f0ced7 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -53,6 +53,7 @@
 #include "catalog/pg_proc.h"
 #include "catalog/pg_trigger.h"
 #include "catalog/pg_type.h"
+#include "common/check_listvars.h"
 #include "libpq/libpq-fs.h"
 
 #include "dumputils.h"
@@ -11880,8 +11881,7 @@ dumpFunc(Archive *fout, FuncInfo *finfo)
          * Some GUC variable names are 'LIST' type and hence must not be
          * quoted.
          */
-        if (pg_strcasecmp(configitem, "DateStyle") == 0
-            || pg_strcasecmp(configitem, "search_path") == 0)
+        if (IsConfigOptionAList(configitem))
             appendPQExpBufferStr(q, pos);
         else
             appendStringLiteralAH(q, pos, fout);
diff --git a/src/include/Makefile b/src/include/Makefile
index a689d352b6..a05f4ca197 100644
--- a/src/include/Makefile
+++ b/src/include/Makefile
@@ -13,7 +13,7 @@ top_builddir = ../..
 include $(top_builddir)/src/Makefile.global
 
 
-all: pg_config.h pg_config_ext.h pg_config_os.h
+all: pg_config.h pg_config_ext.h pg_config_os.h common/check_listvars.h
 
 
 # Subdirectories containing installable headers
@@ -25,6 +25,9 @@ SUBDIRS = access bootstrap catalog commands common datatype \
     port/win32_msvc/sys port/win32/arpa port/win32/netinet \
     port/win32/sys portability
 
+common/check_listvars.h:
+    ./gen_listvars.pl
+
 # Install all headers
 install: all installdirs
 # These headers are needed by the public headers of the interfaces.
@@ -73,7 +76,7 @@ uninstall:
 
 
 clean:
-    rm -f utils/fmgroids.h utils/fmgrprotos.h utils/errcodes.h parser/gram.h utils/probes.h catalog/schemapg.h
+    rm -f utils/fmgroids.h utils/fmgrprotos.h utils/errcodes.h parser/gram.h utils/probes.h catalog/schemapg.h
common/check_listvars.h
 
 distclean maintainer-clean: clean
     rm -f pg_config.h pg_config_ext.h pg_config_os.h dynloader.h stamp-h stamp-ext-h
diff --git a/src/include/gen_listvars.pl b/src/include/gen_listvars.pl
new file mode 100755
index 0000000000..081d2ea855
--- /dev/null
+++ b/src/include/gen_listvars.pl
@@ -0,0 +1,145 @@
+#! /usr/bin/perl
+# src/include/gen_listvars.pl
+#   generates a function definition to check whether a guc variable is
+#   of list type or not.
+use strict;
+use File::Find;
+
+my $output_file = "common/check_listvars.h";
+my @varlist;
+
+find(\&findproc, "../");
+
+open my $out, '>', $output_file ||
+    die "failed to open output file $output_file: $!";
+print $out make_func_def(@varlist);
+close($out);
+print "done. The definition is written as $output_file.\n";
+exit;
+
+###############################################
+# findproc()
+#   callback function for find() to retrieve names of list variables
+#   from a file. The names are stored to @varlist.
+sub findproc
+{
+    my $f = $_;
+    my @l;
+    my $prepend = 0;
+
+    # we are interested only in .c files.
+    return if ($f !~ /\.c$/);
+
+    my $content = load_file($f);
+    if ($f =~ /^guc.c$/)
+    {
+        # guc varialbes are listed first
+        @l = extract_gucdef($content);
+        $prepend = 1;
+    }
+    else
+    {
+        # others follow guc variables
+        @l = extract_customdef($content);
+    }
+
+    @l = sort @l;
+
+    if ($#l >= 0)
+    {
+        printf("%d list variables found in %s\n", $#l + 1, $f);
+    }
+
+    if ($prepend)
+    {
+        @varlist = (@l, @varlist);
+    }
+    else
+    {
+        @varlist = (@varlist, @l);
+    }
+}
+
+###############################################
+# extract_gucdef($filecontent);
+#   collects the the name of a enbedded GUC variable with GUC_LIST_INPUT
+sub extract_gucdef
+{
+    my ($str) = @_;
+    my @ret = ();
+
+    while ($str =~ /{"([^"]+)" *, *PGC_[^}]*\s*GUC_LIST_INPUT\s*[^}]*},/g)
+    {
+        push (@ret, $1);
+    }
+
+    return @ret;
+}
+
+##################################################################
+# extract_customdef($filecontent);
+#   collects the the name of a custom variable with GUC_LIST_INPUT
+sub extract_customdef
+{
+    my ($str) = @_;
+    my @ret = ();
+
+    while ($str =~
/DefineCustomStringVariable\("([^"]+)"\s*,(?:(?!\)\s*;).)*,\s*GUC_LIST_INPUT\s*,(?:(?!\)\s*;).)*\);/g)
+    {
+        push (@ret, $1);
+    }
+
+    return @ret;
+}
+
+
+########################################################################
+# load_file($filename);
+#   returns the file content as a string in which line-feeds are removed
+sub load_file
+{
+    my $f = $_[0];
+    my $ret = "";
+
+    open my $in, '<', $f || die "failed to open file $f: $!";
+    $ret = "";
+    while (<$in>)
+    {
+        chomp;
+        $ret .= $_;
+    }
+    close $in;
+
+    return $ret;
+}
+
+###############################################
+# make_func_def(@varnames);
+#   generates the function definition
+sub make_func_def
+{
+    my @list = @_;
+    my $ret;
+
+    $ret =
+        "/* $output_file. Generated by src/include/gen_listvars.pl */\n".
+        "/* \n".
+        " * This is generated automatically, but not maintained automatically.\n".
+        " * make clean will remove file and generated in the next build.\n".
+        "*/\n\n".
+        "inline bool\n".
+        "IsConfigOptionAList(const char *name)\n".
+        "{\n".
+        "    if (";
+    my $first = 1;
+
+    foreach my $i (@list)
+    {
+        $ret .= " ||\n        " if (!$first);
+        $first = 0;
+        $ret .= "pg_strcasecmp(name, \"$i\") == 0";
+    }
+    $ret .= ")\n        return true;\n    return false;\n}\n";
+
+    return $ret;
+}
diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out
index d7eff6c0a7..ce79515ab6 100644
--- a/src/test/regress/expected/rules.out
+++ b/src/test/regress/expected/rules.out
@@ -3228,6 +3228,30 @@ SELECT pg_get_partkeydef(0);
  
 (1 row)
 
+-- test for pg_get_functiondef with list parameters which should not be
+-- quoted.
+CREATE FUNCTION func_with_set_params() RETURNS integer
+    AS 'select 1;'
+    LANGUAGE SQL
+    SET search_path TO 'pg_catalog'
+    SET wal_consistency_checking TO heap, heap2
+    SET session_preload_libraries TO foo, bar
+    IMMUTABLE STRICT;
+SELECT pg_get_functiondef('func_with_set_params'::regproc);
+                    pg_get_functiondef                    
+----------------------------------------------------------
+ CREATE OR REPLACE FUNCTION public.func_with_set_params()+
+  RETURNS integer                                        +
+  LANGUAGE sql                                           +
+  IMMUTABLE STRICT                                       +
+  SET search_path TO pg_catalog                          +
+  SET wal_consistency_checking TO heap, heap2            +
+  SET session_preload_libraries TO foo, bar              +
+ AS $function$select 1;$function$                        +
+ 
+(1 row)
+
+DROP FUNCTION func_with_set_params();
 -- test rename for a rule defined on a partitioned table
 CREATE TABLE parted_table (a int) PARTITION BY LIST (a);
 CREATE TABLE parted_table_1 PARTITION OF parted_table FOR VALUES IN (1);
diff --git a/src/test/regress/sql/rules.sql b/src/test/regress/sql/rules.sql
index 0823c02acf..0d4938d715 100644
--- a/src/test/regress/sql/rules.sql
+++ b/src/test/regress/sql/rules.sql
@@ -1170,6 +1170,18 @@ SELECT pg_get_function_arg_default(0, 0);
 SELECT pg_get_function_arg_default('pg_class'::regclass, 0);
 SELECT pg_get_partkeydef(0);
 
+-- test for pg_get_functiondef with list parameters which should not be
+-- quoted.
+CREATE FUNCTION func_with_set_params() RETURNS integer
+    AS 'select 1;'
+    LANGUAGE SQL
+    SET search_path TO 'pg_catalog'
+    SET wal_consistency_checking TO heap, heap2
+    SET session_preload_libraries TO foo, bar
+    IMMUTABLE STRICT;
+SELECT pg_get_functiondef('func_with_set_params'::regproc);
+DROP FUNCTION func_with_set_params();
+
 -- test rename for a rule defined on a partitioned table
 CREATE TABLE parted_table (a int) PARTITION BY LIST (a);
 CREATE TABLE parted_table_1 PARTITION OF parted_table FOR VALUES IN (1);
-- 
2.16.2