From: Tom Lane <tgl@sss.pgh.pa.us>
To: pgsql-bugs@lists.postgresql.org
Cc: steven.winfield@cantabcapital.com, Michael Paquier <michael@paquier.xyz>
Subject: Re: BUG #15248: pg_upgrade fails when a function with an empty search_path is encountered
Date: Mon, 30 Jul 2018 16:21:59 -0400
Message-ID: <6354.1532982119@sss.pgh.pa.us> (raw)
In-Reply-To: <7585.1529435872@sss.pgh.pa.us>
References: <152940707697.1452.4922154343836581163@wrigleys.postgresql.org>
<7585.1529435872@sss.pgh.pa.us>
I wrote:
> So it seems like what we have to do here is to teach pg_dump and ruleutils
> to parse a GUC_LIST_QUOTE value the same way SplitIdentifierString does,
> and then quote each extracted list element as a string literal. Bleah.
> It's not *that* much code, but it's annoying, especially because of the
> duplicated logic.
Here's a proposed patch for this. As I feared, there's kind of a lot of
code duplication :-(. I thought for awhile about trying to unify the four
copies of the split-on-delimiters code into one function with a bunch of
option flags ... but it seemed like that would be pretty messy too, so I
desisted.
regards, tom lane
Attachments:
[text/x-diff] fix-GUC-quoting-some-more-1.patch (19.8K, ../6354.1532982119@sss.pgh.pa.us/2-fix-GUC-quoting-some-more-1.patch)
download | inline diff:diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.cindex 5a61d2d..03e9a28 100644
*** a/src/backend/utils/adt/ruleutils.c
--- b/src/backend/utils/adt/ruleutils.c
*************** pg_get_functiondef(PG_FUNCTION_ARGS)
*** 2640,2653 ****
/*
* 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);
appendStringInfoChar(&buf, '\n');
--- 2640,2678 ----
/*
* Variables that are marked GUC_LIST_QUOTE were already fully
* quoted by flatten_set_variable_args() before they were put
! * into the proconfig array. However, because the quoting
! * rules used there aren't exactly like SQL's, we have to
! * break the list value apart and then quote the elements as
! * string literals. (The elements may be double-quoted as-is,
! * but we can't just feed them to the SQL parser; it would do
! * the wrong thing with elements that are zero-length or
! * longer than NAMEDATALEN.)
! *
! * 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 that; this makes it unsafe to use
! * GUC_LIST_QUOTE for extension variables.
*/
if (GetConfigOptionFlags(configitem, true) & GUC_LIST_QUOTE)
! {
! List *namelist;
! ListCell *lc;
!
! /* Parse string into list of identifiers */
! if (!SplitGUCList(pos, ',', &namelist))
! {
! /* this shouldn't fail really */
! elog(ERROR, "invalid list syntax in proconfig item");
! }
! foreach(lc, namelist)
! {
! char *curname = (char *) lfirst(lc);
!
! simple_quote_literal(&buf, curname);
! if (lnext(lc))
! appendStringInfoString(&buf, ", ");
! }
! }
else
simple_quote_literal(&buf, pos);
appendStringInfoChar(&buf, '\n');
diff --git a/src/backend/utils/adt/varlena.c b/src/backend/utils/adt/varlena.cindex 31eaa92..a26ff82 100644
*** a/src/backend/utils/adt/varlena.c
--- b/src/backend/utils/adt/varlena.c
*************** SplitDirectoriesString(char *rawstring,
*** 3503,3508 ****
--- 3503,3620 ----
}
+ /*+ * SplitGUCList --- parse a string containing identifiers or file names+ *+ * This is used to split the value of a GUC_LIST_QUOTE GUC variable, without+ * presuming whether the elements will be taken as identifiers or file names.+ * We assume the input has already been through flatten_set_variable_args(),+ * so that we need never downcase (if appropriate, that was done already).+ * Nor do we ever truncate, since we don't know the correct max length.+ * We disallow embedded whitespace for simplicity (it shouldn't matter,+ * because any embedded whitespace should have led to double-quoting).+ * Otherwise the API is identical to SplitIdentifierString.+ *+ * XXX it's annoying to have three copies of this string-splitting logic.+ * However, it's not clear that having one function with a bunch of option+ * flags would be much better.+ *+ * XXX there is a version of this function in src/bin/pg_dump/dumputils.c.+ * Be sure to update that if you have to change this.+ *+ * Inputs:+ * rawstring: the input string; must be overwritable! On return, it's+ * been modified to contain the separated identifiers.+ * separator: the separator punctuation expected between identifiers+ * (typically '.' or ','). Whitespace may also appear around+ * identifiers.+ * Outputs:+ * namelist: filled with a palloc'd list of pointers to identifiers within+ * rawstring. Caller should list_free() this even on error return.+ *+ * Returns true if okay, false if there is a syntax error in the string.+ */+ bool+ SplitGUCList(char *rawstring, char separator,+ List **namelist)+ {+ char *nextp = rawstring;+ bool done = false;+ + *namelist = NIL;+ + while (scanner_isspace(*nextp))+ nextp++; /* skip leading whitespace */+ + if (*nextp == '\0')+ return true; /* allow empty string */+ + /* At the top of the loop, we are at start of a new identifier. */+ do+ {+ char *curname;+ char *endp;+ + if (*nextp == '"')+ {+ /* Quoted name --- collapse quote-quote pairs */+ curname = nextp + 1;+ for (;;)+ {+ endp = strchr(nextp + 1, '"');+ if (endp == NULL)+ return false; /* mismatched quotes */+ if (endp[1] != '"')+ break; /* found end of quoted name */+ /* Collapse adjacent quotes into one quote, and look again */+ memmove(endp, endp + 1, strlen(endp));+ nextp = endp;+ }+ /* endp now points at the terminating quote */+ nextp = endp + 1;+ }+ else+ {+ /* Unquoted name --- extends to separator or whitespace */+ curname = nextp;+ while (*nextp && *nextp != separator &&+ !scanner_isspace(*nextp))+ nextp++;+ endp = nextp;+ if (curname == nextp)+ return false; /* empty unquoted name not allowed */+ }+ + while (scanner_isspace(*nextp))+ nextp++; /* skip trailing whitespace */+ + if (*nextp == separator)+ {+ nextp++;+ while (scanner_isspace(*nextp))+ nextp++; /* skip leading whitespace for next */+ /* we expect another name, so done remains false */+ }+ else if (*nextp == '\0')+ done = true;+ else+ return false; /* invalid syntax */+ + /* Now safe to overwrite separator with a null */+ *endp = '\0';+ + /*+ * Finished isolating current name --- add it to list+ */+ *namelist = lappend(*namelist, curname);+ + /* Loop back if we didn't reach end of string */+ } while (!done);+ + return true;+ }+ +
/*****************************************************************************
* Comparison Functions used for bytea
*
diff --git a/src/bin/pg_dump/dumputils.c b/src/bin/pg_dump/dumputils.cindex 875545f..8a93ace 100644
*** a/src/bin/pg_dump/dumputils.c
--- b/src/bin/pg_dump/dumputils.c
***************
*** 14,19 ****
--- 14,21 ----
*/
#include "postgres_fe.h"
+ #include <ctype.h>+
#include "dumputils.h"
#include "fe_utils/string_utils.h"
*************** variable_is_guc_list_quote(const char *n
*** 874,879 ****
--- 876,990 ----
}
/*
+ * SplitGUCList --- parse a string containing identifiers or file names+ *+ * This is used to split the value of a GUC_LIST_QUOTE GUC variable, without+ * presuming whether the elements will be taken as identifiers or file names.+ * See comparable code in src/backend/utils/adt/varlena.c.+ *+ * Inputs:+ * rawstring: the input string; must be overwritable! On return, it's+ * been modified to contain the separated identifiers.+ * separator: the separator punctuation expected between identifiers+ * (typically '.' or ','). Whitespace may also appear around+ * identifiers.+ * Outputs:+ * namelist: receives a malloc'd, null-terminated array of pointers to+ * identifiers within rawstring. Caller should free this+ * even on error return.+ *+ * Returns true if okay, false if there is a syntax error in the string.+ */+ bool+ SplitGUCList(char *rawstring, char separator,+ char ***namelist)+ {+ char *nextp = rawstring;+ bool done = false;+ char **nextptr;+ + /*+ * Since we disallow empty identifiers, this is a conservative+ * overestimate of the number of pointers we could need. Allow one for+ * list terminator.+ */+ *namelist = nextptr = (char **)+ pg_malloc((strlen(rawstring) / 2 + 2) * sizeof(char *));+ *nextptr = NULL;+ + while (isspace((unsigned char) *nextp))+ nextp++; /* skip leading whitespace */+ + if (*nextp == '\0')+ return true; /* allow empty string */+ + /* At the top of the loop, we are at start of a new identifier. */+ do+ {+ char *curname;+ char *endp;+ + if (*nextp == '"')+ {+ /* Quoted name --- collapse quote-quote pairs */+ curname = nextp + 1;+ for (;;)+ {+ endp = strchr(nextp + 1, '"');+ if (endp == NULL)+ return false; /* mismatched quotes */+ if (endp[1] != '"')+ break; /* found end of quoted name */+ /* Collapse adjacent quotes into one quote, and look again */+ memmove(endp, endp + 1, strlen(endp));+ nextp = endp;+ }+ /* endp now points at the terminating quote */+ nextp = endp + 1;+ }+ else+ {+ /* Unquoted name --- extends to separator or whitespace */+ curname = nextp;+ while (*nextp && *nextp != separator &&+ !isspace((unsigned char) *nextp))+ nextp++;+ endp = nextp;+ if (curname == nextp)+ return false; /* empty unquoted name not allowed */+ }+ + while (isspace((unsigned char) *nextp))+ nextp++; /* skip trailing whitespace */+ + if (*nextp == separator)+ {+ nextp++;+ while (isspace((unsigned char) *nextp))+ nextp++; /* skip leading whitespace for next */+ /* we expect another name, so done remains false */+ }+ else if (*nextp == '\0')+ done = true;+ else+ return false; /* invalid syntax */+ + /* Now safe to overwrite separator with a null */+ *endp = '\0';+ + /*+ * Finished isolating current name --- add it to output array+ */+ *nextptr++ = curname;+ + /* Loop back if we didn't reach end of string */+ } while (!done);+ + *nextptr = NULL;+ return true;+ }+ + /*
* 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
*** 912,925 ****
/*
* 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);
--- 1023,1057 ----
/*
* Variables that are marked GUC_LIST_QUOTE were already fully quoted by
* flatten_set_variable_args() before they were put into the setconfig
! * array. However, because the quoting rules used there aren't exactly
! * like SQL's, we have to break the list value apart and then quote the
! * elements as string literals. (The elements may be double-quoted as-is,
! * but we can't just feed them to the SQL parser; it would do the wrong
! * thing with elements that are zero-length or longer than NAMEDATALEN.)
! *
! * 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 that; this makes it unsafe to
! * use GUC_LIST_QUOTE for extension variables.
*/
if (variable_is_guc_list_quote(mine))
! {
! char **namelist;
! char **nameptr;
!
! /* Parse string into list of identifiers */
! /* this shouldn't fail really */
! if (SplitGUCList(pos, ',', &namelist))
! {
! for (nameptr = namelist; *nameptr; nameptr++)
! {
! if (nameptr != namelist)
! appendPQExpBufferStr(buf, ", ");
! appendStringLiteralConn(buf, *nameptr, conn);
! }
! }
! pg_free(namelist);
! }
else
appendStringLiteralConn(buf, pos, conn);
diff --git a/src/bin/pg_dump/dumputils.h b/src/bin/pg_dump/dumputils.hindex 7bd3e1d..0043124 100644
*** a/src/bin/pg_dump/dumputils.h
--- b/src/bin/pg_dump/dumputils.h
*************** extern void buildACLQueries(PQExpBuffer
*** 58,63 ****
--- 58,66 ----
extern bool variable_is_guc_list_quote(const char *name);
+ extern bool SplitGUCList(char *rawstring, char separator,+ char ***namelist);+
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.cindex 31ebd8b..20e8aed 100644
*** a/src/bin/pg_dump/pg_dump.c
--- b/src/bin/pg_dump/pg_dump.c
*************** dumpFunc(Archive *fout, FuncInfo *finfo)
*** 11964,11977 ****
/*
* 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);
}
--- 11964,11999 ----
/*
* Variables that are marked GUC_LIST_QUOTE were already fully quoted
* by flatten_set_variable_args() before they were put into the
! * proconfig array. However, because the quoting rules used there
! * aren't exactly like SQL's, we have to break the list value apart
! * and then quote the elements as string literals. (The elements may
! * be double-quoted as-is, but we can't just feed them to the SQL
! * parser; it would do the wrong thing with elements that are
! * zero-length or longer than NAMEDATALEN.)
! *
* 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 that; this makes it unsafe
! * to use GUC_LIST_QUOTE for extension variables.
*/
if (variable_is_guc_list_quote(configitem))
! {
! char **namelist;
! char **nameptr;
!
! /* Parse string into list of identifiers */
! /* this shouldn't fail really */
! if (SplitGUCList(pos, ',', &namelist))
! {
! for (nameptr = namelist; *nameptr; nameptr++)
! {
! if (nameptr != namelist)
! appendPQExpBufferStr(q, ", ");
! appendStringLiteralAH(q, *nameptr, fout);
! }
! }
! pg_free(namelist);
! }
else
appendStringLiteralAH(q, pos, fout);
}
diff --git a/src/include/utils/varlena.h b/src/include/utils/varlena.hindex 90e131a..c776931 100644
*** a/src/include/utils/varlena.h
--- b/src/include/utils/varlena.h
*************** extern bool SplitIdentifierString(char *
*** 31,36 ****
--- 31,38 ----
List **namelist);
extern bool SplitDirectoriesString(char *rawstring, char separator,
List **namelist);
+ extern bool SplitGUCList(char *rawstring, char separator,+ List **namelist);
extern text *replace_text_regexp(text *src_text, void *regexp,
text *replace_text, bool glob);
diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.outindex 744d501..078129f 100644
*** a/src/test/regress/expected/rules.out
--- b/src/test/regress/expected/rules.out
*************** CREATE FUNCTION func_with_set_params() R
*** 3160,3180 ****
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)
--- 3160,3180 ----
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', '', '0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789'
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', '', '0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789'+
! AS $function$select 1;$function$ +
(1 row)
diff --git a/src/test/regress/sql/rules.sql b/src/test/regress/sql/rules.sqlindex 3ca4c07..f4ee30e 100644
*** a/src/test/regress/sql/rules.sql
--- b/src/test/regress/sql/rules.sql
*************** CREATE FUNCTION func_with_set_params() R
*** 1164,1170 ****
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);
--- 1164,1170 ----
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', '', '0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789'
IMMUTABLE STRICT;
SELECT pg_get_functiondef('func_with_set_params()'::regprocedure);
=
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-bugs@postgresql.org
Cc: tgl@sss.pgh.pa.us, pgsql-bugs@lists.postgresql.org, michael@paquier.xyz
Subject: Re: BUG #15248: pg_upgrade fails when a function with an empty search_path is encountered
In-Reply-To: <6354.1532982119@sss.pgh.pa.us>
* 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