Обсуждение: [BUGS] pg_dump 9.6 doesn't honour pg_extension_config_dump for sequences

pg_extension_config_dump() doesn't appear working on sequences; the
resulting dump doesn't contain the state of the sequence. Tested on pg
9.6.1 on Ubuntu. This seems a regression from pg 9.5.


In order to reproduce: create an extension:

piro@risotto:~$ cat /usr/share/postgresql/9.6/extension/testseq.control
# testseq extension
comment = 'test extension for sequence dump'
default_version = '1.0'
module_pathname = '$libdir/testseq'
relocatable = true

piro@risotto:~$ cat /usr/share/postgresql/9.6/extension/testseq--1.0.sql
create table testseq (id serial primary key, data text);

SELECT pg_catalog.pg_extension_config_dump('testseq', '');
SELECT pg_catalog.pg_extension_config_dump('testseq_id_seq', '');


Create the extension in a database and populate some rows:

piro@risotto:~$ sudo -u postgres psql -p 54396
psql (9.6.1)
Type "help" for help.

postgres=# create database test;
CREATE DATABASE
postgres=# \c test
You are now connected to database "test" as user "postgres".
test=# create schema testseq;
CREATE SCHEMA
test=# create extension testseq with schema testseq;
CREATE EXTENSION
test=# insert into testseq.testseq (data) values ('foo');
INSERT 0 1


Create a second database and copy the data:

piro@risotto:~$ sudo -u postgres psql -p 54396 -c "create database test2"
CREATE DATABASE
piro@risotto:~$ sudo -u postgres /usr/lib/postgresql/9.6/bin/pg_dump
-p 54396 test | sudo -u postgres /usr/lib/postgresql/9.6/bin/psql -p
54396 test2
SET
SET
SET
SET
SET
SET
SET
SET
CREATE SCHEMA
ALTER SCHEMA
CREATE EXTENSION
COMMENT
CREATE EXTENSION
COMMENT
SET
COPY 1


The table data was copied but the sequence is in an inconsistent state:

piro@risotto:~$ sudo -u postgres psql -p 54396 -c "insert into
testseq.testseq (data) values ('bar');" test2
ERROR:  duplicate key value violates unique constraint "testseq_pkey"
DETAIL:  Key (id)=(1) already exists.


-- Daniele


-- 
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

* Daniele Varrazzo (daniele.varrazzo@gmail.com) wrote:
> pg_extension_config_dump() doesn't appear working on sequences; the
> resulting dump doesn't contain the state of the sequence. Tested on pg
> 9.6.1 on Ubuntu. This seems a regression from pg 9.5.

Thanks for the detailed bug report, I'll take a look at it.

Stephen

On Wed, Jan 18, 2017 at 2:24 PM, Stephen Frost <sfrost@snowman.net> wrote:
> * Daniele Varrazzo (daniele.varrazzo@gmail.com) wrote:
>> pg_extension_config_dump() doesn't appear working on sequences; the
>> resulting dump doesn't contain the state of the sequence. Tested on pg
>> 9.6.1 on Ubuntu. This seems a regression from pg 9.5.
>
> Thanks for the detailed bug report, I'll take a look at it.

No problem. A detail maybe not clear (it's in the title, not in the
email): the error should be in pg_dump, not in the
pg_extension_config_dump() function, as I've found it using 9.6
pg_dump against a 9.5 database.

-- Daniele


-- 
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

* Daniele Varrazzo (daniele.varrazzo@gmail.com) wrote:
> On Wed, Jan 18, 2017 at 2:24 PM, Stephen Frost <sfrost@snowman.net> wrote:
> > * Daniele Varrazzo (daniele.varrazzo@gmail.com) wrote:
> >> pg_extension_config_dump() doesn't appear working on sequences; the
> >> resulting dump doesn't contain the state of the sequence. Tested on pg
> >> 9.6.1 on Ubuntu. This seems a regression from pg 9.5.
> >
> > Thanks for the detailed bug report, I'll take a look at it.
>
> No problem. A detail maybe not clear (it's in the title, not in the
> email): the error should be in pg_dump, not in the
> pg_extension_config_dump() function, as I've found it using 9.6
> pg_dump against a 9.5 database.

Right, it's most likely something to do with the reworking of pg_dump
that I did for 9.6.

Thanks!

Stephen

On Wed, Jan 18, 2017 at 11:45 PM, Stephen Frost <sfrost@snowman.net> wrote:
> * Daniele Varrazzo (daniele.varrazzo@gmail.com) wrote:
>> On Wed, Jan 18, 2017 at 2:24 PM, Stephen Frost <sfrost@snowman.net> wrote:
>> > * Daniele Varrazzo (daniele.varrazzo@gmail.com) wrote:
>> >> pg_extension_config_dump() doesn't appear working on sequences; the
>> >> resulting dump doesn't contain the state of the sequence. Tested on pg
>> >> 9.6.1 on Ubuntu. This seems a regression from pg 9.5.
>> >
>> > Thanks for the detailed bug report, I'll take a look at it.
>>
>> No problem. A detail maybe not clear (it's in the title, not in the
>> email): the error should be in pg_dump, not in the
>> pg_extension_config_dump() function, as I've found it using 9.6
>> pg_dump against a 9.5 database.
>
> Right, it's most likely something to do with the reworking of pg_dump
> that I did for 9.6.

As far as I can see, the code path to dumpSequenceData() is taken
correctly for such a sequence. processExtensionTables() creates a
dataObj using makeTableDataInfo() only for sequences in extensions
that are dumpable. So I have been wondering what's happening for some
time, until I noticed that:
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -15671,7 +15671,7 @@ dumpSequenceData(Archive *fout, TableDataInfo *tdinfo)
    appendPQExpBuffer(query, ", %s, %s);\n",
                      last, (called ? "true" : "false"));

-   if (tbinfo->dobj.dump & DUMP_COMPONENT_DATA)
+   if (tdinfo->dobj.dump & DUMP_COMPONENT_DATA)
        ArchiveEntry(fout, nilCatalogId, createDumpId(),
                     tbinfo->dobj.name,
                     tbinfo->dobj.namespace->dobj.name,

Not sure if I need to laugh or cry. Attached is a patch with the fix
and new regression tests.
-- 
Michael

-- 
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

Michael,

* Michael Paquier (michael.paquier@gmail.com) wrote:
> As far as I can see, the code path to dumpSequenceData() is taken
> correctly for such a sequence. processExtensionTables() creates a
> dataObj using makeTableDataInfo() only for sequences in extensions
> that are dumpable. So I have been wondering what's happening for some
> time, until I noticed that:

> --- a/src/bin/pg_dump/pg_dump.c
> +++ b/src/bin/pg_dump/pg_dump.c
> @@ -15671,7 +15671,7 @@ dumpSequenceData(Archive *fout, TableDataInfo *tdinfo)
>     appendPQExpBuffer(query, ", %s, %s);\n",
>                       last, (called ? "true" : "false"));
>
> -   if (tbinfo->dobj.dump & DUMP_COMPONENT_DATA)
> +   if (tdinfo->dobj.dump & DUMP_COMPONENT_DATA)
>         ArchiveEntry(fout, nilCatalogId, createDumpId(),
>                      tbinfo->dobj.name,
>                      tbinfo->dobj.namespace->dobj.name,

> Not sure if I need to laugh or cry. Attached is a patch with the fix
> and new regression tests.

Yeah, that looks like the right answer and matches how dumpTableData
handles this, which is also why that's working correctly.

I don't particularly like this arrangement though.  This works because:

getTables() will create the entry for the sequence and call
selectDumpableTable(), which calls checkExtensionMembership(), which
will set the dump flags to ACL | SECLABEL | POLICY, and then
getTableData() will skip adding a data entry for it because DATA isn't
set, allowing processExtensionTables() to add the DATA entry later for
config tables/sequences.

Now that we've got the ability to indicate which pieces of a object
should be dumped, however, it seems like we could just check in
checkExtensionMembership() if it's a config table and set DATA then if
it is, possibly removing the need for processExtensionTables() entirely.

That's really future-work, but I wanted to mention my thoughts here in
case someone wants to work on it, or I might once I've cleared off the
other things on my plate.  For now, I'll go ahead and commit your
suggested fix, and thanks for including a new regression test to cover
this.

Thanks again!

Stephen

Michael,

* Michael Paquier (michael.paquier@gmail.com) wrote:
> Not sure if I need to laugh or cry. Attached is a patch with the fix
> and new regression tests.

Thanks, I've pushed this now.

Daniele, the fix should be released with 9.6.2, or you can use Michael's
patch or pull what I committed (bec96c8) and try it out.

Thanks again!

Stephen

On Thu, Jan 19, 2017 at 10:46 PM, Stephen Frost <sfrost@snowman.net> wrote:
> Yeah, that looks like the right answer and matches how dumpTableData
> handles this, which is also why that's working correctly.
>
> I don't particularly like this arrangement though.  This works because:
>
> getTables() will create the entry for the sequence and call
> selectDumpableTable(), which calls checkExtensionMembership(), which
> will set the dump flags to ACL | SECLABEL | POLICY, and then
> getTableData() will skip adding a data entry for it because DATA isn't
> set, allowing processExtensionTables() to add the DATA entry later for
> config tables/sequences.

Yes, I certainly agree with that. When looking at this bug the first
thing I did was to set DUMP_COMPONENT_DATA in
checkExtensionMembership() this way actually to see that nothing
happened. It would be also better to have a sanity check like
(tbinfo->obj.dataObj != NULL && (tbinfo->dump & DUMP_COMPONENT_DATA)
!= 0) in both the sequence and table data dump paths, and use tbinfo
instead of tdinfo to do this work. This way we check the dependency
between the DATA flag set and the created dataObj by
makeTableInfoData().

> Now that we've got the ability to indicate which pieces of a object
> should be dumped, however, it seems like we could just check in
> checkExtensionMembership() if it's a config table and set DATA then if
> it is, possibly removing the need for processExtensionTables() entirely.

LIkely so.

> That's really future-work, but I wanted to mention my thoughts here in
> case someone wants to work on it, or I might once I've cleared off the
> other things on my plate.  For now, I'll go ahead and commit your
> suggested fix, and thanks for including a new regression test to cover
> this.

Thanks. There are still no tests for dumpable tables though.
-- 
Michael


-- 
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

* Michael Paquier (michael.paquier@gmail.com) wrote:
> On Thu, Jan 19, 2017 at 10:46 PM, Stephen Frost <sfrost@snowman.net> wrote:
> > Yeah, that looks like the right answer and matches how dumpTableData
> > handles this, which is also why that's working correctly.
> >
> > I don't particularly like this arrangement though.  This works because:
> >
> > getTables() will create the entry for the sequence and call
> > selectDumpableTable(), which calls checkExtensionMembership(), which
> > will set the dump flags to ACL | SECLABEL | POLICY, and then
> > getTableData() will skip adding a data entry for it because DATA isn't
> > set, allowing processExtensionTables() to add the DATA entry later for
> > config tables/sequences.
>
> Yes, I certainly agree with that. When looking at this bug the first
> thing I did was to set DUMP_COMPONENT_DATA in
> checkExtensionMembership() this way actually to see that nothing
> happened.

Nothing happened..?  getTableData() should have then created an entry
for it and then dump it out..  If that didn't happen then I'm kind of
curious as to why not.

> It would be also better to have a sanity check like
> (tbinfo->obj.dataObj != NULL && (tbinfo->dump & DUMP_COMPONENT_DATA)
> != 0) in both the sequence and table data dump paths, and use tbinfo
> instead of tdinfo to do this work. This way we check the dependency
> between the DATA flag set and the created dataObj by
> makeTableInfoData().

Well, yes, if we go the route that I suggested and make the tbinfo
actually have the DATA component set for these objects by
checkExtensionMembership().  We can't set that kind of a sanity check
now, of course.

> Thanks. There are still no tests for dumpable tables though.

Agreed.  I have a whole slew of additional regression tests slated for
the src/bin/pg_dump/t/ set of tests, I wouldn't complain if someone
wanted to work through adding more regression tests to
src/test/modules/test_pg_dump/t/ for extensions...

Otherwise, I'll get there once I've gotten the code coverage of pg_dump
*without* worrying about extensions up to something more reasonable.
Currently there are far too many cases that aren't covered (as evidenced
by the current rate of bug-fix back-patching that I'm doing..).

Thanks!

Stephen