From: Tom Lane <tgl@sss.pgh.pa.us>
To: Artur Zakirov <zaartur@gmail.com>
cc: Peter Eisentraut <peter@eisentraut.org>,
        PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>
Subject: Re: pg_dump crash on identity sequence with not loaded attributes
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>
Comments: In-reply-to Artur Zakirov <zaartur@gmail.com>
	message dated "Tue, 10 Dec 2024 19:40:41 +0100"
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0"
Content-ID: <191243.1733865228.0@sss.pgh.pa.us>
Date: Tue, 10 Dec 2024 16:15:16 -0500
Message-ID: <191373.1733865316@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/191373.1733865316%40sss.pgh.pa.us>
Precedence: bulk

------- =_aaaaaaaaaa0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <191243.1733865228.1@sss.pgh.pa.us>

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


------- =_aaaaaaaaaa0
Content-Type: text/x-diff; name="fix-pg_dump-identity-sequence.patch";
	charset="us-ascii"
Content-ID: <191243.1733865228.2@sss.pgh.pa.us>
Content-Description: fix-pg_dump-identity-sequence.patch
Content-Transfer-Encoding: quoted-printable

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 =3D=3D 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[], i=
nt 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 =3D=3D DUMP_COMPONENT_NONE &&
-			seqinfo->is_identity_sequence)
-		{
-			seqinfo->dobj.dump =3D 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[], i=
nt numTables)
 		 * marked by checkExtensionMembership() and this will be a no-op as
 		 * the table will be equivalently marked.
 		 */
-		seqinfo->dobj.dump =3D seqinfo->dobj.dump | owning_tab->dobj.dump;
+		if (seqinfo->is_identity_sequence)
+			seqinfo->dobj.dump =3D owning_tab->dobj.dump;
+		else
+			seqinfo->dobj.dump |=3D owning_tab->dobj.dump;
 =

+		/* Make sure that necessary data is available if we're dumping it */
 		if (seqinfo->dobj.dump !=3D DUMP_COMPONENT_NONE)
+		{
 			seqinfo->interesting =3D true;
+			owning_tab->interesting =3D 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 value=
s,
+ * but be careful about using them in tests.  For example, a test like
+ * "if (dobj->dump =3D=3D DUMP_COMPONENT_NONE)" is probably wrong; you li=
kely want
+ * "if (!(dobj->dump & DUMP_COMPONENT_DEFINITION))" instead.  This is bec=
ause
+ * we aren't too careful about the values of irrelevant bits, as indeed c=
an 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, leadin=
g to
+ * unexpected behavior of a test against NONE.
  */
 typedef uint32 DumpComponents;
 #define DUMP_COMPONENT_NONE			(0)

------- =_aaaaaaaaaa0--