Hello, At Wed, 14 Mar 2018 17:46:21 +0900, Michael Paquier <mich...@paquier.xyz> wrote in <20180314084621.ga...@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.kyot...@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