pg.ddx.io pgsql-bugs@postgresql.org mailing list archive
help / color / mirror / Atom feedpg_dump assertion failure with "-n pg_catalog"
8+ messages / 3 participants
[nested] [flat]
Commits from this thread:
2023-08-22 1d9976d1bd 501704e826 37188cea0c Fix pg_dump assertion failure when dumping pg_catalog.
* pg_dump assertion failure with "-n pg_catalog"
@ 2023-06-07 17:36 Jeff Davis <pgsql@j-davis.com>
0 siblings, 1 reply; 8+ messages in thread
From: Jeff Davis @ 2023-06-07 17:36 UTC (permalink / raw)
To: pgsql-bugs
On version 15 or later:
pg_dump -n pg_catalog postgres > /dev/null
pg_dump: pg_dump.c:13291: dumpCollation: Assertion `collcollate !=
((void *)0)' failed.
Patch attached.
I tried adding a test, but dumping pg_catalog seems to only be
supported for research purposes, so it's not clear what the test should
be testing.
If we went almost two release cycles without anyone noticing, then
perhaps we should just get rid of the ability to dump pg_catalog. But I
think the attached patch clarifies things regardless.
--
Jeff Davis
PostgreSQL Contributor Team - AWS
Attachments:
0001-Fix-pg_dump-assertion-failure-when-dumping-pg_catalo.patch (text/x-patch, 1.9K) download
From 2359f3244cc30bbc3353b8644e25879d8ae87c20 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] Fix pg_dump assertion failure when dumping pg_catalog.
---
src/bin/pg_dump/pg_dump.c | 33 +++++++++++++++++++++++++--------
1 file changed, 25 insertions(+), 8 deletions(-)
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 5dab1ba9ea..2a6a321609 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -13446,13 +13446,34 @@ 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')
{
+ Assert(colliculocale == NULL);
+ Assert(collicurules == NULL);
+ Assert(collcollate == NULL);
+ Assert(collctype == NULL);
+
+ /* no locale -- the default collation cannot be reloaded anyway */
+ }
+ else if (collprovider[0] == 'i')
+ {
+ Assert(colliculocale != NULL);
+ Assert(collcollate == NULL);
+ Assert(collctype == NULL);
+
appendPQExpBufferStr(q, ", locale = ");
appendStringLiteralAH(q, colliculocale, fout);
+
+ if (collicurules)
+ {
+ appendPQExpBufferStr(q, ", rules = ");
+ appendStringLiteralAH(q, collicurules, fout);
+ }
}
- else
+ else if (collprovider[0] == 'c')
{
+ Assert(colliculocale == NULL);
+ Assert(collicurules == NULL);
Assert(collcollate != NULL);
Assert(collctype != NULL);
@@ -13469,12 +13490,8 @@ dumpCollation(Archive *fout, const CollInfo *collinfo)
appendStringLiteralAH(q, 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
--
2.34.1
* Re: pg_dump assertion failure with "-n pg_catalog"
@ 2023-06-12 06:52 Michael Paquier <michael@paquier.xyz>
parent: Jeff Davis <pgsql@j-davis.com>
0 siblings, 1 reply; 8+ messages in thread
From: Michael Paquier @ 2023-06-12 06:52 UTC (permalink / raw)
To: Jeff Davis <pgsql@j-davis.com>; +Cc: pgsql-bugs
On Wed, Jun 07, 2023 at 10:36:29AM -0700, Jeff Davis wrote:
> If we went almost two release cycles without anyone noticing, then
> perhaps we should just get rid of the ability to dump pg_catalog. But I
> think the attached patch clarifies things regardless.
That looks correct for v15 and HEAD, not when dumping from older
versions.
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.
colliculocale and collicurules don't exist there, but the code is
forcing the value to NULL as PQfnumber() returns -1 for an attribute
that does not exist.
else if (collprovider[0] == 'i')
{
+ Assert(colliculocale != NULL);
+ Assert(collcollate == NULL);
+ Assert(collctype == NULL);
If ICU provides the collation, colliculocale is NULL when dumping from
~14, while both collcollate and collctype are not NULL. Hence all
three assertions are incorrect for ~14, while they are correct for
15~. It seems to me that you are going to require a per-version
handling here if you wish to keep collprovider on top of the rest.
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.
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: pg_dump assertion failure with "-n pg_catalog"
@ 2023-06-15 22:47 Jeff Davis <pgsql@j-davis.com>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 2 replies; 8+ messages in thread
From: Jeff Davis @ 2023-06-15 22:47 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: pgsql-bugs
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.c
index 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.pl
index 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
* Re: pg_dump assertion failure with "-n pg_catalog"
@ 2023-06-16 05:29 Michael Paquier <michael@paquier.xyz>
parent: Jeff Davis <pgsql@j-davis.com>
1 sibling, 1 reply; 8+ messages in thread
From: Michael Paquier @ 2023-06-16 05:29 UTC (permalink / raw)
To: Jeff Davis <pgsql@j-davis.com>; +Cc: pgsql-bugs
On Thu, Jun 15, 2023 at 03:47:59PM -0700, Jeff Davis wrote:
> The new patch does add about 40 net lines, though.
I have looked at dumps generated from servers down to 9.2, and these
seemed OK.
> 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?
Not sure to agree about this point. The first version where this
happens is v15, meaning that two branches are impacted currently. So
the cost of backpatch is minimal.
> 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.
Right, there are 14 of them, except _record. This is actually much,
much older than the collation bits.
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: pg_dump assertion failure with "-n pg_catalog"
@ 2023-06-16 17:29 Jeff Davis <pgsql@j-davis.com>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 8+ messages in thread
From: Jeff Davis @ 2023-06-16 17:29 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: pgsql-bugs
On Fri, 2023-06-16 at 14:29 +0900, Michael Paquier wrote:
> Not sure to agree about this point. The first version where this
> happens is v15, meaning that two branches are impacted currently. So
> the cost of backpatch is minimal.
OK, I will plan to commit and backport early next week unless others
have additional input.
Regards,
Jeff Davis
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: pg_dump assertion failure with "-n pg_catalog"
@ 2023-08-22 20:10 Jeff Davis <pgsql@j-davis.com>
parent: Jeff Davis <pgsql@j-davis.com>
0 siblings, 0 replies; 8+ messages in thread
From: Jeff Davis @ 2023-08-22 20:10 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: pgsql-bugs
On Fri, 2023-06-16 at 10:29 -0700, Jeff Davis wrote:
> OK, I will plan to commit and backport early next week unless others
> have additional input.
Done. Sorry for the long delay -- lost track of this one.
Regards,
Jeff Davis
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: pg_dump assertion failure with "-n pg_catalog"
@ 2023-09-05 09:45 Peter Eisentraut <peter@eisentraut.org>
parent: Jeff Davis <pgsql@j-davis.com>
1 sibling, 1 reply; 8+ messages in thread
From: Peter Eisentraut @ 2023-09-05 09:45 UTC (permalink / raw)
To: Jeff Davis <pgsql@j-davis.com>; Michael Paquier <michael@paquier.xyz>; +Cc: pgsql-bugs
On 16.06.23 00:47, Jeff Davis wrote:
> 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);
> }
>
This new code replaces an (unexpected) null value with an empty string.
Is that correct? Empty locale strings have specific behaviors under the
libc and icu providers. It seems to me it would be better to error out
or don't print a CREATE COLLATION command at all rather than
substituting a value that is not a correct substitute.
Alternatively, a more correct way to produce a null value would be to
not issue the assignment at all (omit the " lc_collation = " etc.).
Then we can leave it up to the backend to accept the command or not, but
at least the dump closely reflects the original catalog state.
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: pg_dump assertion failure with "-n pg_catalog"
@ 2023-09-05 22:33 Jeff Davis <pgsql@j-davis.com>
parent: Peter Eisentraut <peter@eisentraut.org>
0 siblings, 0 replies; 8+ messages in thread
From: Jeff Davis @ 2023-09-05 22:33 UTC (permalink / raw)
To: Peter Eisentraut <peter@eisentraut.org>; Michael Paquier <michael@paquier.xyz>; +Cc: pgsql-bugs
On Tue, 2023-09-05 at 11:45 +0200, Peter Eisentraut wrote:
> This new code replaces an (unexpected) null value with an empty
> string.
> Is that correct? Empty locale strings have specific behaviors under
> the
> libc and icu providers. It seems to me it would be better to error
> out
> or don't print a CREATE COLLATION command at all rather than
> substituting a value that is not a correct substitute.
It already issues a warning, which seemed more consistent with other
unexpected situations in pg_dump.
> Alternatively, a more correct way to produce a null value would be to
> not issue the assignment at all (omit the " lc_collation = " etc.).
> Then we can leave it up to the backend to accept the command or not,
> but
> at least the dump closely reflects the original catalog state.
The backend does not accept a collation specified with lc_ctype and not
lc_collate (or vice versa), so it would be an un-restorable dump. That
doesn't seem great.
Regards,
Jeff Davis
^ permalink raw reply [nested|flat] 8+ messages in thread
end of thread, other threads:[~2023-09-05 22:33 UTC | newest]
Thread overview: 8+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2023-06-07 17:36 pg_dump assertion failure with "-n pg_catalog" Jeff Davis <pgsql@j-davis.com>
2023-06-12 06:52 ` Michael Paquier <michael@paquier.xyz>
2023-06-15 22:47 ` Jeff Davis <pgsql@j-davis.com>
2023-06-16 05:29 ` Michael Paquier <michael@paquier.xyz>
2023-06-16 17:29 ` Jeff Davis <pgsql@j-davis.com>
2023-08-22 20:10 ` Jeff Davis <pgsql@j-davis.com>
2023-09-05 09:45 ` Peter Eisentraut <peter@eisentraut.org>
2023-09-05 22:33 ` Jeff Davis <pgsql@j-davis.com>
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