Thread: pg_dump gets attributes from tables in extensions
Hi all,
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?Thoughts?
Regards,
--
Michael
Attachment
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?)
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
Attachment
<div dir="ltr">Thanks to the easy handy testcase, was able to replicate the test scenario<br />on my local environment. Andyes tbinfo->dobj.ext_member check into<br />getTableAttrs() do fix the issue.<br /><br />Looking more into pg_dumpcode what I found that, generally PG don't have <br />tbinfo->dobj.ext_member check to ignore the object. Mostlywe do this kind<br />of check using tbinfo->dobj.dump (look at dumpTable() for reference). Do you<br />have anyparticular reason if choosing dobj.ext_member over dobj.dump ?<br /></div><div class="gmail_extra"><br /><div class="gmail_quote">OnFri, Feb 20, 2015 at 12:20 PM, Michael Paquier <span dir="ltr"><<a href="mailto:michael.paquier@gmail.com"target="_blank">michael.paquier@gmail.com</a>></span> wrote:<br /><blockquote class="gmail_quote"style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">On Fri, Feb 20, 2015at 5:33 AM, Peter Eisentraut <<a href="mailto:peter_e@gmx.net">peter_e@gmx.net</a>> wrote:<br /> > On 2/16/152:45 AM, Michael Paquier wrote:<br /> >> While looking at the patch to fix pg_dump with extensions containing<br/> >> tables referencing each other, I got surprised by the fact that<br /> >> getTableAttrs triesto dump table attributes even for tables that are<br /> >> part of an extension. Is that normal?<br /> >>Attached is a patch that I think makes things right, but not dumping any<br /> >> tables that are part of ext_member.<br/> ><br /> > Can you provide an example/test case? (e.g., which publicly available<br /> > extensioncontains tables with attributes?)<br /><br /></span>Sure. Attached is a simplified version of the extension I usedfor the<br /> other patch on pg_dump.<br /> $ psql -c 'create extension dump_test'<br /> CREATE EXTENSION<br /> $ psql-At -c '\dx+ dump_test'<br /> table aa_tab_fkey<br /> table bb_tab_fkey<br /> $ pg_dump -v 2>&1 | grep "columnsand types"<br /> pg_dump: finding the columns and types of table "public"."bb_tab_fkey"<br /> pg_dump: finding thecolumns and types of table "public"."aa_tab_fkey"<br /><span class="HOEnZb"><font color="#888888">--<br /> Michael<br/></font></span><br /><br /> --<br /> Sent via pgsql-hackers mailing list (<a href="mailto:pgsql-hackers@postgresql.org">pgsql-hackers@postgresql.org</a>)<br/> To make changes to your subscription:<br/><a href="http://www.postgresql.org/mailpref/pgsql-hackers" target="_blank">http://www.postgresql.org/mailpref/pgsql-hackers</a><br/><br /></blockquote></div><br /><br clear="all" /><br/>-- <br /><div class="gmail_signature">Rushabh Lathia</div></div>
<div dir="ltr"><br /><br />On Mon, Feb 23, 2015 at 5:57 PM, Rushabh Lathia <<a href="mailto:rushabh.lathia@gmail.com">rushabh.lathia@gmail.com</a>>wrote:<br />> Thanks to the easy handy testcase,was able to replicate the test scenario<br />> on my local environment. And yes tbinfo->dobj.ext_member checkinto<br />> getTableAttrs() do fix the issue.<br />><br />> Looking more into pg_dump code what I found that,generally PG don't have<br />> tbinfo->dobj.ext_member check to ignore the object. Mostly we do this kind<br />>of check using tbinfo->dobj.dump (look at dumpTable() for reference). Do you<br />> have any particular reasonif choosing dobj.ext_member over dobj.dump ?<br /><br />Hm. I have been pondering about this code more and I am droppingthe patch as either adding a check based on the flag to track dumpable objects or ext_member visibly breaks the logicthat binary upgrade and tables in extensions rely on. Particularly this portion makes me think so in getExtensionMembership():<br/> /*<br /> * Normally, mark the member object as not to be dumped. But in<br /> * binary upgrades, we still dump the members individually, since the<br /> * idea is to exactly reproduce the database contents rather than<br /> * replace the extension contentswith something different.<br /> */<br /> if (!dopt->binary_upgrade)<br /> dobj->dump = false;<br /> else<br /> dobj->dump = refdobj->dump;<br/>Regards,<br />-- <br />Michael</div>
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 ?
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:
This problem is not the object of this patch, but the one reported by Gilles Darold here:
http://www.postgresql.org/message-id/54B7A400.4020805@dalibo.com
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:
http://www.postgresql.org/message-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
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:
http://www.postgresql.org/message-id/CAB7nPqQRjHHmNMcGB0eCVZ2PTMNCmFD+1htLOoxRQjMzgtSetQ@mail.gmail.com
http://www.postgresql.org/message-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: > http://www.postgresql.org/message-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
<div dir="ltr"><br /><div class="gmail_extra"><br /><div class="gmail_quote">On Wed, Feb 25, 2015 at 10:30 AM, Alvaro Herrera<span dir="ltr"><<a href="mailto:alvherre@2ndquadrant.com" target="_blank">alvherre@2ndquadrant.com</a>></span>wrote:<br /><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px#ccc solid;padding-left:1ex"><span class="">Michael Paquier wrote:<br /> > On Tue, Feb 24, 2015 at11:55 PM, Alvaro Herrera <<a href="mailto:alvherre@2ndquadrant.com">alvherre@2ndquadrant.com</a>><br /> > wrote:<br/><br /></span><span class="">> > Maybe something in src/test/modules could keep these files so that<br />> > pg_dump can be tested. Is anybody interested in doing that?<br /> ><br /> > For the patch to fix datadump of extensions that contain tables with FK I<br /> > have implemented a test case in src/test/modules as I didn'twant to look<br /> > at postgis stuff to reproduce the failure, last version is here:<br /> > <a href="http://www.postgresql.org/message-id/CAB7nPqQRjHHmNMcGB0eCVZ2PTMNCmFD+1htLOoxRQjMzgtSetQ@mail.gmail.com" target="_blank">http://www.postgresql.org/message-id/CAB7nPqQRjHHmNMcGB0eCVZ2PTMNCmFD+1htLOoxRQjMzgtSetQ@mail.gmail.com</a><br /><br/></span>That looks interesting -- I don't recall seeing it. Does it support<br /> more doing than one extension? If so, we could certainly integrate it.<br /></blockquote></div><br /></div><div class="gmail_extra">It uses TAPtests to kick pg_dump commands, and I haven't added any logic for having more than one extension installed within a singletest if that is what you mean, but I assume that we could pass to prove_check a variable able to install multiple pathsand install their content. For now though what I have done is adding one line to prove_check to install the contentof current directory to be able to run TAP tests with one extension.<br />-- <br /><div class="gmail_signature">Michael<br/></div></div></div>
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