Thread: pg_get_functiondef forgets about most GUC_LIST_INPUT GUCs
Hi all,
While reviewing another patch related to the use of pg_strcasecmp in the
backend, I have noticed this bit in ruleutils.c:
/*
* 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)
appendStringInfoString(&buf, pos);
else
simple_quote_literal(&buf, pos);
However this fails to consider all GUCs marked as GUC_LIST_INPUT, like
the recent wal_consistency_checking. guc.c already holds a find_option()
which can be used to retrieve the flags of a parameter. What about using
that and filtering by GUC_LIST_INPUT? This requires exposing the
function, and I am not sure what people think about that.
Thoughts?
--
Michael
Attachment
Michael Paquier <michael.paquier@gmail.com> writes:
> While reviewing another patch related to the use of pg_strcasecmp in the
> backend, I have noticed this bit in ruleutils.c:
> /*
> * 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)
> appendStringInfoString(&buf, pos);
> else
> simple_quote_literal(&buf, pos);
> However this fails to consider all GUCs marked as GUC_LIST_INPUT, like
> the recent wal_consistency_checking.
Mmm, yeah, probably time to find a more maintainable solution to that.
> guc.c already holds a find_option()
> which can be used to retrieve the flags of a parameter. What about using
> that and filtering by GUC_LIST_INPUT? This requires exposing the
> function, and I am not sure what people think about that.
Don't like exposing find_option directly, but I think it would make
sense to provide an API along the lines of
int GetConfigOptionFlags(const char *name, bool missing_ok).
regards, tom lane
On Thu, Jan 11, 2018 at 10:42:38AM -0500, Tom Lane wrote: > Michael Paquier <michael.paquier@gmail.com> writes: >> guc.c already holds a find_option() >> which can be used to retrieve the flags of a parameter. What about using >> that and filtering by GUC_LIST_INPUT? This requires exposing the >> function, and I am not sure what people think about that. > > Don't like exposing find_option directly, but I think it would make > sense to provide an API along the lines of > int GetConfigOptionFlags(const char *name, bool missing_ok). OK, I can live with that. What do you think about the attached? I'll be happy to produce patches for back-branches as necessary. When an option is not found, I have made the function return 0 as value for the flags, which is consistent with flatten_set_variable_args(). To make things behave more consistently with GUC_LIST_QUOTE GUCs, it seems to me that those should not be quoted as well (ALTER SYSTEM shares the same compatibility). And attached is a patch. While reviewing more the code, I have noticed the same code pattern in pg_dump.c and pg_dumpall.c. In the patch attached, I have corrected things so as all parameters marked as GUC_LIST_QUOTE are correctly not quoted because we have no generic solution to know from frontends what's a GUC type (it would make sense to me to expose a text[] with this information in pg_settings). However, let's be honest, it does not make sense to update all those parameters because who is really going to use them for functions! Two things that make sense to me are just wal_consistency_checking and synchronous_standby_names for developers which could use it to tune WAL generated within a set of SQL or plpgsql functions. As it is easier to delete than add code here, I have added all of them to ease the production of a committable version. For correctness, still having them may make sense. -- Michael
Attachment
Hello,
On Fri, Jan 12, 2018 at 10:24:40AM +0900, Michael Paquier wrote:
> OK, I can live with that. What do you think about the attached? I'll be
> happy to produce patches for back-branches as necessary. When an option
> is not found, I have made the function return 0 as value for the flags,
> which is consistent with flatten_set_variable_args(). To make things
> behave more consistently with GUC_LIST_QUOTE GUCs, it seems to me that
> those should not be quoted as well (ALTER SYSTEM shares the same
> compatibility). And attached is a patch.
Just 2 cents from me. It seems that there is a problem with extensions
GUCs. For example:
=# CREATE FUNCTION func_with_set_params() RETURNS integer
AS 'select 1;'
LANGUAGE SQL
set plpgsql.extra_errors to 'shadowed_variables';
=# 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 +
SET "plpgsql.extra_errors" TO 'shadowed_variables' +
AS $function$select 1;$function$ +
It is good while plpgsql is loaded. But when we exit the session and try
it again in another:
=# SELECT pg_get_functiondef('func_with_set_params'::regproc);
ERROR: unrecognized configuration parameter "plpgsql.extra_errors"
--
Arthur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company
On Tue, Feb 20, 2018 at 06:46:57PM +0300, Arthur Zakirov wrote:
> Just 2 cents from me. It seems that there is a problem with extensions
> GUCs.
>
> [...]
>
> =# SELECT pg_get_functiondef('func_with_set_params'::regproc);
> ERROR: unrecognized configuration parameter "plpgsql.extra_errors"
You have an excellent point here, and I did not consider it.
For pg_dump and pg_dumpall, something like what I described in
https://www.postgresql.org/message-id/20180112012440.GA2222@paquier.xyz
to extend pg_settings so as GUC types are visible via SQL could make
sense, now it is really a lot for just being able to quote parameters
correctly. On top of that, what I suggested previously would not work
reliably except if we have pg_get_functiondef load the library
associated to a given parameter. Even in this case there could
perfectly be a custom parameter from another plugin and not a parameter
associated to the function language itself.
It seems to me that this brings us back to a best-effort for the backend
and the frontend by maintaining a list of hardcoded parameter names,
which could be refined a maximum by considering lists for in-core
extensions and perhaps the most famous extensions around :(
Thoughts?
--
Michael
Attachment
On Wed, Feb 21, 2018 at 04:35:50PM +0900, Michael Paquier wrote: > You have an excellent point here, and I did not consider it. > For pg_dump and pg_dumpall, something like what I described in > https://www.postgresql.org/message-id/20180112012440.GA2222@paquier.xyz > to extend pg_settings so as GUC types are visible via SQL could make > sense, now it is really a lot for just being able to quote parameters > correctly. On top of that, what I suggested previously would not work > reliably except if we have pg_get_functiondef load the library > associated to a given parameter. Even in this case there could > perfectly be a custom parameter from another plugin and not a parameter > associated to the function language itself. In my opinion GetConfigOptionFlags() can be used for core and plpgsql GUCs. A variable should not be quoted if it has GUC_LIST_INPUT flag or it is plpgsql.extra_warnings or plpgsql.extra_errors. I'm not sure that it is good to raise an error when the variable isn't found, because without the patch the error isn't raised. But maybe better to raise it to notify a user that there is a wrong variable. In this case we may not raise the error if variable name contains GUC_QUALIFIER_SEPARATOR. > It seems to me that this brings us back to a best-effort for the backend > and the frontend by maintaining a list of hardcoded parameter names, > which could be refined a maximum by considering lists for in-core > extensions and perhaps the most famous extensions around :( It seems for frontend maintaining a hardcoded list is the only way. Agree that extending pg_settings for that could be too much. I've searched extensions in GitHub with GUC_LIST_INPUT variables. There are couple of them: - https://github.com/ohmu/pgmemcache - https://github.com/marconeperes/pgBRTypes And some not very fresh: - https://github.com/witblitz/pldotnet - https://github.com/ohmu/pgloggingfilter - https://github.com/wangyuehong/pggearman - https://github.com/siavashg/pgredis -- Arthur Zakirov Postgres Professional: http://www.postgrespro.com Russian Postgres Company
2018-02-21 8:35 GMT+01:00 Michael Paquier <michael@paquier.xyz>:
I afraid so there is not good solution. Is possible to store options in original form? When the function will be displayed, then the original value will be displayed. The current patch (with known extensions) can be used as bug fix and back patched. In new version we store original value with quotes.
On Tue, Feb 20, 2018 at 06:46:57PM +0300, Arthur Zakirov wrote:
> Just 2 cents from me. It seems that there is a problem with extensions
> GUCs.
>
> [...]
>
> =# SELECT pg_get_functiondef('func_with_set_params'::regproc);
> ERROR: unrecognized configuration parameter "plpgsql.extra_errors"
You have an excellent point here, and I did not consider it.
For pg_dump and pg_dumpall, something like what I described in
https://www.postgresql.org/message-id/20180112012440. GA2222@paquier.xyz
to extend pg_settings so as GUC types are visible via SQL could make
sense, now it is really a lot for just being able to quote parameters
correctly. On top of that, what I suggested previously would not work
reliably except if we have pg_get_functiondef load the library
associated to a given parameter. Even in this case there could
perfectly be a custom parameter from another plugin and not a parameter
associated to the function language itself.
It seems to me that this brings us back to a best-effort for the backend
and the frontend by maintaining a list of hardcoded parameter names,
which could be refined a maximum by considering lists for in-core
extensions and perhaps the most famous extensions around :(
Regards
Pavel
Thoughts?
--
Michael
On Mon, Mar 05, 2018 at 09:25:09PM +0100, Pavel Stehule wrote: > I afraid so there is not good solution. Is possible to store options in > original form? When the function will be displayed, then the original value > will be displayed. The current patch (with known extensions) can be used as > bug fix and back patched. In new version we store original value with > quotes. You mean storing the values in pg_proc.proconfig at the creation time of the function? That would basically work, except for the case of a function using a parameter which is not from the same PL. The function creation would fail because it cannot find the parameter it is looking for as we need to look at loaded parameters to know it uses a list or not :( -- Michael
Attachment
2018-03-06 2:32 GMT+01:00 Michael Paquier <michael@paquier.xyz>:
On Mon, Mar 05, 2018 at 09:25:09PM +0100, Pavel Stehule wrote:
> I afraid so there is not good solution. Is possible to store options in
> original form? When the function will be displayed, then the original value
> will be displayed. The current patch (with known extensions) can be used as
> bug fix and back patched. In new version we store original value with
> quotes.
You mean storing the values in pg_proc.proconfig at the creation time of
the function? That would basically work, except for the case of a
function using a parameter which is not from the same PL. The function
creation would fail because it cannot find the parameter it is looking
for as we need to look at loaded parameters to know it uses a list or
not :(
yes. It can fails on execution time, but it is something like runtime error.
just dump should to produce same form like was input. So if on input we got quotes, then we should to use quotes on output. Without querying somewhere.
The possible quotes can be removed in function compile time.
Pavel
--
Michael
Hello,
At Tue, 6 Mar 2018 07:14:00 +0100, Pavel Stehule <pavel.stehule@gmail.com> wrote in
<CAFj8pRB7u-D1NA8a22dytwicKv4RWrYqKCF=yiL5kKMKbssPSw@mail.gmail.com>
> 2018-03-06 2:32 GMT+01:00 Michael Paquier <michael@paquier.xyz>:
>
> > On Mon, Mar 05, 2018 at 09:25:09PM +0100, Pavel Stehule wrote:
> > > I afraid so there is not good solution. Is possible to store options in
> > > original form? When the function will be displayed, then the original
> > value
> > > will be displayed. The current patch (with known extensions) can be used
> > as
> > > bug fix and back patched. In new version we store original value with
> > > quotes.
> >
> > You mean storing the values in pg_proc.proconfig at the creation time of
> > the function? That would basically work, except for the case of a
> > function using a parameter which is not from the same PL. The function
> > creation would fail because it cannot find the parameter it is looking
> > for as we need to look at loaded parameters to know it uses a list or
> > not :(
> >
>
> yes. It can fails on execution time, but it is something like runtime error.
>
> just dump should to produce same form like was input. So if on input we got
> quotes, then we should to use quotes on output. Without querying somewhere.
>
> The possible quotes can be removed in function compile time.
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.
> inline bool
> IsConfigOptionIsAList(const char *name)
>
> {
> if (pg_strcasecmp(name, "DateStyle") == 0
> || pg_strcasecmp(name, "temp_tablespaces") == 0
> || pg_strcasecmp(name, "session_preload_libraries") == 0
> || pg_strcasecmp(name, "shared_preload_libraries") == 0
> || pg_strcasecmp(name, "local_preload_libraries") == 0
> || pg_strcasecmp(name, "search_path") == 0
> || pg_strcasecmp(name, "log_destination") == 0
> || pg_strcasecmp(name, "listen_addresses") == 0
> || pg_strcasecmp(name, "synchronous_standby_names") == 0
> || pg_strcasecmp(name, "wal_consistency_checking") == 0)
> return true;
> return false;
> }
regards,
--
Kyotaro Horiguchi
NTT Open Source Software Center
#! /usr/bin/perl
$gucfile = "src/backend/utils/misc/guc.c";
open $in, '<', $gucfile || die "hoge";
$gucfilecontent = "";
while (<$in>)
{
chomp;
$gucfilecontent .= $_;
}
print "inline bool
IsConfigOptionIsAList(const char *name)\n
{
if (";
$first = 1;
while ($gucfilecontent =~ /{"([^"]+)" *, *PGC_[^}]*[ ]*GUC_LIST_INPUT[ ]*[^}]*},/g)
{
print "\n || " if (!$first);
$first = 0;
print "pg_strcasecmp(name, \"$1\") == 0";
}
print ")\n return true;\n return false;\n}\n";
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. -- Michael
Attachment
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
Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> writes:
> 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?
Surely this is a fundamentally misguided approach. How could it
handle extension GUCs?
regards, tom lane
At Thu, 15 Mar 2018 00:33:25 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote in <22193.1521088405@sss.pgh.pa.us> > Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> writes: > > 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? > > Surely this is a fundamentally misguided approach. How could it > handle extension GUCs? I understand it is "out of scope" of this thread (for now). The starting issue here is "the static list of list-guc's are stale" so what a static list cannot cope with is still cannot be coped with by this. As the discussion upthread, with the dynamic (or on the fly) approach, pg_dump fails when required extension is not loaded. Especially plpgsql variables are the candidate stopper of ordinary pg_dump operation. We might have to implicitly load the module by any means to make it work. If we treat extensions properly, we must find the extension that have defined a GUC that is about to be exported, then load it. I don't think the automatic stuff is essential but the check_listvars.h is still effective to reduce the effort needed to maintain the multiple lists that should have the same set of names of the list-gucs. Or, we could cope with this issue if the list-ness of used GUCs is stored permanently in somewhere. Maybe pg_proc.proconfigislist or something... regards, -- Kyotaro Horiguchi NTT Open Source Software Center
On Thu, Mar 15, 2018 at 02:03:15PM +0900, Kyotaro HORIGUCHI wrote: > At Thu, 15 Mar 2018 00:33:25 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote in <22193.1521088405@sss.pgh.pa.us> >> Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> writes: >>> 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? >> >> Surely this is a fundamentally misguided approach. How could it >> handle extension GUCs? > > I understand it is "out of scope" of this thread (for now). The > starting issue here is "the static list of list-guc's are stale" > so what a static list cannot cope with is still cannot be coped > with by this. I like the mention of the idea, now it is true that that it would be a half-baked work without parameters from plpgsql, and a way to correctly track parameters marking a custom GUC as GUC_INPUT_LIST in all C files. > Or, we could cope with this issue if the list-ness of used GUCs > is stored permanently in somewhere. Maybe pg_proc.proconfigislist > or something... That does not help for PL's GUCs as well. Those may not be loaded when pg_get_functiondef gets called. At the end, we are spending a lot of resources on that. So in order to close this thread, I would suggest to just complete the list of hardcoded parameters on backend and frontend, and add as well a comment including "GUC_INPUT_LIST" so as it can be found by grepping the code. -- Michael
Attachment
At Thu, 15 Mar 2018 15:09:54 +0900, Michael Paquier <michael@paquier.xyz> wrote in <20180315060954.GE617@paquier.xyz>
> On Thu, Mar 15, 2018 at 02:03:15PM +0900, Kyotaro HORIGUCHI wrote:
> > At Thu, 15 Mar 2018 00:33:25 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote in <22193.1521088405@sss.pgh.pa.us>
> >> Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> writes:
> >>> 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?
> >>
> >> Surely this is a fundamentally misguided approach. How could it
> >> handle extension GUCs?
> >
> > I understand it is "out of scope" of this thread (for now). The
> > starting issue here is "the static list of list-guc's are stale"
> > so what a static list cannot cope with is still cannot be coped
> > with by this.
>
> I like the mention of the idea, now it is true that that it would be a
> half-baked work without parameters from plpgsql, and a way to correctly
> track parameters marking a custom GUC as GUC_INPUT_LIST in all C files.
>
> > Or, we could cope with this issue if the list-ness of used GUCs
> > is stored permanently in somewhere. Maybe pg_proc.proconfigislist
> > or something...
>
> That does not help for PL's GUCs as well. Those may not be loaded when
> pg_get_functiondef gets called.
So, we should reject to define function in the case. We don't
accept the GUC element if it is just a placeholder.
The attached is a rush work of my idea. Diff for pg_proc.h is too
large so it is separated and gziped.
It adds a column named "proconfigislist" of array(bool) in
pg_proc. When defined function has set clauses, it generates a
proconfig-is-list-or-not array and set. It ends with error if
required module is not loaded yet. Perhaps
GetConfigOptionFlags(,false) shouldn't return 0 if no option
element is found but I don't amend it for now.
Finally, it works as the follows. I think this leands to a kind
of desired behavior.
======
postgres=# CREATE FUNCTION func_with_set_params() RETURNS integer
AS 'select 1;'
LANGUAGE SQL
set plpgsql.extra_errors to 'shadowed_variables'
set work_mem to '48MB'
set plpgsql.extra_warnings to 'shadowed_variables';
ERROR: the module for variable "plpgsql.extra_errors" is not loaded yet
DETAIL: The module must be loaded before referring this variable.
postgres=# load 'plpgsql';
postgres=# CREATE FUNCTION func_with_set_params() RETURNS integer
AS 'select 1;'
LANGUAGE SQL
set plpgsql.extra_errors to 'shadowed_variables'
set work_mem to '48MB'
set plpgsql.extra_warnings to 'shadowed_variables';
postgres=# select proname, proconfig, proconfigislist from pg_proc where proconfig is not null;
-[ RECORD 1 ]---+--------------------------------------------------------------------------------------------------
proname | func_with_set_params
proconfig | {plpgsql.extra_errors=shadowed_variables,work_mem=48MB,plpgsql.extra_warnings=shadowed_variables}
proconfigislist | {t,f,t}
=======
pg_get_functiondef() can work correctly with this even if
required modules are not loaded.
But, I suppose it is a bit too big.
> At the end, we are spending a lot of resources on that. So in order to
> close this thread, I would suggest to just complete the list of
> hardcoded parameters on backend and frontend, and add as well a comment
> including "GUC_INPUT_LIST" so as it can be found by grepping the code.
regards,
--
Kyotaro Horiguchi
NTT Open Source Software Center
From d48def7a489046e44755bef250f6b35d78339803 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horiguchi.kyotaro@lab.ntt.co.jp>
Date: Thu, 15 Mar 2018 18:21:50 +0900
Subject: [PATCH 1/2] Store whether elements of proconfig are list or not in
pg_proc.
---
src/backend/catalog/pg_aggregate.c | 1 +
src/backend/catalog/pg_proc.c | 5 +++
src/backend/commands/functioncmds.c | 77 ++++++++++++++++++++++++++++++++++++-
src/backend/commands/proclang.c | 3 ++
src/backend/commands/typecmds.c | 1 +
src/backend/utils/misc/guc.c | 24 ++++++++++++
src/include/catalog/pg_class.h | 2 +-
src/include/catalog/pg_proc_fn.h | 1 +
src/include/utils/guc.h | 1 +
9 files changed, 113 insertions(+), 2 deletions(-)
diff --git a/src/backend/catalog/pg_aggregate.c b/src/backend/catalog/pg_aggregate.c
index 50d8d81f2c..3aaf0eac15 100644
--- a/src/backend/catalog/pg_aggregate.c
+++ b/src/backend/catalog/pg_aggregate.c
@@ -631,6 +631,7 @@ AggregateCreate(const char *aggName,
parameterDefaults, /* parameterDefaults */
PointerGetDatum(NULL), /* trftypes */
PointerGetDatum(NULL), /* proconfig */
+ PointerGetDatum(NULL), /* proconfigislist */
1, /* procost */
0); /* prorows */
procOid = myself.objectId;
diff --git a/src/backend/catalog/pg_proc.c b/src/backend/catalog/pg_proc.c
index 40e579f95d..cab0a225bd 100644
--- a/src/backend/catalog/pg_proc.c
+++ b/src/backend/catalog/pg_proc.c
@@ -87,6 +87,7 @@ ProcedureCreate(const char *procedureName,
List *parameterDefaults,
Datum trftypes,
Datum proconfig,
+ Datum proconfigislist,
float4 procost,
float4 prorows)
{
@@ -374,6 +375,10 @@ ProcedureCreate(const char *procedureName,
values[Anum_pg_proc_proconfig - 1] = proconfig;
else
nulls[Anum_pg_proc_proconfig - 1] = true;
+ if (proconfigislist != PointerGetDatum(NULL))
+ values[Anum_pg_proc_proconfigislist - 1] = proconfigislist;
+ else
+ nulls[Anum_pg_proc_proconfigislist - 1] = true;
/* proacl will be determined later */
rel = heap_open(ProcedureRelationId, RowExclusiveLock);
diff --git a/src/backend/commands/functioncmds.c b/src/backend/commands/functioncmds.c
index b1f87d056e..d6da508eb3 100644
--- a/src/backend/commands/functioncmds.c
+++ b/src/backend/commands/functioncmds.c
@@ -631,6 +631,73 @@ update_proconfig_value(ArrayType *a, List *set_items)
return a;
}
+static ArrayType *
+make_proconfig_islist(ArrayType *proconfig)
+{
+ ArrayType *retarray = NULL;
+ int index;
+ int i;
+ bool isnull;
+ int16 bool_typlen;
+ bool bool_typbyval;
+ bool bool_typalign;
+
+ get_typlenbyvalalign(BOOLOID,
+ &bool_typlen, &bool_typbyval, &bool_typalign);
+
+ index = ARR_DIMS(proconfig)[0];
+
+ for (i = 1 ; i <= index ; i++)
+ {
+ Datum d;
+ char *name;
+ bool is_list = false;
+
+ d = array_ref(proconfig, 1, &i,
+ -1 /* varlenarray */ ,
+ -1 /* TEXT's typlen */ ,
+ false /* TEXT's typbyval */ ,
+ 'i' /* TEXT's typalign */ ,
+ &isnull);
+ if (!isnull)
+ {
+ char *p;
+ int flags;
+
+ name = TextDatumGetCString(d);
+ p = strchr(name, '=');
+ if (p == NULL)
+ elog(ERROR, "something's wrong?");
+ *p = 0;
+
+ flags = GetConfigOptionFlags(name, false);
+ if (flags & GUC_CUSTOM_PLACEHOLDER)
+ ereport(ERROR,
+ (errmsg ("the module for variable \"%s\" is not loaded yet",
+ name),
+ errdetail ("The module must be loaded before referring this variable.")));
+ if (flags & GUC_LIST_INPUT)
+ is_list = true;
+ }
+
+ if (retarray)
+ retarray = array_set(retarray, 1, &i,
+ BoolGetDatum(is_list), false,
+ -1,
+ bool_typlen, bool_typbyval, bool_typalign);
+ else
+ {
+ Datum booldatum = BoolGetDatum(is_list);
+ retarray =
+ construct_array(&booldatum, 1,
+ BOOLOID,
+ bool_typlen, bool_typbyval, bool_typalign);
+ }
+ }
+
+ return retarray;
+}
+
/*
* Dissect the list of options assembled in gram.y into function
@@ -649,6 +716,7 @@ compute_function_attributes(ParseState *pstate,
bool *security_definer,
bool *leakproof_p,
ArrayType **proconfig,
+ ArrayType **proconfigislist,
float4 *procost,
float4 *prorows,
char *parallel_p)
@@ -767,7 +835,10 @@ compute_function_attributes(ParseState *pstate,
if (leakproof_item)
*leakproof_p = intVal(leakproof_item->arg);
if (set_items)
+ {
*proconfig = update_proconfig_value(NULL, set_items);
+ *proconfigislist = make_proconfig_islist(*proconfig);
+ }
if (cost_item)
{
*procost = defGetNumeric(cost_item);
@@ -887,6 +958,7 @@ CreateFunction(ParseState *pstate, CreateFunctionStmt *stmt)
isLeakProof;
char volatility;
ArrayType *proconfig;
+ ArrayType *proconfigislist;
float4 procost;
float4 prorows;
HeapTuple languageTuple;
@@ -911,6 +983,7 @@ CreateFunction(ParseState *pstate, CreateFunctionStmt *stmt)
isLeakProof = false;
volatility = PROVOLATILE_VOLATILE;
proconfig = NULL;
+ proconfigislist = NULL;
procost = -1; /* indicates not set */
prorows = -1; /* indicates not set */
parallel = PROPARALLEL_UNSAFE;
@@ -922,7 +995,8 @@ CreateFunction(ParseState *pstate, CreateFunctionStmt *stmt)
&as_clause, &language, &transformDefElem,
&isWindowFunc, &volatility,
&isStrict, &security, &isLeakProof,
- &proconfig, &procost, &prorows, ¶llel);
+ &proconfig, &proconfigislist,
+ &procost, &prorows, ¶llel);
/* Look up the language and validate permissions */
languageTuple = SearchSysCache1(LANGNAME, PointerGetDatum(language));
@@ -1113,6 +1187,7 @@ CreateFunction(ParseState *pstate, CreateFunctionStmt *stmt)
parameterDefaults,
PointerGetDatum(trftypes),
PointerGetDatum(proconfig),
+ PointerGetDatum(proconfigislist),
procost,
prorows);
}
diff --git a/src/backend/commands/proclang.c b/src/backend/commands/proclang.c
index 447bd49f89..2fcba890fe 100644
--- a/src/backend/commands/proclang.c
+++ b/src/backend/commands/proclang.c
@@ -142,6 +142,7 @@ CreateProceduralLanguage(CreatePLangStmt *stmt)
NIL,
PointerGetDatum(NULL),
PointerGetDatum(NULL),
+ PointerGetDatum(NULL),
1,
0);
handlerOid = tmpAddr.objectId;
@@ -181,6 +182,7 @@ CreateProceduralLanguage(CreatePLangStmt *stmt)
NIL,
PointerGetDatum(NULL),
PointerGetDatum(NULL),
+ PointerGetDatum(NULL),
1,
0);
inlineOid = tmpAddr.objectId;
@@ -223,6 +225,7 @@ CreateProceduralLanguage(CreatePLangStmt *stmt)
NIL,
PointerGetDatum(NULL),
PointerGetDatum(NULL),
+ PointerGetDatum(NULL),
1,
0);
valOid = tmpAddr.objectId;
diff --git a/src/backend/commands/typecmds.c b/src/backend/commands/typecmds.c
index bf3cd3a454..9cbd98fbca 100644
--- a/src/backend/commands/typecmds.c
+++ b/src/backend/commands/typecmds.c
@@ -1685,6 +1685,7 @@ makeRangeConstructors(const char *name, Oid namespace,
NIL, /* parameterDefaults */
PointerGetDatum(NULL), /* trftypes */
PointerGetDatum(NULL), /* proconfig */
+ PointerGetDatum(NULL), /* proconfigislist */
1.0, /* procost */
0.0); /* prorows */
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index a4f9b3668e..c67f68dc1b 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -8112,6 +8112,30 @@ GetConfigOptionByName(const char *name, const char **varname, bool missing_ok)
return _ShowOption(record, true);
}
+/*
+ * Return "flags" for a given GUC variable. If missing_ok is true, ignore
+ * any errors and return 0 to the caller instead.
+ */
+int
+GetConfigOptionFlags(const char *name, bool missing_ok)
+{
+ struct config_generic *record;
+
+ record = find_option(name, false, ERROR);
+
+ if (record == NULL)
+ {
+ if (missing_ok)
+ return 0;
+
+ ereport(ERROR,
+ (errcode(ERRCODE_UNDEFINED_OBJECT),
+ errmsg("unrecognized configuration parameter \"%s\"", name)));
+ }
+
+ return record->flags;
+}
+
/*
* Return GUC variable value by variable number; optionally return canonical
* form of name. Return value is palloc'd.
diff --git a/src/include/catalog/pg_class.h b/src/include/catalog/pg_class.h
index 01cf59e7a3..26b1866c69 100644
--- a/src/include/catalog/pg_class.h
+++ b/src/include/catalog/pg_class.h
@@ -151,7 +151,7 @@ DATA(insert OID = 1247 ( pg_type PGNSP 71 0 PGUID 0 0 0 0 0 0 0 f f p r 30 0 t
DESCR("");
DATA(insert OID = 1249 ( pg_attribute PGNSP 75 0 PGUID 0 0 0 0 0 0 0 f f p r 22 0 f f f f f f f t n f 3 1 _null_
_null__null_));
DESCR("");
-DATA(insert OID = 1255 ( pg_proc PGNSP 81 0 PGUID 0 0 0 0 0 0 0 f f p r 28 0 t f f f f f f t n f 3 1 _null_
_null__null_));
+DATA(insert OID = 1255 ( pg_proc PGNSP 81 0 PGUID 0 0 0 0 0 0 0 f f p r 29 0 t f f f f f f t n f 3 1 _null_
_null__null_));
DESCR("");
DATA(insert OID = 1259 ( pg_class PGNSP 83 0 PGUID 0 0 0 0 0 0 0 f f p r 33 0 t f f f f f f t n f 3 1 _null_
_null__null_));
DESCR("");
diff --git a/src/include/catalog/pg_proc_fn.h b/src/include/catalog/pg_proc_fn.h
index b66871bc46..94dadc5c7b 100644
--- a/src/include/catalog/pg_proc_fn.h
+++ b/src/include/catalog/pg_proc_fn.h
@@ -40,6 +40,7 @@ extern ObjectAddress ProcedureCreate(const char *procedureName,
List *parameterDefaults,
Datum trftypes,
Datum proconfig,
+ Datum proconfigislist,
float4 procost,
float4 prorows);
diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h
index 2e03640c0b..7951257c50 100644
--- a/src/include/utils/guc.h
+++ b/src/include/utils/guc.h
@@ -368,6 +368,7 @@ extern int set_config_option(const char *name, const char *value,
extern void AlterSystemSetConfigFile(AlterSystemStmt *setstmt);
extern char *GetConfigOptionByName(const char *name, const char **varname,
bool missing_ok);
+extern int GetConfigOptionFlags(const char *name, bool missing_ok);
extern void GetConfigOptionByNum(int varnum, const char **values, bool *noshow);
extern int GetNumConfigOptions(void);
--
2.16.2
Attachment
On Thu, Mar 15, 2018 at 06:48:36PM +0900, Kyotaro HORIGUCHI wrote: > So, we should reject to define function in the case. We don't > accept the GUC element if it is just a placeholder. > > The attached is a rush work of my idea. Diff for pg_proc.h is too > large so it is separated and gziped. > > It adds a column named "proconfigislist" of array(bool) in > pg_proc. When defined function has set clauses, it generates a > proconfig-is-list-or-not array and set. It ends with error if > required module is not loaded yet. Perhaps > GetConfigOptionFlags(,false) shouldn't return 0 if no option > element is found but I don't amend it for now. I think your approach has a vulnerability too. I believe that a non GUC_LIST_INPUT extension GUC which was used to create a function may become GUC_LIST_INPUT variable. If I'm not mistaken nothing stops from that. In this case values in proconfigislist won't be valide anymore. -- Arthur Zakirov Postgres Professional: http://www.postgrespro.com Russian Postgres Company
On Thu, Mar 15, 2018 at 01:33:51PM +0300, Arthur Zakirov wrote: > I think your approach has a vulnerability too. I believe that a > non GUC_LIST_INPUT extension GUC which was used to create a function may > become GUC_LIST_INPUT variable. If I'm not mistaken nothing stops from > that. In this case values in proconfigislist won't be valide anymore. I don't understand what you mean here. Are you referring to a custom GUC which was initially declared as not being a list, but became a list after a plugin upgrade with the same name? Isn't the author to blame in this case? -- Michael
Attachment
On Thu, Mar 15, 2018 at 06:48:36PM +0900, Kyotaro HORIGUCHI wrote: > postgres=# CREATE FUNCTION func_with_set_params() RETURNS integer > AS 'select 1;' > LANGUAGE SQL > set plpgsql.extra_errors to 'shadowed_variables' > set work_mem to '48MB' > set plpgsql.extra_warnings to 'shadowed_variables'; > ERROR: the module for variable "plpgsql.extra_errors" is not loaded yet > DETAIL: The module must be loaded before referring this variable. How can you be sure that a parameter actually exists? A function definition could as well use a parameter which does not exist, but you would get this error as well, no? I find that error and handling a bit confusing. > postgres=# load 'plpgsql'; > [...] > pg_get_functiondef() can work correctly with this even if > required modules are not loaded. Yeah, but the neck to any approaches here is that many applications may rely on the existing behavior, and would miss the fact that they need to load a module manually. > But, I suppose it is a bit too big. That's of course not backpatchable. -- Michael
Attachment
On Fri, Mar 16, 2018 at 10:25:35AM +0900, Michael Paquier wrote: >> But, I suppose it is a bit too big. > > That's of course not backpatchable. So in this jungle attached is my counter-proposal. As the same code pattern is repeated in three places, we could as well refactor the whole into a common routine, say in src/common/guc_options.c or similar. Perhaps just on HEAD and not back-branches as this is always annoying for packagers on Windows using custom scripts. Per the lack of complains only doing something on HEAD, with only a subset of the parameters which make sense for CREATE FUNCTION is what makes the most sense in my opinion. As mentioned in this bug, the handling of empty values gets kind of tricky as in this case proconfig stores a set of quotes and not an actual value: https://www.postgresql.org/message-id/152049236165.23137.5241258464332317087@wrigleys.postgresql.org -- Michael
Attachment
2018-03-16 5:46 GMT+01:00 Michael Paquier <michael@paquier.xyz>:
On Fri, Mar 16, 2018 at 10:25:35AM +0900, Michael Paquier wrote:
>> But, I suppose it is a bit too big.
>
> That's of course not backpatchable.
So in this jungle attached is my counter-proposal. As the same code
pattern is repeated in three places, we could as well refactor the whole
into a common routine, say in src/common/guc_options.c or similar.
Perhaps just on HEAD and not back-branches as this is always annoying
for packagers on Windows using custom scripts. Per the lack of
complains only doing something on HEAD, with only a subset of the
parameters which make sense for CREATE FUNCTION is what makes the most
sense in my opinion.
Last patch is good enough solution, I tested it and it is working
Little bit strange is support of GUC, that cannot be atteched to any fx. Probably only PGC_USERSET, maybe PGC_SUSET has sense there
+ if (pg_strcasecmp(configitem, "DateStyle") == 0 ||
+ pg_strcasecmp(configitem, "listen_addresses") == 0 ||
+ pg_strcasecmp(configitem, "local_preload_libraries") == 0 ||
+ pg_strcasecmp(configitem, "log_destination") == 0 ||
+ pg_strcasecmp(configitem, "plpgsql.extra_errors") == 0 ||
+ pg_strcasecmp(configitem, "plpgsql.extra_warnings") == 0 ||
+ pg_strcasecmp(configitem, "search_path") == 0 ||
+ pg_strcasecmp(configitem, "session_preload_libraries") == 0 ||
+ pg_strcasecmp(configitem, "shared_preload_libraries") == 0 ||
+ pg_strcasecmp(configitem, "synchronous_standby_names") == 0 ||
+ pg_strcasecmp(configitem, "temp_tablespaces") == 0 ||
+ pg_strcasecmp(configitem, "wal_consistency_checking") == 0)
+ if (pg_strcasecmp(configitem, "DateStyle") == 0 ||
+ pg_strcasecmp(configitem, "listen_addresses") == 0 ||
+ pg_strcasecmp(configitem, "local_preload_libraries") == 0 ||
+ pg_strcasecmp(configitem, "log_destination") == 0 ||
+ pg_strcasecmp(configitem, "plpgsql.extra_errors") == 0 ||
+ pg_strcasecmp(configitem, "plpgsql.extra_warnings") == 0 ||
+ pg_strcasecmp(configitem, "search_path") == 0 ||
+ pg_strcasecmp(configitem, "session_preload_libraries") == 0 ||
+ pg_strcasecmp(configitem, "shared_preload_libraries") == 0 ||
+ pg_strcasecmp(configitem, "synchronous_standby_names") == 0 ||
+ pg_strcasecmp(configitem, "temp_tablespaces") == 0 ||
+ pg_strcasecmp(configitem, "wal_consistency_checking") == 0)
another idea, how to solve this issue for extensions. The information about format of extensions GUC can be stored in new column pg_extension - maybe just a array of list of LIST GUC.
CREATE EXTENSION plpgsql ...
GUC_PREFIX 'plpgsql'
LIST_GUC ('extra_errors','extra_warnings')
...
...
Regards
Pavel
As mentioned in this bug, the handling of empty values gets kind of
tricky as in this case proconfig stores a set of quotes and not an
actual value:
https://www.postgresql.org/message-id/152049236165.23137. 5241258464332317087@wrigleys. postgresql.org
--
Michael
At Fri, 16 Mar 2018 09:16:54 +0100, Pavel Stehule <pavel.stehule@gmail.com> wrote in
<CAFj8pRC6+_8c6PLHbDzCSKEDpkEhcVqyFmJn9_zYUjAwdnUboQ@mail.gmail.com>
> 2018-03-16 5:46 GMT+01:00 Michael Paquier <michael@paquier.xyz>:
>
> > On Fri, Mar 16, 2018 at 10:25:35AM +0900, Michael Paquier wrote:
> > >> But, I suppose it is a bit too big.
> > >
> > > That's of course not backpatchable.
I meant that it is just too big for the objective regardless of
back-patching:p
So, automatic generation is not acceptable, dynamic checking
(including that at the function creation) would be
unacceptable. So I agree that what we can do now is just maintian
the list of pg_strcasecmp.
> > So in this jungle attached is my counter-proposal. As the same code
> > pattern is repeated in three places, we could as well refactor the whole
> > into a common routine, say in src/common/guc_options.c or similar.
> > Perhaps just on HEAD and not back-branches as this is always annoying
> > for packagers on Windows using custom scripts. Per the lack of
> > complains only doing something on HEAD, with only a subset of the
> > parameters which make sense for CREATE FUNCTION is what makes the most
> > sense in my opinion.
> >
>
> Last patch is good enough solution, I tested it and it is working
>
> Little bit strange is support of GUC, that cannot be atteched to any fx.
> Probably only PGC_USERSET, maybe PGC_SUSET has sense there
That's true, but doesn't the additional rule make it more
bothersome to maintain the list?
> + if (pg_strcasecmp(configitem, "DateStyle") == 0 ||
> + pg_strcasecmp(configitem, "listen_addresses") == 0 ||
> + pg_strcasecmp(configitem, "local_preload_libraries")
> == 0 ||
> + pg_strcasecmp(configitem, "log_destination") == 0 ||
> + pg_strcasecmp(configitem, "plpgsql.extra_errors") == 0
> ||
> + pg_strcasecmp(configitem, "plpgsql.extra_warnings") ==
> 0 ||
> + pg_strcasecmp(configitem, "search_path") == 0 ||
> + pg_strcasecmp(configitem, "session_preload_libraries")
> == 0 ||
> + pg_strcasecmp(configitem, "shared_preload_libraries")
> == 0 ||
> + pg_strcasecmp(configitem, "synchronous_standby_names")
> == 0 ||
> + pg_strcasecmp(configitem, "temp_tablespaces") == 0 ||
> + pg_strcasecmp(configitem, "wal_consistency_checking")
> == 0)
>
>
> another idea, how to solve this issue for extensions. The information about
> format of extensions GUC can be stored in new column pg_extension - maybe
> just a array of list of LIST GUC.
>
> CREATE EXTENSION plpgsql ...
> GUC_PREFIX 'plpgsql'
> LIST_GUC ('extra_errors','extra_warnings')
> ...
>
> Regards
>
> Pavel
>
>
>
> >
> > As mentioned in this bug, the handling of empty values gets kind of
> > tricky as in this case proconfig stores a set of quotes and not an
> > actual value:
> > https://www.postgresql.org/message-id/152049236165.23137.
> > 5241258464332317087@wrigleys.postgresql.org
> > --
> > Michael
--
Kyotaro Horiguchi
NTT Open Source Software Center
2018-03-16 9:34 GMT+01:00 Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp>:
At Fri, 16 Mar 2018 09:16:54 +0100, Pavel Stehule <pavel.stehule@gmail.com> wrote in <CAFj8pRC6+_8c6PLHbDzCSKEDpkEhcVqyFmJn9_ zYUjAwdnUboQ@mail.gmail.com>
> 2018-03-16 5:46 GMT+01:00 Michael Paquier <michael@paquier.xyz>:
>
> > On Fri, Mar 16, 2018 at 10:25:35AM +0900, Michael Paquier wrote:
> > >> But, I suppose it is a bit too big.
> > >
> > > That's of course not backpatchable.
I meant that it is just too big for the objective regardless of
back-patching:p
So, automatic generation is not acceptable, dynamic checking
(including that at the function creation) would be
unacceptable. So I agree that what we can do now is just maintian
the list of pg_strcasecmp.
> > So in this jungle attached is my counter-proposal. As the same code
> > pattern is repeated in three places, we could as well refactor the whole
> > into a common routine, say in src/common/guc_options.c or similar.
> > Perhaps just on HEAD and not back-branches as this is always annoying
> > for packagers on Windows using custom scripts. Per the lack of
> > complains only doing something on HEAD, with only a subset of the
> > parameters which make sense for CREATE FUNCTION is what makes the most
> > sense in my opinion.
> >
>
> Last patch is good enough solution, I tested it and it is working
>
> Little bit strange is support of GUC, that cannot be atteched to any fx.
> Probably only PGC_USERSET, maybe PGC_SUSET has sense there
That's true, but doesn't the additional rule make it more
bothersome to maintain the list?
Hard to say. I have not opinion. But just when I see in this context variables like listen_address, shared_preload_librarries, then it looks messy.
> + if (pg_strcasecmp(configitem, "DateStyle") == 0 ||
> + pg_strcasecmp(configitem, "listen_addresses") == 0 ||
> + pg_strcasecmp(configitem, "local_preload_libraries")
> == 0 ||
> + pg_strcasecmp(configitem, "log_destination") == 0 ||
> + pg_strcasecmp(configitem, "plpgsql.extra_errors") == 0
> ||
> + pg_strcasecmp(configitem, "plpgsql.extra_warnings") ==
> 0 ||
> + pg_strcasecmp(configitem, "search_path") == 0 ||
> + pg_strcasecmp(configitem, "session_preload_libraries")
> == 0 ||
> + pg_strcasecmp(configitem, "shared_preload_libraries")
> == 0 ||
> + pg_strcasecmp(configitem, "synchronous_standby_names")
> == 0 ||
> + pg_strcasecmp(configitem, "temp_tablespaces") == 0 ||
> + pg_strcasecmp(configitem, "wal_consistency_checking")
> == 0)
>
>
> another idea, how to solve this issue for extensions. The information about
> format of extensions GUC can be stored in new column pg_extension - maybe
> just a array of list of LIST GUC.
>
> CREATE EXTENSION plpgsql ...
> GUC_PREFIX 'plpgsql'
> LIST_GUC ('extra_errors','extra_warnings')
> ...
>
> Regards
>
> Pavel
>
>
>
> >
> > As mentioned in this bug, the handling of empty values gets kind of
> > tricky as in this case proconfig stores a set of quotes and not an
> > actual value:
> > https://www.postgresql.org/message-id/152049236165.23137.
> > 5241258464332317087@wrigleys.postgresql.org
> > --
> > Michael--
Kyotaro Horiguchi
NTT Open Source Software Center
On Fri, Mar 16, 2018 at 10:21:39AM +0900, Michael Paquier wrote: > On Thu, Mar 15, 2018 at 01:33:51PM +0300, Arthur Zakirov wrote: > > I think your approach has a vulnerability too. I believe that a > > non GUC_LIST_INPUT extension GUC which was used to create a function may > > become GUC_LIST_INPUT variable. If I'm not mistaken nothing stops from > > that. In this case values in proconfigislist won't be valide anymore. > > I don't understand what you mean here. Are you referring to a custom > GUC which was initially declared as not being a list, but became a list > after a plugin upgrade with the same name? Yes exactly. Sorry for the unclear message. > Isn't the author to blame in this case? Maybe he is. It may be better to rename a variable if it became a list. I haven't strong opinion here though. I wanted to point the case where proconfigislist column won't work. -- Arthur Zakirov Postgres Professional: http://www.postgrespro.com Russian Postgres Company
On Fri, Mar 16, 2018 at 09:40:18AM +0100, Pavel Stehule wrote: > 2018-03-16 9:34 GMT+01:00 Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp >> That's true, but doesn't the additional rule make it more >> bothersome to maintain the list? > > Hard to say. I have not opinion. But just when I see in this context > variables like listen_address, shared_preload_libraries, then it looks > messy. Let's be clear. I have listed all the variables in the patch to gather more easily opinions, and because it is easier to review the whole stack this way. I personally think that the only variables where the patch makes sense are: - DateStyle - search_path - plpgsql.extra_errors - plpgsql.extra_warnings - wal_consistency_checking So I would be incline to drop the rest from the patch. If there are authors of popular extensions willing to get this support, let's update the list once they argue about it and only if it makes sense. However, as far as I can see, there are no real candidates. So let's keep the list simple. -- Michael
Attachment
At Fri, 16 Mar 2018 21:15:54 +0900, Michael Paquier <michael@paquier.xyz> wrote in <20180316121554.GA2552@paquier.xyz> > On Fri, Mar 16, 2018 at 09:40:18AM +0100, Pavel Stehule wrote: > > 2018-03-16 9:34 GMT+01:00 Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp > >> That's true, but doesn't the additional rule make it more > >> bothersome to maintain the list? > > > > Hard to say. I have not opinion. But just when I see in this context > > variables like listen_address, shared_preload_libraries, then it looks > > messy. > > Let's be clear. I have listed all the variables in the patch to gather > more easily opinions, and because it is easier to review the whole stack > this way. I personally think that the only variables where the patch > makes sense are: > - DateStyle > - search_path > - plpgsql.extra_errors > - plpgsql.extra_warnings > - wal_consistency_checking > So I would be incline to drop the rest from the patch. If there are > authors of popular extensions willing to get this support, let's update > the list once they argue about it and only if it makes sense. However, > as far as I can see, there are no real candidates. So let's keep the > list simple. FWIW +1 from me. It seems reasonable as the amendment to the current status. regards, -- Kyotaro Horiguchi NTT Open Source Software Center
Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> writes:
> At Fri, 16 Mar 2018 21:15:54 +0900, Michael Paquier <michael@paquier.xyz> wrote in
<20180316121554.GA2552@paquier.xyz>
>> Let's be clear. I have listed all the variables in the patch to gather
>> more easily opinions, and because it is easier to review the whole stack
>> this way. I personally think that the only variables where the patch
>> makes sense are:
>> - DateStyle
>> - search_path
>> - plpgsql.extra_errors
>> - plpgsql.extra_warnings
>> - wal_consistency_checking
>> So I would be incline to drop the rest from the patch. If there are
>> authors of popular extensions willing to get this support, let's update
>> the list once they argue about it and only if it makes sense. However,
>> as far as I can see, there are no real candidates. So let's keep the
>> list simple.
> FWIW +1 from me. It seems reasonable as the amendment to the
> current status.
It suddenly struck me that the scope of the patch is wider than it needs
to be. We don't need special behavior for all GUC_LIST variables, only
for GUC_LIST_QUOTE variables. (For example, SET datestyle = 'iso, mdy'
works just as well as SET datestyle = iso, mdy.)
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 (and, perhaps,
enforce this in DefineCustomStringVariable? or is that cure worse than
the disease?)
regards, tom lane
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. For example
with HEAD and this function:
=# CREATE or replace FUNCTION func_with_set_params() RETURNS integer
AS 'select 1;'
LANGUAGE SQL
SET search_path TO 'pg_catalog, public'
SET wal_consistency_checking TO heap, heap2
SET session_preload_libraries TO 'foo, bar'
IMMUTABLE STRICT;
Then retrieving the function definition results in that:
=# SELECT pg_get_functiondef('func_with_set_params'::regproc);
CREATE OR REPLACE FUNCTION public.func_with_set_params()
RETURNS integer
LANGUAGE sql
IMMUTABLE STRICT
SET search_path TO "pg_catalog, public"
SET wal_consistency_checking TO 'heap, heap2'
SET session_preload_libraries TO '"foo, bar"'
AS $function$select 1;$function$
And that's wrong as "pg_catalog, public" is only a one-element list, no?
> (and, perhaps,
> enforce this in DefineCustomStringVariable? or is that cure worse than
> the disease?)
Extension authors can just mark the variables they define with
GUC_LIST_QUOTE, so enforcing it would be a step backwards in my
opinion.
--
Michael
Attachment
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.
regards, tom lane
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); }
So here's what I think we should do about this:
1. Only GUC_LIST_QUOTE variables need to be special-cased. It works
fine to treat plain LIST variables (e.g. datestyle) with the regular
code path. This is good because there are so few of them.
2. We should make an effort to minimize the number of places that know
explicitly which variables are GUC_LIST_QUOTE. In the backend, the
right thing to do is ask guc.c. In pg_dump, there's no convenient
way to ask the backend (and certainly nothing that would work against
old servers), but we should at least make sure there's only one place
to fix not two.
3. We need to prevent extensions from trying to create GUC_LIST_QUOTE
variables, because those are not going to work correctly if they're
set before the extension is loaded, nor is there any hope of pg_dump
dumping them correctly.
The attached 0001 patch does the first two things, and could be
back-patched. The 0002 patch, which I'd only propose for HEAD,
is one way we could address point 3. A different way would be
to throw a WARNING rather than ERROR and then just mask off the
problematic bit. Or we could decide that either of these cures
is worse than the disease, but I don't really agree with that.
Comments?
regards, tom lane
diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index 2cd54ec..b0559ca 100644
*** a/src/backend/utils/adt/ruleutils.c
--- b/src/backend/utils/adt/ruleutils.c
***************
*** 61,66 ****
--- 61,67 ----
#include "utils/array.h"
#include "utils/builtins.h"
#include "utils/fmgroids.h"
+ #include "utils/guc.h"
#include "utils/hsearch.h"
#include "utils/lsyscache.h"
#include "utils/rel.h"
*************** pg_get_functiondef(PG_FUNCTION_ARGS)
*** 2597,2607 ****
quote_identifier(configitem));
/*
! * 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)
appendStringInfoString(&buf, pos);
else
simple_quote_literal(&buf, pos);
--- 2598,2612 ----
quote_identifier(configitem));
/*
! * Variables that are marked GUC_LIST_QUOTE were already fully
! * quoted by flatten_set_variable_args() before they were put
! * into the proconfig array; we mustn't re-quote them or we'll
! * make a mess. Variables that are not so marked should just
! * be emitted as simple string literals. If the variable is
! * not known to guc.c, we'll do the latter; this makes it
! * unsafe to use GUC_LIST_QUOTE for extension variables.
*/
! if (GetConfigOptionFlags(configitem, true) & GUC_LIST_QUOTE)
appendStringInfoString(&buf, pos);
else
simple_quote_literal(&buf, pos);
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 7a7ac47..398680a 100644
*** a/src/backend/utils/misc/guc.c
--- b/src/backend/utils/misc/guc.c
*************** static const unit_conversion time_unit_c
*** 796,803 ****
*
* 6. Don't forget to document the option (at least in config.sgml).
*
! * 7. If it's a new GUC_LIST option you must edit pg_dumpall.c to ensure
! * it is not single quoted at dump time.
*/
--- 796,803 ----
*
* 6. Don't forget to document the option (at least in config.sgml).
*
! * 7. If it's a new GUC_LIST_QUOTE option, you must add it to
! * variable_is_guc_list_quote() in src/bin/pg_dump/dumputils.c.
*/
*************** GetConfigOptionResetString(const char *n
*** 6859,6864 ****
--- 6859,6888 ----
return NULL;
}
+ /*
+ * Get the GUC flags associated with the given option.
+ *
+ * If the option doesn't exist, return 0 if missing_ok is true,
+ * otherwise throw an ereport and don't return.
+ */
+ int
+ GetConfigOptionFlags(const char *name, bool missing_ok)
+ {
+ struct config_generic *record;
+
+ record = find_option(name, false, WARNING);
+ if (record == NULL)
+ {
+ if (missing_ok)
+ return 0;
+ ereport(ERROR,
+ (errcode(ERRCODE_UNDEFINED_OBJECT),
+ errmsg("unrecognized configuration parameter \"%s\"",
+ name)));
+ }
+ return record->flags;
+ }
+
/*
* flatten_set_variable_args
diff --git a/src/bin/pg_dump/dumputils.c b/src/bin/pg_dump/dumputils.c
index 7f5bb13..875545f 100644
*** a/src/bin/pg_dump/dumputils.c
--- b/src/bin/pg_dump/dumputils.c
*************** buildACLQueries(PQExpBuffer acl_subquery
*** 851,856 ****
--- 851,879 ----
}
/*
+ * Detect whether the given GUC variable is of GUC_LIST_QUOTE type.
+ *
+ * It'd be better if we could inquire this directly from the backend; but even
+ * if there were a function for that, it could only tell us about variables
+ * currently known to guc.c, so that it'd be unsafe for extensions to declare
+ * GUC_LIST_QUOTE variables anyway. Lacking a solution for that, it doesn't
+ * seem worth the work to do more than have this list, which must be kept in
+ * sync with the variables actually marked GUC_LIST_QUOTE in guc.c.
+ */
+ bool
+ variable_is_guc_list_quote(const char *name)
+ {
+ if (pg_strcasecmp(name, "temp_tablespaces") == 0 ||
+ pg_strcasecmp(name, "session_preload_libraries") == 0 ||
+ pg_strcasecmp(name, "shared_preload_libraries") == 0 ||
+ pg_strcasecmp(name, "local_preload_libraries") == 0 ||
+ pg_strcasecmp(name, "search_path") == 0)
+ return true;
+ else
+ return false;
+ }
+
+ /*
* Helper function for dumping "ALTER DATABASE/ROLE SET ..." commands.
*
* Parse the contents of configitem (a "name=value" string), wrap it in
*************** makeAlterConfigCommand(PGconn *conn, con
*** 887,897 ****
appendPQExpBuffer(buf, "SET %s TO ", fmtId(mine));
/*
! * 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)
appendPQExpBufferStr(buf, pos);
else
appendStringLiteralConn(buf, pos, conn);
--- 910,924 ----
appendPQExpBuffer(buf, "SET %s TO ", fmtId(mine));
/*
! * Variables that are marked GUC_LIST_QUOTE were already fully quoted by
! * flatten_set_variable_args() before they were put into the setconfig
! * array; we mustn't re-quote them or we'll make a mess. Variables that
! * are not so marked should just be emitted as simple string literals. If
! * the variable is not known to variable_is_guc_list_quote(), we'll do the
! * latter; this makes it unsafe to use GUC_LIST_QUOTE for extension
! * variables.
*/
! if (variable_is_guc_list_quote(mine))
appendPQExpBufferStr(buf, pos);
else
appendStringLiteralConn(buf, pos, conn);
diff --git a/src/bin/pg_dump/dumputils.h b/src/bin/pg_dump/dumputils.h
index a9e26ae..7bd3e1d 100644
*** a/src/bin/pg_dump/dumputils.h
--- b/src/bin/pg_dump/dumputils.h
*************** extern void buildACLQueries(PQExpBuffer
*** 56,61 ****
--- 56,63 ----
const char *acl_column, const char *acl_owner,
const char *obj_kind, bool binary_upgrade);
+ extern bool variable_is_guc_list_quote(const char *name);
+
extern void makeAlterConfigCommand(PGconn *conn, const char *configitem,
const char *type, const char *name,
const char *type2, const char *name2,
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 566cbf2..b8d65a9 100644
*** a/src/bin/pg_dump/pg_dump.c
--- b/src/bin/pg_dump/pg_dump.c
*************** dumpFunc(Archive *fout, FuncInfo *finfo)
*** 11877,11887 ****
appendPQExpBuffer(q, "\n SET %s TO ", fmtId(configitem));
/*
! * 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)
appendPQExpBufferStr(q, pos);
else
appendStringLiteralAH(q, pos, fout);
--- 11877,11891 ----
appendPQExpBuffer(q, "\n SET %s TO ", fmtId(configitem));
/*
! * Variables that are marked GUC_LIST_QUOTE were already fully quoted
! * by flatten_set_variable_args() before they were put into the
! * proconfig array; we mustn't re-quote them or we'll make a mess.
! * Variables that are not so marked should just be emitted as simple
! * string literals. If the variable is not known to
! * variable_is_guc_list_quote(), we'll do the latter; this makes it
! * unsafe to use GUC_LIST_QUOTE for extension variables.
*/
! if (variable_is_guc_list_quote(configitem))
appendPQExpBufferStr(q, pos);
else
appendStringLiteralAH(q, pos, fout);
diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h
index 2e03640..3d13a33 100644
*** a/src/include/utils/guc.h
--- b/src/include/utils/guc.h
*************** extern void EmitWarningsOnPlaceholders(c
*** 349,354 ****
--- 349,355 ----
extern const char *GetConfigOption(const char *name, bool missing_ok,
bool restrict_superuser);
extern const char *GetConfigOptionResetString(const char *name);
+ extern int GetConfigOptionFlags(const char *name, bool missing_ok);
extern void ProcessConfigFile(GucContext context);
extern void InitializeGUCOptions(void);
extern bool SelectConfigFiles(const char *userDoption, const char *progname);
diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out
index 5e0597e..9730c6b 100644
*** a/src/test/regress/expected/rules.out
--- b/src/test/regress/expected/rules.out
*************** SELECT pg_get_partkeydef(0);
*** 3228,3233 ****
--- 3228,3260 ----
(1 row)
+ -- test for pg_get_functiondef properly regurgitating SET parameters
+ -- Note that the function is kept around to stress pg_dump.
+ CREATE FUNCTION func_with_set_params() RETURNS integer
+ AS 'select 1;'
+ LANGUAGE SQL
+ SET search_path TO PG_CATALOG
+ SET extra_float_digits TO 2
+ SET work_mem TO '4MB'
+ SET datestyle to iso, mdy
+ SET local_preload_libraries TO "Mixed/Case", 'c:/"a"/path'
+ IMMUTABLE STRICT;
+ SELECT pg_get_functiondef('func_with_set_params()'::regprocedure);
+ 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 extra_float_digits TO '2' +
+ SET work_mem TO '4MB' +
+ SET "DateStyle" TO 'iso, mdy' +
+ SET local_preload_libraries TO "Mixed/Case", "c:/""a""/path"+
+ AS $function$select 1;$function$ +
+
+ (1 row)
+
-- test rename for a rule defined on a partitioned table
CREATE TABLE rules_parted_table (a int) PARTITION BY LIST (a);
CREATE TABLE rules_parted_table_1 PARTITION OF rules_parted_table FOR VALUES IN (1);
diff --git a/src/test/regress/sql/rules.sql b/src/test/regress/sql/rules.sql
index 6021212..61f97f5 100644
*** a/src/test/regress/sql/rules.sql
--- b/src/test/regress/sql/rules.sql
*************** SELECT pg_get_function_arg_default(0, 0)
*** 1170,1175 ****
--- 1170,1188 ----
SELECT pg_get_function_arg_default('pg_class'::regclass, 0);
SELECT pg_get_partkeydef(0);
+ -- test for pg_get_functiondef properly regurgitating SET parameters
+ -- Note that the function is kept around to stress pg_dump.
+ CREATE FUNCTION func_with_set_params() RETURNS integer
+ AS 'select 1;'
+ LANGUAGE SQL
+ SET search_path TO PG_CATALOG
+ SET extra_float_digits TO 2
+ SET work_mem TO '4MB'
+ SET datestyle to iso, mdy
+ SET local_preload_libraries TO "Mixed/Case", 'c:/"a"/path'
+ IMMUTABLE STRICT;
+ SELECT pg_get_functiondef('func_with_set_params()'::regprocedure);
+
-- test rename for a rule defined on a partitioned table
CREATE TABLE rules_parted_table (a int) PARTITION BY LIST (a);
CREATE TABLE rules_parted_table_1 PARTITION OF rules_parted_table FOR VALUES IN (1);
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 398680a..153373e 100644
*** a/src/backend/utils/misc/guc.c
--- b/src/backend/utils/misc/guc.c
*************** init_custom_variable(const char *name,
*** 7607,7612 ****
--- 7607,7621 ----
elog(FATAL, "cannot create PGC_POSTMASTER variables after startup");
/*
+ * We can't support custom GUC_LIST_QUOTE variables, because the wrong
+ * things would happen if such a variable were set or pg_dump'd when the
+ * defining extension isn't loaded. Again, treat this as fatal because
+ * the loadable module may be partly initialized already.
+ */
+ if (flags & GUC_LIST_QUOTE)
+ elog(FATAL, "extensions cannot define GUC_LIST_QUOTE variables");
+
+ /*
* Before pljava commit 398f3b876ed402bdaec8bc804f29e2be95c75139
* (2015-12-15), two of that module's PGC_USERSET variables facilitated
* trivial escalation to superuser privileges. Restrict the variables to
On Tue, Mar 20, 2018 at 01:27:35PM -0400, Tom Lane wrote: > 1. Only GUC_LIST_QUOTE variables need to be special-cased. It works > fine to treat plain LIST variables (e.g. datestyle) with the regular > code path. This is good because there are so few of them. Check. > 2. We should make an effort to minimize the number of places that know > explicitly which variables are GUC_LIST_QUOTE. In the backend, the > right thing to do is ask guc.c. In pg_dump, there's no convenient > way to ask the backend (and certainly nothing that would work against > old servers), but we should at least make sure there's only one place > to fix not two. The only way I can think about for that would be to provide this information in pg_settings using a text[] array or such which lists all the GUC flags of a parameter. That's a hell lot of infrastructure though which can be easily replaced with the maintenance of a small hardcoded parameter list. > 3. We need to prevent extensions from trying to create GUC_LIST_QUOTE > variables, because those are not going to work correctly if they're > set before the extension is loaded, nor is there any hope of pg_dump > dumping them correctly. This really depends on the elements of the list involved here, so pg_dump may be able to dump them correctly, though not reliably as that would be fully application-dependent. At the end I think that I am on board with what you are proposing here. > The attached 0001 patch does the first two things, and could be > back-patched. The 0002 patch, which I'd only propose for HEAD, > is one way we could address point 3. A different way would be > to throw a WARNING rather than ERROR and then just mask off the > problematic bit. Or we could decide that either of these cures > is worse than the disease, but I don't really agree with that. Looks roughly sane to me. + if (flags & GUC_LIST_QUOTE) + elog(FATAL, "extensions cannot define GUC_LIST_QUOTE variables"); This would be better as an ereport with ERRCODE_FEATURE_NOT_SUPPORTED I think. An ERROR is better in my opinion. - if (pg_strcasecmp(configitem, "DateStyle") == 0 - || pg_strcasecmp(configitem, "search_path") == 0) + if (GetConfigOptionFlags(configitem, true) & GUC_LIST_QUOTE) For boolean-based comparisons, I would recommend using a comparison with zero. + record = find_option(name, false, WARNING); + if (record == NULL) LOG instead of WARNING? -- Michael
Attachment
Michael Paquier <michael@paquier.xyz> writes:
> On Tue, Mar 20, 2018 at 01:27:35PM -0400, Tom Lane wrote:
>> + if (flags & GUC_LIST_QUOTE)
>> + elog(FATAL, "extensions cannot define GUC_LIST_QUOTE variables");
> This would be better as an ereport with ERRCODE_FEATURE_NOT_SUPPORTED I
> think. An ERROR is better in my opinion.
I don't mind making it an ereport, but I think it needs to be FATAL
for the reason stated in the comment.
>> + record = find_option(name, false, WARNING);
>> + if (record == NULL)
> LOG instead of WARNING?
I did it like that to match the similar code in flatten_set_variable_args.
In the end it doesn't matter, because find_option only uses its elevel
argument when create_placeholders is passed as true.
regards, tom lane
On Wed, Mar 21, 2018 at 01:40:23AM -0400, Tom Lane wrote: > Michael Paquier <michael@paquier.xyz> writes: >> On Tue, Mar 20, 2018 at 01:27:35PM -0400, Tom Lane wrote: >>> + if (flags & GUC_LIST_QUOTE) >>> + elog(FATAL, "extensions cannot define GUC_LIST_QUOTE variables"); > >> This would be better as an ereport with ERRCODE_FEATURE_NOT_SUPPORTED I >> think. An ERROR is better in my opinion. > > I don't mind making it an ereport, but I think it needs to be FATAL > for the reason stated in the comment. Okay for the FATAL. I can see that at this time of the day your patch 0002 has already been pushed as 846b5a5 with an elog(). Still, it seems to me that this is not an internal error but an error caused by an external cause which can be user-visible, so I would push forward with switching it to an ereport(). -- Michael
Attachment
Michael Paquier <michael@paquier.xyz> writes:
> On Wed, Mar 21, 2018 at 01:40:23AM -0400, Tom Lane wrote:
>> I don't mind making it an ereport, but I think it needs to be FATAL
>> for the reason stated in the comment.
> Okay for the FATAL. I can see that at this time of the day your patch
> 0002 has already been pushed as 846b5a5 with an elog(). Still, it seems
> to me that this is not an internal error but an error caused by an
> external cause which can be user-visible, so I would push forward with
> switching it to an ereport().
Well, after some consideration I pushed it like that because it's not
really a user-facing error but a developer-facing error. Should we ask
translators to spend time on that message? I doubt it. Also, the
adjacent test to refuse PGC_POSTMASTER variables is just an elog;
that seems like pretty much the same sort of situation, and nobody's
complained about it.
If we do something here we should change both of those calls, but
I really doubt it's worth the effort.
regards, tom lane