Re: pg_dump crash on identity sequence with not loaded attributes - Mailing list pgsql-bugs

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

diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 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.h
index 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)