Fix possible crash in pg_dump with identity sequences.

If an owned sequence is considered interesting, force its owning
table to be marked interesting too.  This ensures, in particular,
that we'll fetch the owning table's column names so we have the
data needed for ALTER TABLE ... ADD GENERATED.  Previously there were
edge cases where pg_dump could get SIGSEGV due to not having filled in
the column names.  (The known case is where the owning table has been
made part of an extension while its identity sequence is not a member;
but there may be others.)

Also, if it's an identity sequence, force its dumped-components mask
to exactly match the owning table: dump definition only if we're
dumping the table's definition, dump data only if we're dumping the
table's data, etc.  This generalizes the code introduced in commit
b965f2617 that set the sequence's dump mask to NONE if the owning
table's mask is NONE.  That's insufficient to prevent failures,
because for example the table's mask might only request dumping ACLs,
which would lead us to still emit ALTER TABLE ADD GENERATED even
though we didn't create the table.  It seems better to treat an
identity sequence as though it were an inseparable aspect of the
table, matching the treatment used in the backend's dependency logic.
Perhaps this policy needs additional refinement, but let's wait to
see some field use-cases before changing it further.

While here, add a comment in pg_dump.h warning against writing tests
like "if (dobj->dump == DUMP_COMPONENT_NONE)", which was a bug in this
case.  There is one other example in getPublicationNamespaces, which
if it's not a bug is at least remarkably unclear and under-documented.
Changing that requires a separate discussion, however.

Per report from Artur Zakirov.  Back- to all supported branches.

Discussion: https://postgr.es/m/CAKNkYnwXFBf136=u9UqUxFUVagevLQJ=zGd5BsLhCsatDvQsKQ@mail.gmail.com

diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index d34b8ed4bb12d12e380ec4dfc6ab16955dd674b8..19969e400fca63b930e06a3cb38301f2b59e249e 100644 (file)
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -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.h
index 2e55a0e3bbb386d1e134de072a6d0bf2682d9df7..9c5ddd20cf705d2c5fa865dc6d5f47e096bb822c 100644 (file)
--- 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)