Date: Fri, 12 Jan 2018 10:24:40 +0900
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
Message-ID: <20180112012440.GA2222@paquier.xyz>
References: <20180111064900.GA51030@paquier.xyz>
 <13647.1515685358@sss.pgh.pa.us>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
	protocol="application/pgp-signature"; boundary="kORqDWCi7qDJ0mEj"
Content-Disposition: inline
In-Reply-To: <13647.1515685358@sss.pgh.pa.us>
Precedence: bulk


--kORqDWCi7qDJ0mEj
Content-Type: multipart/mixed; boundary="PNTmBPCT7hxwcZjr"
Content-Disposition: inline


--PNTmBPCT7hxwcZjr
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

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.
>=20
> 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

--PNTmBPCT7hxwcZjr
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment; filename="guc-list-track-v1.patch"
Content-Transfer-Encoding: quoted-printable

diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/rule=
utils.c
index 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 =3D TextDatumGetCString(d);
 				char	   *pos;
+				int			flags;
=20
 				pos =3D strchr(configitem, '=3D');
 				if (pos =3D=3D NULL)
@@ -2571,11 +2573,11 @@ pg_get_functiondef(PG_FUNCTION_ARGS)
 								 quote_identifier(configitem));
=20
 				/*
-				 * 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") =3D=3D 0
-					|| pg_strcasecmp(configitem, "search_path") =3D=3D 0)
+				flags =3D GetConfigOptionFlags(configitem, false);
+				if ((flags & GUC_LIST_INPUT) !=3D 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.c
index 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);
 }
=20
+/*
+ * 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 =3D find_option(name, false, ERROR);
+
+	if (record =3D=3D 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 canonic=
al
  * 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.c
index 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));
=20
 		/*
-		 * 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") =3D=3D 0
-			|| pg_strcasecmp(configitem, "search_path") =3D=3D 0)
+		if (pg_strcasecmp(configitem, "DateStyle") =3D=3D 0 ||
+			pg_strcasecmp(configitem, "listen_addresses") =3D=3D 0 ||
+			pg_strcasecmp(configitem, "local_preload_libraries") =3D=3D 0 ||
+			pg_strcasecmp(configitem, "log_destination") =3D=3D 0 ||
+			pg_strcasecmp(configitem, "search_path") =3D=3D 0 ||
+			pg_strcasecmp(configitem, "session_preload_libraries") =3D=3D 0 ||
+			pg_strcasecmp(configitem, "shared_preload_libraries") =3D=3D 0 ||
+			pg_strcasecmp(configitem, "synchronous_standby_names") =3D=3D 0 ||
+			pg_strcasecmp(configitem, "temp_tablespaces") =3D=3D 0 ||
+			pg_strcasecmp(configitem, "wal_consistency_checking") =3D=3D 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.c
index 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 *ar=
rayitem,
 	appendPQExpBuffer(buf, "SET %s TO ", fmtId(mine));
=20
 	/*
-	 * 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") =3D=3D 0
-		|| pg_strcasecmp(mine, "search_path") =3D=3D 0)
+	if (pg_strcasecmp(mine, "DateStyle") =3D=3D 0 ||
+		pg_strcasecmp(mine, "listen_addresses") =3D=3D 0 ||
+		pg_strcasecmp(mine, "local_preload_libraries") =3D=3D 0 ||
+		pg_strcasecmp(mine, "log_destination") =3D=3D 0 ||
+		pg_strcasecmp(mine, "search_path") =3D=3D 0 ||
+		pg_strcasecmp(mine, "session_preload_libraries") =3D=3D 0 ||
+		pg_strcasecmp(mine, "shared_preload_libraries") =3D=3D 0 ||
+		pg_strcasecmp(mine, "synchronous_standby_names") =3D=3D 0 ||
+		pg_strcasecmp(mine, "temp_tablespaces") =3D=3D 0 ||
+		pg_strcasecmp(mine, "wal_consistency_checking") =3D=3D 0)
 		appendPQExpBufferStr(buf, pos + 1);
 	else
 		appendStringLiteralConn(buf, pos + 1, conn);
diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h
index 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 ch=
ar *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 *no=
show);
 extern int	GetNumConfigOptions(void);
=20
diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expecte=
d/rules.out
index 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);
 =20
 (1 row)
=20
+-- 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                   =20
+----------------------------------------------------------
+ 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$                        +
+=20
+(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 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);
=20
+-- 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);

--PNTmBPCT7hxwcZjr--

--kORqDWCi7qDJ0mEj
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAEBCAAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAlpYDlgACgkQnvQgOdby
QH2nEQ/6A8SsNJ8vQHIUoHohwGQIn+Sc3SZiz4i5QUTIUBGgN4Z8XxQViZUI57Ex
yuUZowKj2Y5LtdG4WFvz7diuN+lXio9ZKe16Q4RRLLNRiGGcURWKDsqD7uf7qNR5
6oQF9c55nYRu97rO2eFtpMe8hJMaIg7MLkeTtEr9W2lH8C689QlsS1OQ/93hs1qI
vqeQnBeZMLPFJzCp+OiSTpKAtcBnNAtF71TjHke7UZxf/kRQ52OBM9EEfgjmwBT1
4b5LdbN02EPsB+Mnz+dgbwaHqLoeEO3kI4scFOf5UWGc+ULSL9NJuPl3tSZbVFFm
U4g2+hymMS09Cozv3/2KiLEWsGeVHx7fw9nTi7HL+RHkd9A7UGJFXYifxAv2ctnU
diGxsIX2jV0mgYw+o/SH283ipswgessADMqI7jT6P1OwYND8raGHCX6XLDlSEvil
0I65a3xv+sY7tn1H87Xs3No1U3t69qHXNrqu8FRI1vzR/v8uzScurbeK2EoqlF1/
sJ7oTDJ/mYA+mWijciBZHsr6mUhENA03WO4d+95ReS7nWALST6qH43d48FigBfrq
UFdi4HUVycNuC3faOQzzJcGrWgQUj6MgWNJEN4iDoFiPBpfD5tnS0jMhPWO0ygPu
GLcImvmtyFY8eI5HzGkxkabEdpyjznlqUF7sxrId61vyhQ3xTTY=
=TG2v
-----END PGP SIGNATURE-----

--kORqDWCi7qDJ0mEj--