BUG #15248: pg_upgrade fails when a function with an empty search_path is encountered

-----Original Message-----
From: PG Bug reporting form [mailto:noreply@postgresql.org]
Sent: 19 June 2018 12:18
To: pgsql-bugs@lists.postgresql.org
Cc: Steven Winfield
Subject: BUG #15248: pg_upgrade fails when a function with an empty search_path is encountered

The following bug has been logged on the website:

Bug reference: 15248
Logged by: Steven Winfield
Email address: steven.winfield@cantabcapital.com
PostgreSQL version: 11beta1
Operating system: RHEL 7.4
Description:

I suspect this is reproducible with pg_dump / pg_restore too.

If a function is defined like this:

CREATE FUNCTION public.foo(int) RETURNS int
LANGUAGE "sql"
SET search_path TO ''
AS $_$ SELECT 1; $_$;

...then, during pg_upgrade, pg_restore renders this SQL to recreate the
function using double-quotes around the empty search_path:

pg_restore: creating FUNCTION "public.foo("int")"
pg_restore: [archiver (db)] Error while PROCESSING TOC:
pg_restore: [archiver (db)] Error from TOC entry 3488; 1255 67351571
FUNCTION foo("int") dbadmin
pg_restore: [archiver (db)] could not execute query: ERROR: zero-length
delimited identifier at or near """"
LINE 3: SET "search_path" TO ""
^
Command was: CREATE FUNCTION "public"."foo"("int") RETURNS int
LANGUAGE "sql"
SET "search_path" TO ""
AS $_$ SELECT 1 $_$

The fix is to use single quotes in this case, and I suppose the workaround
is to specify 'pg_temp,pg_catalog' instead of the empty string.

Also, either pg_get_functiondef is similarly affected, or pg_dump/pg_restore is using that that to generate the SQL containing the double quotes.

This email is confidential. If you are not the intended recipient, please advise us immediately and delete this message.
The registered name of Cantab- part of GAM Systematic is Cantab Capital Partners LLP.
See - http://www.gam.com/en/Legal/Email+disclosures+EU for further information on confidentiality, the risks of non-secure electronic communication, and certain disclosures which we are required to make in accordance with applicable legislation and regulations.
If you cannot access this link, please notify us by reply message and we will send the contents to you.

GAM Holding AG and its subsidiaries (Cantab – GAM Systematic) will collect and use information about you in the course of your interactions with us.
Full details about the data types we collect and what we use this for and your related rights is set out in our online privacy policy at https://www.gam.com/en/legal/privacy-policy.
Please familiarise yourself with this policy and check it from time to time for updates as it supplements this notice.

=?utf-8?q?PG_Bug_reporting_form?= <noreply@postgresql.org> writes:

If a function is defined like this:

CREATE FUNCTION public.foo(int) RETURNS int
LANGUAGE "sql"
SET search_path TO ''
AS $_$ SELECT 1; $_$;

...then, during pg_upgrade, pg_restore renders this SQL to recreate the
function using double-quotes around the empty search_path:

Command was: CREATE FUNCTION "public"."foo"("int") RETURNS int
LANGUAGE "sql"
SET "search_path" TO ""
AS $_$ SELECT 1 $_$

[ which isn't legal syntax ]

Hm. So, basically, this is due to insufficient thought about quoting
rules in commits 742869946 et al. We made pg_dump print out exactly
what it sees in pg_proc.proconfig for this case --- but that string
has been constructed by flatten_set_variable_args, and while its
quoting rules are a lot like those for double-quoted SQL identifiers,
they aren't identical. Notably we have this issue, that "" is a legal
empty string as per flatten_set_variable_args, but it's not a legal
SQL identifier. Another problem, now that I think about it, is that
on reload a double-quoted identifier is going to get truncated to
NAMEDATALEN, but that's not necessarily appropriate --- some
GUC_LIST_QUOTE variables contain file path names that could legally
be longer than that.

If we were working in a green field I'd think about revising
flatten_set_variable_args, but touching those rules is pretty scary;
even if we found all the relevant places in the backend and pg_dump,
there might be third-party code that is looking at proconfig or
related catalog columns. Also, that'd do nothing for existing
database objects that have config strings in the existing style.

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.

regards, tom lane

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