BUG #18923: pg_dump 18beta1 fails to process complex table names

PG Bug reporting form <noreply@postgresql.org> writes:

I have just run the E-Maj project regression test suite with PG18 beta1 and
found a pg_dump abort when trying to dump a schema prefixed table having a
complex name including single quote, double quote and space.

Thanks for the report! Looks like fetchAttributeStats() is completely
misguided about the appropriate quoting rules for array elements.

regards, tom lane

On Mon, May 12, 2025 at 9:51 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

PG Bug reporting form <noreply@postgresql.org> writes:

I have just run the E-Maj project regression test suite with PG18 beta1 and
found a pg_dump abort when trying to dump a schema prefixed table having a
complex name including single quote, double quote and space.

Thanks for the report! Looks like fetchAttributeStats() is completely
misguided about the appropriate quoting rules for array elements.

re-read ReadArrayToken.
In this context, we need to care about single-quote, double-quotes.
for backslash, we skip it.

appendPGArray can help us handle double quotes,
now we just need to make it also handle single quotes.

On Tue, May 13, 2025 at 07:34:51PM +0800, jian he wrote:

On Mon, May 12, 2025 at 9:51 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

PG Bug reporting form <noreply@postgresql.org> writes:

I have just run the E-Maj project regression test suite with PG18 beta1 and
found a pg_dump abort when trying to dump a schema prefixed table having a
complex name including single quote, double quote and space.

Thanks for the report! Looks like fetchAttributeStats() is completely
misguided about the appropriate quoting rules for array elements.

D'oh, my bad.

re-read ReadArrayToken.
In this context, we need to care about single-quote, double-quotes.
for backslash, we skip it.

appendPGArray can help us handle double quotes,
now we just need to make it also handle single quotes.

I don't think we want to teach appendPGArray() to handle single quotes.
That's meant for building a text representation of a 1-dimensional Postgres
array, which IIUC doesn't need to escape single quotes. Take the following
example:

postgres=# select array['nathan''s element'];
array
----------------------
{"nathan's element"}
(1 row)

My first instinct is that we need to build the array with appendPGArray()
and then append it to the query using appendStringLiteralAH(), as done in
the attached patch.

--
nathan

My first instinct is that we need to build the array with appendPGArray()
and then append it to the query using appendStringLiteralAH(), as done in
the attached patch.

This patch looks good to me, though I'm wondering if we should add a test
case.

Nathan Bossart <nathandbossart@gmail.com> writes:

I don't think we want to teach appendPGArray() to handle single quotes.

No, that seems quite wrong. array_in won't de-dup single quotes.

My first instinct is that we need to build the array with appendPGArray()
and then append it to the query using appendStringLiteralAH(), as done in
the attached patch.

Yeah, I think so. I was confused for a bit because the one extant
user of appendPGArray is getNamespaces which does an additional layer
of quote-doubling via quoteAclUserName. However, that seems to be because
it's trying to build aclitem[] arrays whose elements will be read by
aclitemin, and that de-dups double quotes.

Wouldn't be a bad idea to add a test case.

regards, tom lane

On Fri, May 16, 2025 at 3:03 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Yeah, I think so. I was confused for a bit because the one extant
user of appendPGArray is getNamespaces which does an additional layer
of quote-doubling via quoteAclUserName. However, that seems to be because
it's trying to build aclitem[] arrays whose elements will be read by
aclitemin, and that de-dups double quotes.

getNamespaces use appendPGArray for object's initial ACL string

getNamespaces
``
nsinfo[i].dacl.initprivs = pstrdup(aclarray->data);
``

and we seems *only* use it dumpACL->buildACLCommands
``
if (!parsePGArray(baseacls, &baseitems, &nbaseitems))
{
free(aclitems);
free(baseitems);
return false;
}
``
parsePGArray didn't handle single-quotes.

I think the above is the reason single-quotes within array elements didn't cause
trouble.

On Fri, May 16, 2025 at 9:51 PM jian he <jian.universality@gmail.com> wrote:

On Fri, May 16, 2025 at 3:03 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Yeah, I think so. I was confused for a bit because the one extant
user of appendPGArray is getNamespaces which does an additional layer
of quote-doubling via quoteAclUserName. However, that seems to be

because

it's trying to build aclitem[] arrays whose elements will be read by
aclitemin, and that de-dups double quotes.

getNamespaces use appendPGArray for object's initial ACL string

getNamespaces
``
nsinfo[i].dacl.initprivs = pstrdup(aclarray->data);
``

and we seems *only* use it dumpACL->buildACLCommands
``
if (!parsePGArray(baseacls, &baseitems, &nbaseitems))
{
free(aclitems);
free(baseitems);
return false;
}
``
parsePGArray didn't handle single-quotes.

I think the above is the reason single-quotes within array elements didn't
cause
trouble.

Hi,

I’ve added a regression test covering problem

This patch combines Nathan Bossart’s original fix with suggestions from Tom
Lane and Jian He. It uses appendPGArray() to build the arrays and
appendStringLiteralAH() to quote them correctly in the EXECUTE statement.

Patch attached. Looking forward to your feedback.

Best regards,

Stepan Neretin

On Mon, May 19, 2025 at 02:24:47PM +0700, Stepan Neretin wrote:

+	'CREATE SCHEMA "phil\'s schema\"3"' => {
+		create_sql => q{
+			CREATE SCHEMA "phil's schema""3";
+			SET search_path = "phil's schema""3";
+			DROP TABLE IF EXISTS "phil's tbl1";
+			CREATE TABLE "phil's tbl1" (
+				"phil's col11" serial PRIMARY KEY,
+				"phil's col12" text
+			);
+		},
+		regexp => qr/^
+		CREATE\ SCHEMA\ "phil's\ schema""3";\n
+		SET\ search_path\ =\ "phil's\ schema""3";\n
+		DROP\ TABLE\ IF\ EXISTS\ "phil's\ tbl1";\n
+		CREATE\ TABLE\ "phil's\ tbl1"\ \(\n
+		\s+"phil's\ col11"\ integer\ NOT\ NULL\ DEFAULT\ nextval\('[^']+'\)::regclass,\n
+		\s+"phil's\ col12"\ text\n
+		\s+CONSTRAINT\ "[^"]+"\ PRIMARY\ KEY\ \("phil's\ col11"\)\n
+		\);/xm,
+		like => {},
+	},

I don't think this is quite right. IIUC this test runs some commands and
makes sure that a slightly adjusted version of those commands never appears
in the dump output. This is presumably enough to reach the reported
problem, but AFAICT most of the test content is unnecessary. I think we
should simply modify an existing stats test so that we also verify the
dumped stats commands, as done in the attached patch.

--
nathan

Committed.

--
nathan