Обсуждение: Re: pg_dumping extensions having sequences with 9.6beta3

On Sat, Jul 23, 2016 at 01:40:01PM +0900, Michael Paquier wrote:
> On Fri, Jul 22, 2016 at 6:27 PM, Philippe BEAUDOIN <phb.emaj@free.fr> wrote:
> > I am currently playing with extensions. And I found a strange behaviour
> > change with 9.6beta2 and 3 when pg_dumping a database with an extension
> > having sequences. This looks like a bug, ... unless I did something wrong.
> > [...]
> > => as expected, with latest minor versions of postgres 9.1 to 9.5, the
> > sequences associated to the t1.c1 and t1.c3 columns are not dumped,
> >    while the sequence associated to t2.c1 is dumped.
> > => with 9.6beta3 (as with beta2), the 3 sequences are dumped.
>
> Thanks for the report! I haven't looked at the problem in details yet,
> but my guess is that this is owned by Stephen Frost. test_pg_dump does
> not cover sequences yet, it would be visibly good to get coverage for
> that. I am adding an open item as well.

[Action required within 72 hours.  This is a generic notification.]

The above-described topic is currently a PostgreSQL 9.6 open item.  Stephen,
since you committed the patch believed to have created it, you own this open
item.  If some other commit is more relevant or if this does not belong as a
9.6 open item, please let us know.  Otherwise, please observe the policy on
open item ownership[1] and send a status update within 72 hours of this
message.  Include a date for your subsequent status update.  Testers may
discover new open items at any time, and I want to plan to get them all fixed
well in advance of shipping 9.6rc1.  Consequently, I will appreciate your
efforts toward speedy resolution.  Thanks.

[1] http://www.postgresql.org/message-id/20160527025039.GA447393@tornado.leadboat.com

On Tue, Jul 26, 2016 at 4:50 PM, Noah Misch <noah@leadboat.com> wrote:
> [Action required within 72 hours.  This is a generic notification.]
>
> The above-described topic is currently a PostgreSQL 9.6 open item.  Stephen,
> since you committed the patch believed to have created it, you own this open
> item.  If some other commit is more relevant or if this does not belong as a
> 9.6 open item, please let us know.  Otherwise, please observe the policy on
> open item ownership[1] and send a status update within 72 hours of this
> message.  Include a date for your subsequent status update.  Testers may
> discover new open items at any time, and I want to plan to get them all fixed
> well in advance of shipping 9.6rc1.  Consequently, I will appreciate your
> efforts toward speedy resolution.  Thanks.
>
> [1] http://www.postgresql.org/message-id/20160527025039.GA447393@tornado.leadboat.com

I am not sure what's Stephen's status on this item, but I am planning
to look at it within the next 24 hours.
--
Michael

Michael,

* Michael Paquier (michael.paquier@gmail.com) wrote:
> On Tue, Jul 26, 2016 at 4:50 PM, Noah Misch <noah@leadboat.com> wrote:
> > [Action required within 72 hours.  This is a generic notification.]
> >
> > The above-described topic is currently a PostgreSQL 9.6 open item.  Stephen,
> > since you committed the patch believed to have created it, you own this open
> > item.  If some other commit is more relevant or if this does not belong as a
> > 9.6 open item, please let us know.  Otherwise, please observe the policy on
> > open item ownership[1] and send a status update within 72 hours of this
> > message.  Include a date for your subsequent status update.  Testers may
> > discover new open items at any time, and I want to plan to get them all fixed
> > well in advance of shipping 9.6rc1.  Consequently, I will appreciate your
> > efforts toward speedy resolution.  Thanks.
> >
> > [1] http://www.postgresql.org/message-id/20160527025039.GA447393@tornado.leadboat.com
>
> I am not sure what's Stephen's status on this item, but I am planning
> to look at it within the next 24 hours.

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.

Thanks!

Stephen

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.

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.

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.
--
Michael

On Wed, Jul 27, 2016 at 2:24 AM, 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.
>
> 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.
>
> 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.

Stephen, is this still on your list of things for today?  Please
provide a status update.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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

On Sat, Jul 30, 2016 at 5:47 AM, Stephen Frost <sfrost@snowman.net> wrote:
> 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.

OK, thanks for the confirmation. I have been playing a bit with your
patch and it is correctly dumping ACLs and policies that are modified
after creating an extension. So that looks good to me.

> 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.

+        * table will be equivilantly marked.
s/equivilantly/equivalently/.

By the way, I noticed 3 places in dumpProcLang and 1 place in
dumpSequence where dobj.dump is used in a test but it is not directly
compared with DUMP_COMPONENT_NONE. Perhaps you'd want to change that
as well..
-- 
Michael

Michael,

* Michael Paquier (michael.paquier@gmail.com) wrote:
> On Sat, Jul 30, 2016 at 5:47 AM, Stephen Frost <sfrost@snowman.net> wrote:
> > 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.
>
> OK, thanks for the confirmation. I have been playing a bit with your
> patch and it is correctly dumping ACLs and policies that are modified
> after creating an extension. So that looks good to me.
>
> > 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.
>
> +        * table will be equivilantly marked.
> s/equivilantly/equivalently/.
>
> By the way, I noticed 3 places in dumpProcLang and 1 place in
> dumpSequence where dobj.dump is used in a test but it is not directly
> compared with DUMP_COMPONENT_NONE. Perhaps you'd want to change that
> as well..

Thanks again for the help on this item, I've pushed the fix and updated
the open items wiki.

Stephen