From: Michael Paquier <michael.paquier@gmail.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Postgres hackers <pgsql-hackers@postgresql.org>
Subject: Re: pg_get_functiondef forgets about most GUC_LIST_INPUT GUCs
Date: Fri, 12 Jan 2018 10:24:40 +0900
Message-ID: <20180112012440.GA2222@paquier.xyz> (raw)
In-Reply-To: <13647.1515685358@sss.pgh.pa.us>
References: <20180111064900.GA51030@paquier.xyz>
<13647.1515685358@sss.pgh.pa.us>
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
Attachments:guc-list-track-v1.patch (text/plain, 7.7K) downloaddiff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.cindex 9cdbb06add..7914f4dd88 100644--- a/src/backend/utils/adt/ruleutils.c+++ b/src/backend/utils/adt/ruleutils.c@@ -61,6 +61,7 @@
#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"
@@ -2561,6 +2562,7 @@ pg_get_functiondef(PG_FUNCTION_ARGS)
{
char *configitem = TextDatumGetCString(d);
char *pos;
+ int flags;
pos = strchr(configitem, '=');
if (pos == NULL)
@@ -2571,11 +2573,11 @@ pg_get_functiondef(PG_FUNCTION_ARGS)
quote_identifier(configitem));
/*
- * Some GUC variable names are 'LIST' type and hence must not- * be quoted.+ * Some GUC variable names are of type GUC_LIST_INPUT+ * and hence must not be quoted.
*/
- if (pg_strcasecmp(configitem, "DateStyle") == 0- || pg_strcasecmp(configitem, "search_path") == 0)+ flags = GetConfigOptionFlags(configitem, false);+ if ((flags & GUC_LIST_INPUT) != 0)
appendStringInfoString(&buf, pos);
else
simple_quote_literal(&buf, pos);
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.cindex 72f6be329e..5a39c96242 100644--- a/src/backend/utils/misc/guc.c+++ b/src/backend/utils/misc/guc.c@@ -8099,6 +8099,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/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.cindex 27628a397c..f9b9de8894 100644--- a/src/bin/pg_dump/pg_dump.c+++ b/src/bin/pg_dump/pg_dump.c@@ -11777,11 +11777,20 @@ dumpFunc(Archive *fout, FuncInfo *finfo)
appendPQExpBuffer(q, "\n SET %s TO ", fmtId(configitem));
/*
- * Some GUC variable names are 'LIST' type and hence must not be- * quoted.+ * Some GUC variable names are of type GUC_LIST_INPUT and hence+ * must not be quoted.+ * XXX: Keep this list in sync with what is defined in guc.c!
*/
- if (pg_strcasecmp(configitem, "DateStyle") == 0- || pg_strcasecmp(configitem, "search_path") == 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, "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)
appendPQExpBufferStr(q, pos);
else
appendStringLiteralAH(q, pos, fout);
diff --git a/src/bin/pg_dump/pg_dumpall.c b/src/bin/pg_dump/pg_dumpall.cindex 3dd2c3871e..2fd43b039e 100644--- a/src/bin/pg_dump/pg_dumpall.c+++ b/src/bin/pg_dump/pg_dumpall.c@@ -1728,10 +1728,20 @@ makeAlterConfigCommand(PGconn *conn, const char *arrayitem,
appendPQExpBuffer(buf, "SET %s TO ", fmtId(mine));
/*
- * Some GUC variable names are 'LIST' type and hence must not be quoted.+ * Some GUC variable names are of type GUC_LIST_INPUT and hence+ * must not be quoted.+ * XXX: Keep this list in sync with what is defined in guc.c!
*/
- if (pg_strcasecmp(mine, "DateStyle") == 0- || pg_strcasecmp(mine, "search_path") == 0)+ if (pg_strcasecmp(mine, "DateStyle") == 0 ||+ pg_strcasecmp(mine, "listen_addresses") == 0 ||+ pg_strcasecmp(mine, "local_preload_libraries") == 0 ||+ pg_strcasecmp(mine, "log_destination") == 0 ||+ pg_strcasecmp(mine, "search_path") == 0 ||+ pg_strcasecmp(mine, "session_preload_libraries") == 0 ||+ pg_strcasecmp(mine, "shared_preload_libraries") == 0 ||+ pg_strcasecmp(mine, "synchronous_standby_names") == 0 ||+ pg_strcasecmp(mine, "temp_tablespaces") == 0 ||+ pg_strcasecmp(mine, "wal_consistency_checking") == 0)
appendPQExpBufferStr(buf, pos + 1);
else
appendStringLiteralConn(buf, pos + 1, conn);
diff --git a/src/include/utils/guc.h b/src/include/utils/guc.hindex 77daa5a539..b02e65cf59 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);
diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.outindex f1c1b44d6f..c7ebb29b65 100644--- a/src/test/regress/expected/rules.out+++ b/src/test/regress/expected/rules.out@@ -3226,6 +3226,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.sqlindex 0ded0f01d2..40a6914e69 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);
signature.asc (application/pgp-signature, 832B) download
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: michael.paquier@gmail.com, tgl@sss.pgh.pa.us
Subject: Re: pg_get_functiondef forgets about most GUC_LIST_INPUT GUCs
In-Reply-To: <20180112012440.GA2222@paquier.xyz>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox