From: Tom Lane <tgl@sss.pgh.pa.us>
To: Artur Zakirov <zaartur@gmail.com>
Cc: Peter Eisentraut <peter@eisentraut.org>
Cc: PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>
Subject: Re: pg_dump crash on identity sequence with not loaded attributes
Date: Tue, 10 Dec 2024 16:15:16 -0500
Message-ID: <191373.1733865316@sss.pgh.pa.us> (raw)
In-Reply-To: <CAKNkYnz-uX2fk_V6bdp+KvFTCU8RtPG-k8MfMkgWRnM7nHoyQg@mail.gmail.com>
References: <CAKNkYnwXFBf136=u9UqUxFUVagevLQJ=zGd5BsLhCsatDvQsKQ@mail.gmail.com>
<4137493.1733781111@sss.pgh.pa.us>
<CAKNkYnwTypJPsc258ZwVJ+n_Ti-8VxiQ6Gy4oMPmCA9fKo+KKg@mail.gmail.com>
<66804.1733844592@sss.pgh.pa.us>
<CAKNkYnwR0iLr2YeB9dkDXqh1vXvCyx9VyYTxw_mppCbkjqwP0A@mail.gmail.com>
<84249.1733854089@sss.pgh.pa.us>
<CAKNkYnz-uX2fk_V6bdp+KvFTCU8RtPG-k8MfMkgWRnM7nHoyQg@mail.gmail.com>
Artur Zakirov <zaartur@gmail.com> writes:
> I think it will still be necessary to negate DUMP_COMPONENT_DEFINITION
> from seqinfo->dobj.dump because we shouldn't dump statements like ...
> ALTER COLUMN ... ADD GENERATED ..., if the table's definition isn't
> dumped.
Yeah. After chewing on this for awhile, I think the cleanest solution
is say that b965f2617 was just wrong, and we should revert it in
favor of adopting this logic:
if (seqinfo->is_identity_sequence)
seqinfo->dobj.dump = owning_tab->dobj.dump;
else
seqinfo->dobj.dump |= owning_tab->dobj.dump;
That is, for an identity sequence we should dump the exact same
properties as for its owning table. This basically rejects the
notion that a table can be inside an extension while its identity
sequence is not (or vice versa), which is the same position we
take for indexes, rules, etc. However, for non-identity owned
sequences (i.e. serials) keep the same behavior as before.
The large comment block just below here (added by f9e439b1c)
carefully describes the behavior that was wanted for non-identity
sequences, and I don't think we need to break that.
The attached patch also gets rid of the dubious coding in
getPublicationNamespaces. We might get push-back on that ignoring
schemas belonging to extensions, but if so I'd prefer to see the
behavior coded in a more transparent fashion.
regards, tom lane
Attachments:fix-pg_dump-identity-sequence.patch (text/x-diff, 3.5K) downloaddiff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.cindex ec0cdf4ed7..53dc5cb2b3 100644--- a/src/bin/pg_dump/pg_dump.c+++ b/src/bin/pg_dump/pg_dump.c@@ -4538,7 +4538,7 @@ getPublicationNamespaces(Archive *fout)
* We always dump publication namespaces unless the corresponding
* namespace is excluded from the dump.
*/
- if (nspinfo->dobj.dump == DUMP_COMPONENT_NONE)+ if (!(nspinfo->dobj.dump & DUMP_COMPONENT_DEFINITION))
continue;
/* OK, make a DumpableObject for this relationship */
@@ -7253,20 +7253,15 @@ getOwnedSeqs(Archive *fout, TableInfo tblinfo[], int numTables)
seqinfo->owning_tab, seqinfo->dobj.catId.oid);
/*
- * Only dump identity sequences if we're going to dump the table that- * it belongs to.- */- if (owning_tab->dobj.dump == DUMP_COMPONENT_NONE &&- seqinfo->is_identity_sequence)- {- seqinfo->dobj.dump = DUMP_COMPONENT_NONE;- continue;- }-- /*- * Otherwise we need to dump the components that are being dumped for- * the table and any components which the sequence is explicitly- * marked with.+ * For an identity sequence, dump exactly the same components for the+ * sequence as for the owning table. This is important because we+ * treat the identity sequence as an integral part of the table. For+ * example, there is not any DDL command that allows creation of such+ * a sequence independently of the table.+ *+ * For other owned sequences such as serial sequences, we need to dump+ * the components that are being dumped for the table and any+ * components that the sequence is explicitly marked with.
*
* We can't simply use the set of components which are being dumped
* for the table as the table might be in an extension (and only the
@@ -7279,10 +7274,17 @@ getOwnedSeqs(Archive *fout, TableInfo tblinfo[], int numTables)
* marked by checkExtensionMembership() and this will be a no-op as
* the table will be equivalently marked.
*/
- seqinfo->dobj.dump = seqinfo->dobj.dump | owning_tab->dobj.dump;+ if (seqinfo->is_identity_sequence)+ seqinfo->dobj.dump = owning_tab->dobj.dump;+ else+ seqinfo->dobj.dump |= owning_tab->dobj.dump;+ /* Make sure that necessary data is available if we're dumping it */
if (seqinfo->dobj.dump != DUMP_COMPONENT_NONE)
+ {
seqinfo->interesting = true;
+ owning_tab->interesting = true;+ }
}
}
diff --git a/src/bin/pg_dump/pg_dump.h b/src/bin/pg_dump/pg_dump.hindex 2e55a0e3bb..9c5ddd20cf 100644--- a/src/bin/pg_dump/pg_dump.h+++ b/src/bin/pg_dump/pg_dump.h@@ -89,8 +89,16 @@ typedef enum
/*
* DumpComponents is a bitmask of the potentially dumpable components of
* a database object: its core definition, plus optional attributes such
- * as ACL, comments, etc. The NONE and ALL symbols are convenient- * shorthands.+ * as ACL, comments, etc.+ *+ * The NONE and ALL symbols are convenient shorthands for assigning values,+ * but be careful about using them in tests. For example, a test like+ * "if (dobj->dump == DUMP_COMPONENT_NONE)" is probably wrong; you likely want+ * "if (!(dobj->dump & DUMP_COMPONENT_DEFINITION))" instead. This is because+ * we aren't too careful about the values of irrelevant bits, as indeed can be+ * seen in the definition of DUMP_COMPONENT_ALL. It's also possible that an+ * object has only subsidiary bits such as DUMP_COMPONENT_ACL set, leading to+ * unexpected behavior of a test against NONE.
*/
typedef uint32 DumpComponents;
#define DUMP_COMPONENT_NONE (0)
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: tgl@sss.pgh.pa.us, zaartur@gmail.com, peter@eisentraut.org, pgsql-bugs@lists.postgresql.org
Subject: Re: pg_dump crash on identity sequence with not loaded attributes
In-Reply-To: <191373.1733865316@sss.pgh.pa.us>
* 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