Received: from malur.postgresql.org ([2a02:16a8:dc51::56]) by arkaria.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.89) (envelope-from ) id 1fkEgG-0008T6-E1 for pgsql-bugs@arkaria.postgresql.org; Mon, 30 Jul 2018 20:22:12 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1fkEgE-000878-4f for pgsql-bugs@arkaria.postgresql.org; Mon, 30 Jul 2018 20:22:10 +0000 Received: from makus.postgresql.org ([2001:4800:1501:1::229]) by malur.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.89) (envelope-from ) id 1fkEgD-00084f-MB for pgsql-bugs@lists.postgresql.org; Mon, 30 Jul 2018 20:22:09 +0000 Received: from sss.pgh.pa.us ([66.207.139.130]) by makus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.89) (envelope-from ) id 1fkEgA-0000jH-HE for pgsql-bugs@lists.postgresql.org; Mon, 30 Jul 2018 20:22:08 +0000 Received: from sss1.sss.pgh.pa.us (localhost [127.0.0.1]) by sss.pgh.pa.us (8.14.4/8.14.4) with ESMTP id w6UKLxVE006355; Mon, 30 Jul 2018 16:21:59 -0400 From: Tom Lane To: pgsql-bugs@lists.postgresql.org cc: steven.winfield@cantabcapital.com, Michael Paquier Subject: Re: BUG #15248: pg_upgrade fails when a function with an empty search_path is encountered In-reply-to: <7585.1529435872@sss.pgh.pa.us> References: <152940707697.1452.4922154343836581163@wrigleys.postgresql.org> <7585.1529435872@sss.pgh.pa.us> Comments: In-reply-to Tom Lane message dated "Tue, 19 Jun 2018 15:17:52 -0400" MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0" Content-ID: <6325.1532982096.0@sss.pgh.pa.us> Date: Mon, 30 Jul 2018 16:21:59 -0400 Message-ID: <6354.1532982119@sss.pgh.pa.us> List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk ------- =_aaaaaaaaaa0 Content-Type: text/plain; charset="us-ascii" Content-ID: <6325.1532982096.1@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 ------- =_aaaaaaaaaa0 Content-Type: text/x-diff; name="fix-GUC-quoting-some-more-1.patch"; charset="us-ascii" Content-ID: <6325.1532982096.2@sss.pgh.pa.us> Content-Description: fix-GUC-quoting-some-more-1.patch Content-Transfer-Encoding: quoted-printable diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/rul= eutils.c index 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 =3D (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/varle= na.c index 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, wit= hout + * presuming whether the elements will be taken as identifiers or file n= ames. + * We assume the input has already been through flatten_set_variable_arg= s(), + * 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 opti= on + * 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 with= in + * 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 =3D rawstring; + bool done =3D false; + = + *namelist =3D NIL; + = + while (scanner_isspace(*nextp)) + nextp++; /* skip leading whitespace */ + = + if (*nextp =3D=3D '\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 =3D=3D '"') + { + /* Quoted name --- collapse quote-quote pairs */ + curname =3D nextp + 1; + for (;;) + { + endp =3D strchr(nextp + 1, '"'); + if (endp =3D=3D NULL) + return false; /* mismatched quotes */ + if (endp[1] !=3D '"') + break; /* found end of quoted name */ + /* Collapse adjacent quotes into one quote, and look again */ + memmove(endp, endp + 1, strlen(endp)); + nextp =3D endp; + } + /* endp now points at the terminating quote */ + nextp =3D endp + 1; + } + else + { + /* Unquoted name --- extends to separator or whitespace */ + curname =3D nextp; + while (*nextp && *nextp !=3D separator && + !scanner_isspace(*nextp)) + nextp++; + endp =3D nextp; + if (curname =3D=3D nextp) + return false; /* empty unquoted name not allowed */ + } + = + while (scanner_isspace(*nextp)) + nextp++; /* skip trailing whitespace */ + = + if (*nextp =3D=3D separator) + { + nextp++; + while (scanner_isspace(*nextp)) + nextp++; /* skip leading whitespace for next */ + /* we expect another name, so done remains false */ + } + else if (*nextp =3D=3D '\0') + done =3D true; + else + return false; /* invalid syntax */ + = + /* Now safe to overwrite separator with a null */ + *endp =3D '\0'; + = + /* + * Finished isolating current name --- add it to list + */ + *namelist =3D 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.c index 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 + = #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, wit= hout + * presuming whether the elements will be taken as identifiers or file n= ames. + * 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 =3D rawstring; + bool done =3D 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 =3D nextptr =3D (char **) + pg_malloc((strlen(rawstring) / 2 + 2) * sizeof(char *)); + *nextptr =3D NULL; + = + while (isspace((unsigned char) *nextp)) + nextp++; /* skip leading whitespace */ + = + if (*nextp =3D=3D '\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 =3D=3D '"') + { + /* Quoted name --- collapse quote-quote pairs */ + curname =3D nextp + 1; + for (;;) + { + endp =3D strchr(nextp + 1, '"'); + if (endp =3D=3D NULL) + return false; /* mismatched quotes */ + if (endp[1] !=3D '"') + break; /* found end of quoted name */ + /* Collapse adjacent quotes into one quote, and look again */ + memmove(endp, endp + 1, strlen(endp)); + nextp =3D endp; + } + /* endp now points at the terminating quote */ + nextp =3D endp + 1; + } + else + { + /* Unquoted name --- extends to separator or whitespace */ + curname =3D nextp; + while (*nextp && *nextp !=3D separator && + !isspace((unsigned char) *nextp)) + nextp++; + endp =3D nextp; + if (curname =3D=3D nextp) + return false; /* empty unquoted name not allowed */ + } + = + while (isspace((unsigned char) *nextp)) + nextp++; /* skip trailing whitespace */ + = + if (*nextp =3D=3D separator) + { + nextp++; + while (isspace((unsigned char) *nextp)) + nextp++; /* skip leading whitespace for next */ + /* we expect another name, so done remains false */ + } + else if (*nextp =3D=3D '\0') + done =3D true; + else + return false; /* invalid syntax */ + = + /* Now safe to overwrite separator with a null */ + *endp =3D '\0'; + = + /* + * Finished isolating current name --- add it to output array + */ + *nextptr++ =3D curname; + = + /* Loop back if we didn't reach end of string */ + } while (!done); + = + *nextptr =3D NULL; + return true; + } + = + /* * Helper function for dumping "ALTER DATABASE/ROLE SET ..." commands. * * Parse the contents of configitem (a "name=3Dvalue" string), wrap it i= n *************** makeAlterConfigCommand(PGconn *conn, con *** 912,925 **** /* * Variables that are marked GUC_LIST_QUOTE were already fully quoted b= y * 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 tha= t ! * 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 b= y * 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 =3D namelist; *nameptr; nameptr++) ! { ! if (nameptr !=3D 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.h index 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.c index 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 =3D namelist; *nameptr; nameptr++) ! { ! if (nameptr !=3D 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.h index 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/expect= ed/rules.out index 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', '', '01= 23456789012345678901234567890123456789012345678901234567890123456789012345= 678901234567890123456789' 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', '', '0123= 45678901234567890123456789012345678901234567890123456789012345678901234567= 8901234567890123456789'+ ! AS $function$select 1;$function$ = = + = (1 row) = diff --git a/src/test/regress/sql/rules.sql b/src/test/regress/sql/rules.s= ql index 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', '', '01= 23456789012345678901234567890123456789012345678901234567890123456789012345= 678901234567890123456789' IMMUTABLE STRICT; SELECT pg_get_functiondef('func_with_set_params()'::regprocedure); = ------- =_aaaaaaaaaa0--