Thread: Re: pg_dump crash on identity sequence with not loaded attributes
Artur Zakirov <zaartur@gmail.com> writes:
> pg_dump crashes when a table is added into an extension and its
> identity sequence isn't.
Yup, reproduced here.
> The attached patch fixes the issue on pg_dump side by ignoring
> identity sequences if their table attributes are not loaded. I'm not
> sure if this is the best fix.
I don't like it much. That test is trying to avoid exactly this
situation; why is it failing to do so? I traced through it with
gdb and found that when we get here,
(gdb) s
7259 if (owning_tab->dobj.dump == DUMP_COMPONENT_NONE &&
(gdb) p owning_tab->dobj.dump
$7 = 16
(gdb) p seqinfo->is_identity_sequence
$8 = true
(gdb) p owning_tab->interesting
$9 = false
That is, the owning table is marked to dump ACLs and nothing else.
Per getTables(), we only mark a table interesting if we need to
dump its DEFINITION or DATA component. So kaboom.
I didn't check the git history, but I suspect that this code was
correct when written and got broken by init_privs changes, which
allowed the DUMP_COMPONENT_ACL flag to get set along with nothing
else (cf checkExtensionMembership). I'm inclined to fix it
like this:
/*
* Only dump identity sequences if we're going to dump the table that
* it belongs to.
*/
- if (owning_tab->dobj.dump == DUMP_COMPONENT_NONE &&
+ if ((owning_tab->dobj.dump & DUMP_COMPONENT_DEFINITION) &&
seqinfo->is_identity_sequence)
{
seqinfo->dobj.dump = DUMP_COMPONENT_NONE;
(The comment could use a bit of adjustment too.) The idea here is
that so far as the user is concerned, an identity sequence is part
of the DDL that defines the table, so it should be dumped if and
only if we're dumping the table's definitional DDL. Ancillary
stuff like ACLs shouldn't change this conclusion.
A different approach that we could take is to decide that by golly,
we're supposed to dump this sequence and we should do so. That
could be done by forcing owning_tab->interesting to true just below
here. However, in that case the entire bit quoted above seems wrong,
because we'd no longer be using the policy stated by the comment.
Certainly it still shouldn't matter whether the table is by chance
marked to dump ACLs.
I poked around for other places that might have a similar disease.
The only other direct test like "foo == DUMP_COMPONENT_NONE" is in
getPublicationNamespaces:
/*
* We always dump publication namespaces unless the corresponding
* namespace is excluded from the dump.
*/
if (nspinfo->dobj.dump == DUMP_COMPONENT_NONE)
continue;
I didn't try to break that but I suspect it is equally wrong, ie
it'll behave surprisingly differently for schemas that belong to
extensions vs. those that don't. I'm less sure what to do here.
It seems like publishing a namespace that belongs to an extension
might not be an unreasonable thing to do, in which case users
would be sad if we suppressed it from the publication's dump.
Perhaps this should be like
if (!((nspinfo->dobj.dump & DUMP_COMPONENT_DEFINITION) ||
(nspinfo->ext_member && extension-is-being-dumped)))
continue;
regards, tom lane
Artur Zakirov <zaartur@gmail.com> writes:
> On Mon, 9 Dec 2024 at 22:51, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> - if (owning_tab->dobj.dump == DUMP_COMPONENT_NONE &&
>> + if ((owning_tab->dobj.dump & DUMP_COMPONENT_DEFINITION) &&
> I think it is necessary to use negation in this condition.
D'oh, of course. But what's your thoughts on the other points?
Is this what we want to do at all?
regards, tom lane
On Tue, 10 Dec 2024 at 16:29, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > I think it is necessary to use negation in this condition.
>
> D'oh, of course. But what's your thoughts on the other points?
> Is this what we want to do at all?
Alternatively I was thinking about this change:
continue;
}
+ if (!(owning_tab->dobj.dump & DUMP_COMPONENT_DEFINITION) &&
+ seqinfo->is_identity_sequence)
+ seqinfo->dobj.dump &= ~DUMP_COMPONENT_DEFINITION;
+
/*
* Otherwise we need to dump the components that are
being dumped for
* the table and any components which the sequence is explicitly
This way we wouldn't ignore ACLs for example. Currently a user might
not have permissions to a sequence but still be able to read/write to
its table if they have permissions. Although the user cannot execute
nextval/setval explicitly.
If an admin grants permissions to the sequence explicitly (for some
reason) the user will be able to execute nextval/setval. And that will
be lost during dump/restore. Currently this doesn't work at all and
ignoring sequences won't break anything.
Ideally I would like to have the ability to dump ACL of those
sequences too, even though this looks like a quite narrow use case.
Alternatively, instead of forcing owning_tab->interesting to true, I
think we could always initialize owning_tab's attributes (i.e. arrays
like owning_tab->attnames, owning_tab->attidenity), which are used by
dumpSequence() and which causes the crash. Are there any downsides of
it?
--
Kind regards,
Artur
Artur Zakirov <zaartur@gmail.com> writes:
> Alternatively, instead of forcing owning_tab->interesting to true, I
> think we could always initialize owning_tab's attributes (i.e. arrays
> like owning_tab->attnames, owning_tab->attidenity), which are used by
> dumpSequence() and which causes the crash. Are there any downsides of
> it?
Lots. The entire point of the ->interesting flag is to avoid fetching
additional details about tables that we don't really care about.
Unless I misunderstand, you're proposing throwing away that whole
optimization, which has got to be an overall loss.
regards, tom lane
On Tue, 10 Dec 2024 at 19:08, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Artur Zakirov <zaartur@gmail.com> writes: > > Alternatively, instead of forcing owning_tab->interesting to true, I > > think we could always initialize owning_tab's attributes (i.e. arrays > > like owning_tab->attnames, owning_tab->attidenity), which are used by > > dumpSequence() and which causes the crash. Are there any downsides of > > it? > > Lots. The entire point of the ->interesting flag is to avoid fetching > additional details about tables that we don't really care about. > Unless I misunderstand, you're proposing throwing away that whole > optimization, which has got to be an overall loss. Yeah, I rechecked the code and it seems getTableAttrs() is called later than getOwnedSeqs(). And we can set owning_tab->interesting to true to load data only of needed tables. 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. -- Kind regards, Artur
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)
On Tue, 10 Dec 2024 at 22:15, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 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;
Thank you for the new version of the patch. It looks good to me and it
works as expected after I tested it.
> 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.
I don't have a strong opinion about this part and I'm not sure that we
need to ignore mapping between a namespace and a publication if the
namespace belongs to the extension. Maybe we could always dump the
mapping if the publication itself is dumped.
But other than that it seems the patch breaks dumps of mapping of a
publication with the public namespace. The issue with the public
namespace is mentioned in the original thread of the feature of adding
schema mapping [1]. I couldn't find in the thread if it was addressed
in the end. With the new patch the following mapping won't be mapped:
create publication pub_test for tables in schema public;
pg_dump won't generate corresponding "ALTER PUBLICATION pub_test ADD
TABLES IN SCHEMA public".
1.
https://www.postgresql.org/message-id/OS0PR01MB61137F0B8498E9241F2D960EFB149%40OS0PR01MB6113.jpnprd01.prod.outlook.com
--
Kind regards,
Artur
Artur Zakirov <zaartur@gmail.com> writes:
> On Tue, 10 Dec 2024 at 22:15, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> 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.
> But other than that it seems the patch breaks dumps of mapping of a
> publication with the public namespace. The issue with the public
> namespace is mentioned in the original thread of the feature of adding
> schema mapping [1]. I couldn't find in the thread if it was addressed
> in the end. With the new patch the following mapping won't be mapped:
> create publication pub_test for tables in schema public;
Ouch. Okay, I agree that's probably bad, but then it raises the
question of why this filter exists at all. It certainly seems
100% magic and undocumented that it does the "right" thing for
these cases.
In any case, that's mostly orthogonal to the problem with identity
sequences. I now propose leaving out that change and just pushing
the change in getOwnedSeqs(). The publication-namespace business
should be discussed on its own thread.
regards, tom lane
On Wed, 11 Dec 2024 at 16:48, Tom Lane <tgl@sss.pgh.pa.us> wrote: > In any case, that's mostly orthogonal to the problem with identity > sequences. I now propose leaving out that change and just pushing > the change in getOwnedSeqs(). The publication-namespace business > should be discussed on its own thread. +1, agree it looks like a bit different issue and for now we can fix only the issue with sequences. I found another inconsistency with publishing mapping and just mention it here. According to the code of selectDumpableNamespace() namespaces "pg_*" and "information_schema" are ignored by pg_dump. But it is possible to create mapping for publications with "information_schema" and those mappings will be lost by dump/restore. But I agree that publication-namespace should have its own thread. -- Kind regards, Artur