Re: pg_dumping extensions having sequences with 9.6beta3

Michael,

(dropping -general, not sure why that list was included...)

* Michael Paquier (michael.paquier@gmail.com) wrote:
> On Wed, Jul 27, 2016 at 8:07 AM, Stephen Frost <sfrost@snowman.net> wrote:
> > That'd be great.  It's definitely on my list of things to look into, but
> > I'm extremely busy this week.  I hope to look into it on Friday, would
> > be great to see what you find.
>
> Sequences that are directly defined in extensions do not get dumped,
> and sequences that are part of a serial column in an extension are
> getting dumped. Looking into this problem, getOwnedSeqs() is visibly
> doing an incorrect assumption: sequences owned by table columns are
> dumped unconditionally, but this is not true for sequences that are
> part of extensions. More precisely, dobj->dump is being enforced to
> DUMP_COMPONENT_ALL, which makes the sequence definition to be dumped.
> Oops.

Right, it should be set to the same value as the table which is being
dumped rather than DUMP_COMPONENT_ALL.  Moreover, the
owning_tab->dobj.dump check should explicitly check against
DUMP_COMPONENT_NONE, but that's just to be neat since that's '0'.

> The patch attached fixes the problem for me. I have added as well
> tests in test_pg_dump in the shape of sequences defined in an
> extension, and sequences that are part of a serial column. This patch
> is also able to work in the case where a sequence is created as part
> of a serial column, and gets removed after, say with ALTER EXTENSION
> DROP SEQUENCE. The behavior for sequences and serial columns that are
> not part of extensions is unchanged.

This isn't correct as the dump components which are independent of the
extension (ACLs, security labels, policies) won't be dumped.

Consider, for example, what happens if the user runs:

GRANT USAGE ON SEQUENCE <extension_sequence> TO role;

This wouldn't get dumped out with your patch, since we would decide that,
because the sequence is a member of the extension, that nothing about it
should be dumped, which isn't correct.

> Stephen, it would be good if you could check the correctness of this
> patch as you did all this refactoring of pg_dump to support catalog
> ACLs. I am sure by the way that checking for (owning_tab->dobj.dump &&
> DUMP_COMPONENT_DEFINITION) != 0 is not good because of for example the
> case of a serial column created in an extension where the sequence is
> dropped from the extension afterwards.

If the sequence is dropped from the extension then the sequence will be
ignored by checkExtensionMembership() during selectDumpableObject() and
the regular selectDumpableObject() code will handle marking the sequence
appropriately.

What we need to be doing here is combining the set of components that
the sequence has been marked with and the set of components that the
table has been marked with, not trying to figure out if the sequence is
a member of an extension or not and changing what to do in those cases,
that's checkExtensionMembership()'s job, really.

Attached is a patch implementing this and which passes the regression
tests you added and a couple that I added for the non-extension case.
I'm going to look at adding a few more regression tests and if I don't
come across any issues then I'll likely push the fix sometime tomorrow.

Comments welcome, of course.

Thanks!

Stephen