pg_dump gets attributes from tables in extensions

On Mon, Feb 16, 2015 at 4:45 PM, Michael Paquier
<michael.paquier@gmail.com>wrote:

but not dumping any tables that are part of ext_member.

Excuse the typo => s/but/by/.
--
Michael

On 2/16/15 2:45 AM, Michael Paquier wrote:

While looking at the patch to fix pg_dump with extensions containing
tables referencing each other, I got surprised by the fact that
getTableAttrs tries to dump table attributes even for tables that are
part of an extension. Is that normal?
Attached is a patch that I think makes things right, but not dumping any
tables that are part of ext_member.

Can you provide an example/test case? (e.g., which publicly available
extension contains tables with attributes?)

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

On Fri, Feb 20, 2015 at 5:33 AM, Peter Eisentraut <peter_e@gmx.net> wrote:

On 2/16/15 2:45 AM, Michael Paquier wrote:

While looking at the patch to fix pg_dump with extensions containing
tables referencing each other, I got surprised by the fact that
getTableAttrs tries to dump table attributes even for tables that are
part of an extension. Is that normal?
Attached is a patch that I think makes things right, but not dumping any
tables that are part of ext_member.

Can you provide an example/test case? (e.g., which publicly available
extension contains tables with attributes?)

Sure. Attached is a simplified version of the extension I used for the
other patch on pg_dump.
$ psql -c 'create extension dump_test'
CREATE EXTENSION
$ psql -At -c '\dx+ dump_test'
table aa_tab_fkey
table bb_tab_fkey
$ pg_dump -v 2>&1 | grep "columns and types"
pg_dump: finding the columns and types of table "public"."bb_tab_fkey"
pg_dump: finding the columns and types of table "public"."aa_tab_fkey"
--
Michael

Thanks to the easy handy testcase, was able to replicate the test scenario
on my local environment. And yes tbinfo->dobj.ext_member check into
getTableAttrs() do fix the issue.

Looking more into pg_dump code what I found that, generally PG don't have
tbinfo->dobj.ext_member check to ignore the object. Mostly we do this kind
of check using tbinfo->dobj.dump (look at dumpTable() for reference). Do you
have any particular reason if choosing dobj.ext_member over dobj.dump ?

On Fri, Feb 20, 2015 at 12:20 PM, Michael Paquier <michael.paquier@gmail.com

wrote:

On Fri, Feb 20, 2015 at 5:33 AM, Peter Eisentraut <peter_e@gmx.net> wrote:

On 2/16/15 2:45 AM, Michael Paquier wrote:

While looking at the patch to fix pg_dump with extensions containing
tables referencing each other, I got surprised by the fact that
getTableAttrs tries to dump table attributes even for tables that are
part of an extension. Is that normal?
Attached is a patch that I think makes things right, but not dumping any
tables that are part of ext_member.

Can you provide an example/test case? (e.g., which publicly available
extension contains tables with attributes?)

Sure. Attached is a simplified version of the extension I used for the
other patch on pg_dump.
$ psql -c 'create extension dump_test'
CREATE EXTENSION
$ psql -At -c '\dx+ dump_test'
table aa_tab_fkey
table bb_tab_fkey
$ pg_dump -v 2>&1 | grep "columns and types"
pg_dump: finding the columns and types of table "public"."bb_tab_fkey"
pg_dump: finding the columns and types of table "public"."aa_tab_fkey"
--
Michael

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

--
Rushabh Lathia

On Mon, Feb 23, 2015 at 5:57 PM, Rushabh Lathia <rushabh.lathia@gmail.com>
wrote:

Thanks to the easy handy testcase, was able to replicate the test scenario
on my local environment. And yes tbinfo->dobj.ext_member check into
getTableAttrs() do fix the issue.

Looking more into pg_dump code what I found that, generally PG don't have
tbinfo->dobj.ext_member check to ignore the object. Mostly we do this kind
of check using tbinfo->dobj.dump (look at dumpTable() for reference). Do

you

have any particular reason if choosing dobj.ext_member over dobj.dump ?

Hm. I have been pondering about this code more and I am dropping the patch
as either adding a check based on the flag to track dumpable objects or
ext_member visibly breaks the logic that binary upgrade and tables in
extensions rely on. Particularly this portion makes me think so in
getExtensionMembership():
/*
* Normally, mark the member object as not to be dumped.
But in
* binary upgrades, we still dump the members individually,
since the
* idea is to exactly reproduce the database contents
rather than
* replace the extension contents with something different.
*/
if (!dopt->binary_upgrade)
dobj->dump = false;
else
dobj->dump = refdobj->dump;
Regards,
--
Michael

On Mon, Feb 23, 2015 at 7:45 PM, Michael Paquier <michael.paquier@gmail.com>
wrote:

On Mon, Feb 23, 2015 at 5:57 PM, Rushabh Lathia <rushabh.lathia@gmail.com>
wrote:

Thanks to the easy handy testcase, was able to replicate the test

scenario

on my local environment. And yes tbinfo->dobj.ext_member check into
getTableAttrs() do fix the issue.

Looking more into pg_dump code what I found that, generally PG don't have
tbinfo->dobj.ext_member check to ignore the object. Mostly we do this

kind

of check using tbinfo->dobj.dump (look at dumpTable() for reference). Do

you

have any particular reason if choosing dobj.ext_member over dobj.dump ?

Hm. I have been pondering about this code more and I am dropping the patch
as either adding a check based on the flag to track dumpable objects or
ext_member visibly breaks the logic that binary upgrade and tables in
extensions rely on. Particularly this portion makes me think so in
getExtensionMembership():
/*
* Normally, mark the member object as not to be dumped.
But in
* binary upgrades, we still dump the members
individually, since the
* idea is to exactly reproduce the database contents
rather than
* replace the extension contents with something different.
*/
if (!dopt->binary_upgrade)
dobj->dump = false;
else
dobj->dump = refdobj->dump;

Ok. Looking at above code into getExtensionMembership(). It seems like you
fix you suggested is not correct. I new table with DEFAULT attribute into
dump_test extension and pg_dump with binary-upgrade is failing with
pg_dump: invalid column number 1 for table "bb_tab_fkey".

rushabh@rushabh-centos-vm:dump_test$ cat dump_test--1.0.sql
/* dump_test/dump_test--1.0.sql */

-- complain if script is sourced in psql, rather than via CREATE EXTENSION
\echo Use "CREATE EXTENSION dump_test" to load this file. \quit

CREATE TABLE IF NOT EXISTS bb_tab_fkey (
id int PRIMARY KEY
);

CREATE TABLE IF NOT EXISTS aa_tab_fkey (
id int REFERENCES bb_tab_fkey(id)
);

CREATE TABLE IF NOT EXISTS foo ( a int default 100 );

This gave another strong reason to add if (!tbinfo->dobj.dump) check rather
then ext_member check. What you say ?

Regards,

On Tue, Feb 24, 2015 at 3:13 PM, Rushabh Lathia <rushabh.lathia@gmail.com>
wrote:

Ok. Looking at above code into getExtensionMembership(). It seems like you
fix you suggested is not correct. I new table with DEFAULT attribute into
dump_test extension and pg_dump with binary-upgrade is failing with
pg_dump: invalid column number 1 for table "bb_tab_fkey".

This problem is not the object of this patch, but the one reported by
Gilles Darold here:
/messages/by-id/54B7A400.4020805@dalibo.com
And there are patches traded in this CF to solve this issue.

rushabh@rushabh-centos-vm:dump_test$ cat dump_test--1.0.sql
/* dump_test/dump_test--1.0.sql */

-- complain if script is sourced in psql, rather than via CREATE EXTENSION
\echo Use "CREATE EXTENSION dump_test" to load this file. \quit

CREATE TABLE IF NOT EXISTS bb_tab_fkey (
id int PRIMARY KEY
);

CREATE TABLE IF NOT EXISTS aa_tab_fkey (
id int REFERENCES bb_tab_fkey(id)
);

CREATE TABLE IF NOT EXISTS foo ( a int default 100 );

This gave another strong reason to add if (!tbinfo->dobj.dump) check
rather then ext_member check. What you say ?

Nope. Using dobj.dump is a bad idea as well, I think that this would break
the tracking of inherit tables and their parent relations, aka the
relations that are marked as interesting to track and from which we need
attribute information.

Btw, perhaps you may have noticed, but I marked this patch as rejected... I
don't think it makes much sense to put restrictions in this code path after
finding my way through all the stuff of pg_dump.
--
Michael

Michael Paquier wrote:

On Tue, Feb 24, 2015 at 3:13 PM, Rushabh Lathia <rushabh.lathia@gmail.com>
wrote:

rushabh@rushabh-centos-vm:dump_test$ cat dump_test--1.0.sql
/* dump_test/dump_test--1.0.sql */

Hm. I think it would be a good idea to collect these extension files
somewhere so that pg_dump hacking can be tested with them. Right now,
the extension part of pg_dump is easy to break in subtle ways, which
discourages anyone who wants to meddle with it.

Maybe something in src/test/modules could keep these files so that
pg_dump can be tested. Is anybody interested in doing that?

Btw, perhaps you may have noticed, but I marked this patch as rejected... I
don't think it makes much sense to put restrictions in this code path after
finding my way through all the stuff of pg_dump.

Please don't do that -- I mean don't use the commitfest app as the only
source for random bits of data. If you want to reject a patch, please
make sure to point that out in the email thread. It's impossible to go
from email archives to commitfest entries.

--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

On Tue, Feb 24, 2015 at 11:55 PM, Alvaro Herrera <alvherre@2ndquadrant.com>
wrote:

Michael Paquier wrote:

On Tue, Feb 24, 2015 at 3:13 PM, Rushabh Lathia <

rushabh.lathia@gmail.com>

wrote:

rushabh@rushabh-centos-vm:dump_test$ cat dump_test--1.0.sql
/* dump_test/dump_test--1.0.sql */

Hm. I think it would be a good idea to collect these extension files
somewhere so that pg_dump hacking can be tested with them. Right now,
the extension part of pg_dump is easy to break in subtle ways, which
discourages anyone who wants to meddle with it.

Maybe something in src/test/modules could keep these files so that
pg_dump can be tested. Is anybody interested in doing that?

For the patch to fix data dump of extensions that contain tables with FK I
have implemented a test case in src/test/modules as I didn't want to look
at postgis stuff to reproduce the failure, last version is here:
/messages/by-id/CAB7nPqQRjHHmNMcGB0eCVZ2PTMNCmFD+1htLOoxRQjMzgtSetQ@mail.gmail.com

Btw, perhaps you may have noticed, but I marked this patch as rejected...
I

don't think it makes much sense to put restrictions in this code path

after

finding my way through all the stuff of pg_dump.

Please don't do that -- I mean don't use the commitfest app as the only
source for random bits of data. If you want to reject a patch, please
make sure to point that out in the email thread. It's impossible to go
from email archives to commitfest entries.

Sure. Sorry for that...
--
Michael

Michael Paquier wrote:

On Tue, Feb 24, 2015 at 11:55 PM, Alvaro Herrera <alvherre@2ndquadrant.com>
wrote:

Maybe something in src/test/modules could keep these files so that
pg_dump can be tested. Is anybody interested in doing that?

For the patch to fix data dump of extensions that contain tables with FK I
have implemented a test case in src/test/modules as I didn't want to look
at postgis stuff to reproduce the failure, last version is here:
/messages/by-id/CAB7nPqQRjHHmNMcGB0eCVZ2PTMNCmFD+1htLOoxRQjMzgtSetQ@mail.gmail.com

That looks interesting -- I don't recall seeing it. Does it support
more doing than one extension? If so, we could certainly integrate it.

--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

On Wed, Feb 25, 2015 at 10:30 AM, Alvaro Herrera <alvherre@2ndquadrant.com>
wrote:

Michael Paquier wrote:

On Tue, Feb 24, 2015 at 11:55 PM, Alvaro Herrera <

alvherre@2ndquadrant.com>

wrote:

Maybe something in src/test/modules could keep these files so that
pg_dump can be tested. Is anybody interested in doing that?

For the patch to fix data dump of extensions that contain tables with FK

I

have implemented a test case in src/test/modules as I didn't want to look
at postgis stuff to reproduce the failure, last version is here:

/messages/by-id/CAB7nPqQRjHHmNMcGB0eCVZ2PTMNCmFD+1htLOoxRQjMzgtSetQ@mail.gmail.com

That looks interesting -- I don't recall seeing it. Does it support
more doing than one extension? If so, we could certainly integrate it.

It uses TAP tests to kick pg_dump commands, and I haven't added any logic
for having more than one extension installed within a single test if that
is what you mean, but I assume that we could pass to prove_check a variable
able to install multiple paths and install their content. For now though
what I have done is adding one line to prove_check to install the content
of current directory to be able to run TAP tests with one extension.
--
Michael

Michael Paquier wrote:

On Wed, Feb 25, 2015 at 10:30 AM, Alvaro Herrera <alvherre@2ndquadrant.com>
wrote:

That looks interesting -- I don't recall seeing it. Does it support
more doing than one extension? If so, we could certainly integrate it.

It uses TAP tests to kick pg_dump commands, and I haven't added any logic
for having more than one extension installed within a single test if that
is what you mean,

Yeah --- it would be horrible to have modules test_dump1, test_dump2 and
so forth for different details in the extension SQL files.

but I assume that we could pass to prove_check a variable able to
install multiple paths and install their content. For now though what
I have done is adding one line to prove_check to install the content
of current directory to be able to run TAP tests with one extension.

Haven't looked at the TAP stuff so I have no suggestions.

--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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