From: Jeff Davis <pgsql@j-davis.com>
To: Michael Paquier <michael@paquier.xyz>
Cc: pgsql-bugs@postgresql.org
Subject: Re: pg_dump assertion failure with "-n pg_catalog"
Date: Thu, 15 Jun 2023 15:47:59 -0700
Message-ID: <e7c1adfe66542eca348de7485990abfabdbc1086.camel@j-davis.com> (raw)
In-Reply-To: <ZIbAuHfnWY05VxVt@paquier.xyz>
References: <ce071503fee88334aa70f360e6e4ea14d48305ee.camel@j-davis.com>
<ZIbAuHfnWY05VxVt@paquier.xyz>
On Mon, 2023-06-12 at 15:52 +0900, Michael Paquier wrote:
> Attempting the same command with a patched pg_dump on ~14 triggers
> more failures. The first assertion hit is this one, for instance:
> $ pg_dump -n pg_catalog
> pg_dump: pg_dump.c:13460: dumpCollation: Assertion `colliculocale !=
> ((void *)0)' failed.
Thank you, new patch attached.
I changed the Assert()s (the old ones as well as the ones I added) into
pg_log_warning()s. I don't think it makes sense to assert based on the
catalog contents in pg_dump -- other places just warn.
Also added a test.
> I'd like to agree with you about the fact that having collprovider
> handled before the rest makes things easier to follow in the future.
> At least that's my feeling after looking at your patch.
The new patch does add about 40 net lines, though.
Given that it fixes an obscure issue, I'm not even sure if it should be
backported -- perhaps it could just go in 17 as more of a cleanup than
a bugfix?
Even aside from the issue fixed by this patch, there are other warnings
emitted when dumping pg_catalog related to types with typtype='p' and
typisdefined=t.
Regards,
Jeff Davis
Attachments:v2-0001-Fix-pg_dump-assertion-failure-when-dumping-pg_cat.patch (text/x-patch, 4.2K) download
From f9c4c3cc58e1e3ec306dd04e1c5f9f3ce48bd741 Mon Sep 17 00:00:00 2001
From: Jeff Davis <jeff@j-davis.com>
Date: Wed, 7 Jun 2023 10:32:52 -0700
Subject: [PATCH v2] Fix pg_dump assertion failure when dumping pg_catalog.
---
src/bin/pg_dump/pg_dump.c | 69 ++++++++++++++++++++++++--------
src/bin/pg_dump/t/002_pg_dump.pl | 8 ++++
2 files changed, 61 insertions(+), 16 deletions(-)
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.cindex 5dab1ba9ea..c73e9a11da 100644--- a/src/bin/pg_dump/pg_dump.c+++ b/src/bin/pg_dump/pg_dump.c@@ -13415,6 +13415,18 @@ dumpCollation(Archive *fout, const CollInfo *collinfo)
else
collctype = NULL;
+ /*+ * Before version 15, collcollate and collctype were of type NAME and+ * non-nullable. Treat empty strings as NULL for consistency.+ */+ if (fout->remoteVersion < 150000)+ {+ if (collcollate[0] == '\0')+ collcollate = NULL;+ if (collctype[0] == '\0')+ collctype = NULL;+ }+
if (!PQgetisnull(res, 0, i_colliculocale))
colliculocale = PQgetvalue(res, 0, i_colliculocale);
else
@@ -13446,35 +13458,60 @@ dumpCollation(Archive *fout, const CollInfo *collinfo)
if (strcmp(PQgetvalue(res, 0, i_collisdeterministic), "f") == 0)
appendPQExpBufferStr(q, ", deterministic = false");
- if (colliculocale != NULL)+ if (collprovider[0] == 'd')
{
- appendPQExpBufferStr(q, ", locale = ");- appendStringLiteralAH(q, colliculocale, fout);+ if (collcollate || collctype || colliculocale || collicurules)+ pg_log_warning("invalid collation \"%s\"", qcollname);++ /* no locale -- the default collation cannot be reloaded anyway */
}
- else+ else if (collprovider[0] == 'i')
{
- Assert(collcollate != NULL);- Assert(collctype != NULL);+ if (fout->remoteVersion >= 150000)+ {+ if (collcollate || collctype || !colliculocale)+ pg_log_warning("invalid collation \"%s\"", qcollname);- if (strcmp(collcollate, collctype) == 0)+ appendPQExpBufferStr(q, ", locale = ");+ appendStringLiteralAH(q, colliculocale ? colliculocale : "",+ fout);+ }+ else
{
+ if (!collcollate || !collctype || colliculocale ||+ strcmp(collcollate, collctype) != 0)+ pg_log_warning("invalid collation \"%s\"", qcollname);+
appendPQExpBufferStr(q, ", locale = ");
- appendStringLiteralAH(q, collcollate, fout);+ appendStringLiteralAH(q, collcollate ? collcollate : "", fout);+ }++ if (collicurules)+ {+ appendPQExpBufferStr(q, ", rules = ");+ appendStringLiteralAH(q, collicurules ? collicurules : "", fout);+ }+ }+ else if (collprovider[0] == 'c')+ {+ if (colliculocale || collicurules || !collcollate || !collctype)+ pg_log_warning("invalid collation \"%s\"", qcollname);++ if (collcollate && collctype && strcmp(collcollate, collctype) == 0)+ {+ appendPQExpBufferStr(q, ", locale = ");+ appendStringLiteralAH(q, collcollate ? collcollate : "", fout);
}
else
{
appendPQExpBufferStr(q, ", lc_collate = ");
- appendStringLiteralAH(q, collcollate, fout);+ appendStringLiteralAH(q, collcollate ? collcollate : "", fout);
appendPQExpBufferStr(q, ", lc_ctype = ");
- appendStringLiteralAH(q, collctype, fout);+ appendStringLiteralAH(q, collctype ? collctype : "", fout);
}
}
-- if (collicurules)- {- appendPQExpBufferStr(q, ", rules = ");- appendStringLiteralAH(q, collicurules, fout);- }+ else+ pg_fatal("unrecognized collation provider '%c'", collprovider[0]);
/*
* For binary upgrade, carry over the collation version. For normal
diff --git a/src/bin/pg_dump/t/002_pg_dump.pl b/src/bin/pg_dump/t/002_pg_dump.plindex 15852188c4..12815109f8 100644--- a/src/bin/pg_dump/t/002_pg_dump.pl+++ b/src/bin/pg_dump/t/002_pg_dump.pl@@ -4734,6 +4734,14 @@ $node->command_fails_like(
qr/pg_dumpall: error: improper qualified name \(too many dotted names\): myhost\.mydb/,
'pg_dumpall: option --exclude-database rejects multipart database names');
+##############################################################+# Test dumping pg_catalog (for research -- cannot be reloaded)++$node->command_ok(+ [ 'pg_dump', '-p', "$port", '-n', 'pg_catalog' ],+ 'pg_dump: option -n pg_catalog'+);+
#########################################
# Test valid database exclusion patterns
--
2.34.1
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: pgsql@j-davis.com, michael@paquier.xyz
Subject: Re: pg_dump assertion failure with "-n pg_catalog"
In-Reply-To: <e7c1adfe66542eca348de7485990abfabdbc1086.camel@j-davis.com>
* 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