Re: pg_dump crash on identity sequence with not loaded attributes

Artur Zakirov <zaartur(at)gmail(dot)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