Bug in pg_dump

Hello,

There's a long pending issue with pg_dump and extensions that have table
members with foreign keys. This was previously reported in this thread
http://www.postgresql.org/message-id/CA+TgmoYVZkAdMGh_8EL7UVM472GerU0b4pnNFjQYe6ss1K9wDQ@mail.gmail.com
and discuss by Robert. All PostgreSQL users that use the PostGis
extension postgis_topology are facing the issue because the two members
tables (topology and layer) are linked by foreign keys.

If you dump a database with this extension and try to import it you will
experience this error:

pg_restore: [archiver (db)] Error while PROCESSING TOC:
pg_restore: [archiver (db)] Error from TOC entry 3345; 0 157059176
TABLE DATA layer gilles
pg_restore: [archiver (db)] COPY failed for table "layer": ERROR:
insert or update on table "layer" violates foreign key constraint
"layer_topology_id_fkey"
DETAIL: Key (topology_id)=(1) is not present in table "topology".
WARNING: errors ignored on restore: 1

The problem is that, whatever export type you choose (plain/custom and
full-export/data-only) the data of tables "topology" and "layer" are
always exported in alphabetic order. I think this is a bug because
outside extension, in data-only export, pg_dump is able to find foreign
keys dependency and dump table's data in the right order but not with
extension's members. Default is alphabetic order but that should not be
the case with extension's members because constraints are recreated
during the CREATE EXTENSION order. I hope I am clear enough.

Here we have three solutions:

1/ Inform developers of extensions to take care to alphabetical
order when they have member tables using foreign keys.
2/ Inform DBAs that they have to restore the failing table
independently. The use case above can be resumed using the following
command:

pg_restore -h localhost -n topology -t layer -Fc -d
testdb_empty testdump.dump

3/ Inform DBAs that they have to restore the schema first then the
data only using --disable-triggers
4/ Patch pg_dump to solve this issue.

I attach a patch that solves the issue in pg_dump, let me know if it
might be included in Commit Fest or if the three other solutions are a
better choice. I also join a sample extension (test_fk_in_ext) to be
able to reproduce the issue and test the patch. Note that it might
exists a simpler solution than the one I used in this patch, if this is
the case please point me on the right way, I will be pleased to rewrite
and send an other patch.

In the test extension attached, there is a file called
test_fk_in_ext/SYNOPSIS.txt that describe all actions to reproduce the
issue and test the patch. Here is the SQL part of the test extension:

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

CREATE TABLE IF NOT EXISTS a_test_fk_in_ext1 (
id int REFERENCES b_test_fk_in_ext1(id)
);

SELECT pg_catalog.pg_extension_config_dump('b_test_fk_in_ext1', '');
SELECT pg_catalog.pg_extension_config_dump('a_test_fk_in_ext1', '');

Best regards,

--
Gilles Darold
Consultant PostgreSQL
http://dalibo.com - http://dalibo.org

On 1/15/15 5:26 AM, Gilles Darold wrote:
> Hello,
>
> There's a long pending issue with pg_dump and extensions that have table members with foreign keys. This was previously reported in this thread http://www.postgresql.org/message-id/CA+TgmoYVZkAdMGh_8EL7UVM472GerU0b4pnNFjQYe6ss1K9wDQ@mail.gmail.com and discuss by Robert. All PostgreSQL users that use the PostGis extension postgis_topology are facing the issue because the two members tables (topology and layer) are linked by foreign keys.
>
> If you dump a database with this extension and try to import it you will experience this error:
>
> pg_restore: [archiver (db)] Error while PROCESSING TOC:
> pg_restore: [archiver (db)] Error from TOC entry 3345; 0 157059176 TABLE DATA layer gilles
> pg_restore: [archiver (db)] COPY failed for table "layer": ERROR: insert or update on table "layer" violates foreign key constraint "layer_topology_id_fkey"
> DETAIL: Key (topology_id)=(1) is not present in table "topology".
> WARNING: errors ignored on restore: 1
>
>
> The problem is that, whatever export type you choose (plain/custom and full-export/data-only) the data of tables "topology" and "layer" are always exported in alphabetic order. I think this is a bug because outside extension, in data-only export, pg_dump is able to find foreign keys dependency and dump table's data in the right order but not with extension's members. Default is alphabetic order but that should not be the case with extension's members because constraints are recreated during the CREATE EXTENSION order. I hope I am clear enough.
>
> Here we have three solutions:
>
> 1/ Inform developers of extensions to take care to alphabetical order when they have member tables using foreign keys.
> 2/ Inform DBAs that they have to restore the failing table independently. The use case above can be resumed using the following command:
>
> pg_restore -h localhost -n topology -t layer -Fc -d testdb_empty testdump.dump
>
> 3/ Inform DBAs that they have to restore the schema first then the data only using --disable-triggers

I don't like 1-3, and I doubt anyone else does...

> 4/ Patch pg_dump to solve this issue.

5. Disable FK's during load.
This is really a bigger item than just extensions. It would have the nice benefit of doing a wholesale FK validation instead of firing per-row triggers, but it would leave the database in a weird state if a restore failed...

> I attach a patch that solves the issue in pg_dump, let me know if it might be included in Commit Fest or if the three other solutions are a better choice. I also join a sample extension (test_fk_in_ext) to be able to reproduce the issue and test the patch. Note that it might exists a simpler solution than the one I used in this patch, if this is the case please point me on the right way, I will be pleased to rewrite and send an other patch.

The only problem I see with this approach is circular FK's:

decibel(at)decina(dot)local=# create table a(a_id serial primary key, b_id int);
CREATE TABLE
decibel(at)decina(dot)local=# create table b(b_id serial primary key, a_id int references a);
CREATE TABLE
decibel(at)decina(dot)local=# alter table a add foreign key(b_id) references b;
ALTER TABLE
decibel(at)decina(dot)local=#

That's esoteric enough that I think it's OK not to directly support them, but pg_dump shouldn't puke on them (and really should throw a warning). Though it looks like it doesn't handle that in the data-only case anyway...
--
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com

On 16/01/2015 01:06, Jim Nasby wrote:
> On 1/15/15 5:26 AM, Gilles Darold wrote:
>> Hello,
>>
>> There's a long pending issue with pg_dump and extensions that have
>> table members with foreign keys. This was previously reported in this
>> thread
>> http://www.postgresql.org/message-id/CA+TgmoYVZkAdMGh_8EL7UVM472GerU0b4pnNFjQYe6ss1K9wDQ@mail.gmail.com
>> and discuss by Robert. All PostgreSQL users that use the PostGis
>> extension postgis_topology are facing the issue because the two
>> members tables (topology and layer) are linked by foreign keys.
>>
>> If you dump a database with this extension and try to import it you
>> will experience this error:
>>
>> pg_restore: [archiver (db)] Error while PROCESSING TOC:
>> pg_restore: [archiver (db)] Error from TOC entry 3345; 0
>> 157059176 TABLE DATA layer gilles
>> pg_restore: [archiver (db)] COPY failed for table "layer": ERROR:
>> insert or update on table "layer" violates foreign key constraint
>> "layer_topology_id_fkey"
>> DETAIL: Key (topology_id)=(1) is not present in table "topology".
>> WARNING: errors ignored on restore: 1
>>
>>
>> The problem is that, whatever export type you choose (plain/custom
>> and full-export/data-only) the data of tables "topology" and "layer"
>> are always exported in alphabetic order. I think this is a bug
>> because outside extension, in data-only export, pg_dump is able to
>> find foreign keys dependency and dump table's data in the right order
>> but not with extension's members. Default is alphabetic order but
>> that should not be the case with extension's members because
>> constraints are recreated during the CREATE EXTENSION order. I hope I
>> am clear enough.
>>
>> Here we have three solutions:
>>
>> 1/ Inform developers of extensions to take care to alphabetical
>> order when they have member tables using foreign keys.
>> 2/ Inform DBAs that they have to restore the failing table
>> independently. The use case above can be resumed using the following
>> command:
>>
>> pg_restore -h localhost -n topology -t layer -Fc -d
>> testdb_empty testdump.dump
>>
>> 3/ Inform DBAs that they have to restore the schema first then
>> the data only using --disable-triggers
>
> I don't like 1-3, and I doubt anyone else does...
>
>> 4/ Patch pg_dump to solve this issue.
>
> 5. Disable FK's during load.
> This is really a bigger item than just extensions. It would have the
> nice benefit of doing a wholesale FK validation instead of firing
> per-row triggers, but it would leave the database in a weird state if
> a restore failed...

I think this is an other problem. Here we just need to apply to
extension's members tables the same work than to normal tables. I guess
this is what this patch try to solve.

>
>> I attach a patch that solves the issue in pg_dump, let me know if it
>> might be included in Commit Fest or if the three other solutions are
>> a better choice. I also join a sample extension (test_fk_in_ext) to
>> be able to reproduce the issue and test the patch. Note that it might
>> exists a simpler solution than the one I used in this patch, if this
>> is the case please point me on the right way, I will be pleased to
>> rewrite and send an other patch.
>
> The only problem I see with this approach is circular FK's:
>
> decibel(at)decina(dot)local=# create table a(a_id serial primary key, b_id int);
> CREATE TABLE
> decibel(at)decina(dot)local=# create table b(b_id serial primary key, a_id
> int references a);
> CREATE TABLE
> decibel(at)decina(dot)local=# alter table a add foreign key(b_id) references b;
> ALTER TABLE
> decibel(at)decina(dot)local=#
>
> That's esoteric enough that I think it's OK not to directly support
> them, but pg_dump shouldn't puke on them (and really should throw a
> warning). Though it looks like it doesn't handle that in the data-only
> case anyway...

The patch is taking care or circular references and you will be warn if
pg_dump found it in the extension members. That was not the case before.
If you try do dump a database with the postgis extension you will be
warn about FK defined on the edge_data table.

--
Gilles Darold
Consultant PostgreSQL
http://dalibo.com - http://dalibo.org

On Thu, Jan 15, 2015 at 6:26 AM, Gilles Darold <gilles(dot)darold(at)dalibo(dot)com> wrote:
> I attach a patch that solves the issue in pg_dump, let me know if it might
> be included in Commit Fest or if the three other solutions are a better
> choice.

I think a fix in pg_dump is appropriate, so I'd encourage you to add
this to the next CommitFest.

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

Le 19/01/2015 14:41, Robert Haas a écrit :
> On Thu, Jan 15, 2015 at 6:26 AM, Gilles Darold <gilles(dot)darold(at)dalibo(dot)com> wrote:
>> I attach a patch that solves the issue in pg_dump, let me know if it might
>> be included in Commit Fest or if the three other solutions are a better
>> choice.
> I think a fix in pg_dump is appropriate, so I'd encourage you to add
> this to the next CommitFest.
>
Ok, thanks Robert. The patch have been added to next CommitFest under
the Bug Fixes section.

I've also made some review of the patch and more test. I've rewritten
some comments and added a test when TableInfo is NULL to avoid potential
pg_dump crash.

New patch attached: pg_dump.c-extension-FK-v2.patch

Regards

--
Gilles Darold
Consultant PostgreSQL
http://dalibo.com - http://dalibo.org

On Tue, Jan 20, 2015 at 8:06 PM, Gilles Darold <gilles(dot)darold(at)dalibo(dot)com> wrote:
> Le 19/01/2015 14:41, Robert Haas a écrit :
>> On Thu, Jan 15, 2015 at 6:26 AM, Gilles Darold <gilles(dot)darold(at)dalibo(dot)com> wrote:
>>> I attach a patch that solves the issue in pg_dump, let me know if it might
>>> be included in Commit Fest or if the three other solutions are a better
>>> choice.
>> I think a fix in pg_dump is appropriate, so I'd encourage you to add
>> this to the next CommitFest.
>>
> Ok, thanks Robert. The patch have been added to next CommitFest under
> the Bug Fixes section.
>
> I've also made some review of the patch and more test. I've rewritten
> some comments and added a test when TableInfo is NULL to avoid potential
> pg_dump crash.
>
> New patch attached: pg_dump.c-extension-FK-v2.patch

So, I looked at your patch and I have some comments...

The approach taken by the patch looks correct to me as we cannot
create FK constraints after loading the data in the case of an
extension, something that can be done with a data-only dump.

Now, I think that the routine hasExtensionMember may impact
performance unnecessarily in the case of databases with many tables,
say thousands or more. And we can easily avoid this performance
problem by checking the content of each object dumped by doing the
legwork directly in getTableData. Also, most of the NULL pointer
checks are not needed as most of those code paths would crash if
tbinfo is NULL, and actually only keeping the one in dumpConstraint is
fine.

On top of those things, I have written a small extension able to
reproduce the problem that I have included as a test module in
src/test/modules. The dump/restore check is done using the TAP tests,
and I have hacked prove_check to install as well the contents of the
current folder to be able to use the TAP routines with an extension
easily. IMO, as we have no coverage of pg_dump with extensions I think
that it would be useful to ensure that this does not break again in
the future.

All the hacking I have done during the review results in the set of
patch attached:
- 0001 is your patch, Gilles, with some comment fixes as well as the
performance issue with hasExtensionMember fix
- 0002 is the modification of prove_check that makes TAP tests usable
with extensions
- 0003 is the test module covering the tests needed for pg_dump, at
least for the problem of this thread.

Gilles, how does that look to you?
(Btw, be sure to generate your patches directly with git next time. :) )

Regards,
--
Michael

Le 17/02/2015 14:44, Michael Paquier a écrit :
> On Tue, Jan 20, 2015 at 8:06 PM, Gilles Darold <gilles(dot)darold(at)dalibo(dot)com> wrote:
>> Le 19/01/2015 14:41, Robert Haas a écrit :
>>> On Thu, Jan 15, 2015 at 6:26 AM, Gilles Darold <gilles(dot)darold(at)dalibo(dot)com> wrote:
>>>> I attach a patch that solves the issue in pg_dump, let me know if it might
>>>> be included in Commit Fest or if the three other solutions are a better
>>>> choice.
>>> I think a fix in pg_dump is appropriate, so I'd encourage you to add
>>> this to the next CommitFest.
>>>
>> Ok, thanks Robert. The patch have been added to next CommitFest under
>> the Bug Fixes section.
>>
>> I've also made some review of the patch and more test. I've rewritten
>> some comments and added a test when TableInfo is NULL to avoid potential
>> pg_dump crash.
>>
>> New patch attached: pg_dump.c-extension-FK-v2.patch
> So, I looked at your patch and I have some comments...
>
> The approach taken by the patch looks correct to me as we cannot
> create FK constraints after loading the data in the case of an
> extension, something that can be done with a data-only dump.
>
> Now, I think that the routine hasExtensionMember may impact
> performance unnecessarily in the case of databases with many tables,
> say thousands or more. And we can easily avoid this performance
> problem by checking the content of each object dumped by doing the
> legwork directly in getTableData. Also, most of the NULL pointer
> checks are not needed as most of those code paths would crash if
> tbinfo is NULL, and actually only keeping the one in dumpConstraint is
> fine.

Yes that's exactly what we discuss at Moscow, thanks for removing the
hasExtensionMember() routine. About NULL pointer I'm was not sure that
it could not crash on some other parts so I decided to check it
everywhere. If that's ok to just check in dumpConstraint() that's fine.

> On top of those things, I have written a small extension able to
> reproduce the problem that I have included as a test module in
> src/test/modules. The dump/restore check is done using the TAP tests,
> and I have hacked prove_check to install as well the contents of the
> current folder to be able to use the TAP routines with an extension
> easily. IMO, as we have no coverage of pg_dump with extensions I think
> that it would be useful to ensure that this does not break again in
> the future.

Great ! I did not had time to do that, thank you so much.

> All the hacking I have done during the review results in the set of
> patch attached:
> - 0001 is your patch, Gilles, with some comment fixes as well as the
> performance issue with hasExtensionMember fix
> - 0002 is the modification of prove_check that makes TAP tests usable
> with extensions
> - 0003 is the test module covering the tests needed for pg_dump, at
> least for the problem of this thread.
>
> Gilles, how does that look to you?

Looks great to me, I have tested with the postgis_topology extension
everything works fine.

> (Btw, be sure to generate your patches directly with git next time. :) )

Sorry, some old reminiscence :-)

Thanks for the review.

Best regards,

--
Gilles Darold
Consultant PostgreSQL
http://dalibo.com - http://dalibo.org

On Tue, Feb 24, 2015 at 2:17 AM, Gilles Darold <gilles(dot)darold(at)dalibo(dot)com>
wrote:

> Looks great to me, I have tested with the postgis_topology extension
> everything works fine.
>

Actually, after looking more in depth at the internals of pg_dump I think
that both patches are wrong (did that yesterday night for another patch).
First this patch marks a table in an extension as always dumpable:
+ /* Mark member table as dumpable */
+ configtbl->dobj.dump = true;
And then many checks on ext_member are added in many code paths to ensure
that objects in extensions have their definition never dumped.
But actually this assumption is not true all the time per this code in
getExtensionMemberShip:
if (!dopt->binary_upgrade)
dobj->dump = false;
else
dobj->dump = refdobj->dump;
So this patch would break binary upgrade where some extension objects
should be dumped (one reason why I haven't noticed that before is that
pg_upgrade tests do not include extensions).

Hence, one idea coming to my mind to fix the problem would be to add some
extra processing directly in getExtensionMembership() after building the
objects DO_TABLE_DATA with makeTableDataInfo() by checking the FK
dependencies and add a dependency link with addObjectDependency. The good
part with that is that even user tables that reference extension tables
with a FK can be restored correctly because their constraint is added
*after* loading the data. I noticed as well that with this patch the
--data-only mode was dumping tables in the correct order.

Speaking of which, patches implementing this idea are attached. The module
test case has been improved as well with a check using a table not in an
extension linked with a FK to a table in an extension.
--
Michael

Le 24/02/2015 05:40, Michael Paquier a écrit :
>
>
> On Tue, Feb 24, 2015 at 2:17 AM, Gilles Darold
> <gilles(dot)darold(at)dalibo(dot)com <mailto:gilles(dot)darold(at)dalibo(dot)com>> wrote:
>
> Looks great to me, I have tested with the postgis_topology extension
> everything works fine.
>
>
> Actually, after looking more in depth at the internals of pg_dump I
> think that both patches are wrong (did that yesterday night for
> another patch). First this patch marks a table in an extension as
> always dumpable:
> + /* Mark member table as dumpable */
> + configtbl->dobj.dump = true;
> And then many checks on ext_member are added in many code paths to
> ensure that objects in extensions have their definition never dumped.
> But actually this assumption is not true all the time per this code in
> getExtensionMemberShip:
> if (!dopt->binary_upgrade)
> dobj->dump = false;
> else
> dobj->dump = refdobj->dump;
> So this patch would break binary upgrade where some extension objects
> should be dumped (one reason why I haven't noticed that before is that
> pg_upgrade tests do not include extensions).
>
> Hence, one idea coming to my mind to fix the problem would be to add
> some extra processing directly in getExtensionMembership() after
> building the objects DO_TABLE_DATA with makeTableDataInfo() by
> checking the FK dependencies and add a dependency link with
> addObjectDependency. The good part with that is that even user tables
> that reference extension tables with a FK can be restored correctly
> because their constraint is added *after* loading the data. I noticed
> as well that with this patch the --data-only mode was dumping tables
> in the correct order.
>
> Speaking of which, patches implementing this idea are attached. The
> module test case has been improved as well with a check using a table
> not in an extension linked with a FK to a table in an extension.
> --
> Michael

This is a far better patch and the test to export/import of the
postgis_topology extension works great for me.

Thanks for the work.

--
Gilles Darold
Consultant PostgreSQL
http://dalibo.com - http://dalibo.org

On Wed, Feb 25, 2015 at 7:46 AM, Gilles Darold <gilles(dot)darold(at)dalibo(dot)com> wrote:
> This is a far better patch and the test to export/import of the
> postgis_topology extension works great for me.
>
> Thanks for the work.

Attached is a patch that uses an even better approach by querying only
once all the FK dependencies of tables in extensions whose data is
registered as dumpable by getExtensionMembership(). Could you check
that it works correctly? My test case passes but an extra check would
be a good nice. The patch footprint is pretty low so we may be able to
backport this patch easily.
--
Michael

Le 26/02/2015 12:41, Michael Paquier a écrit :
> On Wed, Feb 25, 2015 at 7:46 AM, Gilles Darold <gilles(dot)darold(at)dalibo(dot)com> wrote:
>> This is a far better patch and the test to export/import of the
>> postgis_topology extension works great for me.
>>
>> Thanks for the work.
> Attached is a patch that uses an even better approach by querying only
> once all the FK dependencies of tables in extensions whose data is
> registered as dumpable by getExtensionMembership(). Could you check
> that it works correctly? My test case passes but an extra check would
> be a good nice. The patch footprint is pretty low so we may be able to
> backport this patch easily.

Works great too. I'm agree that this patch should be backported but I
think we need to warn admins in the release note that their
import/restore scripts may be broken.

One of the common workaround about this issue was to not take care of
the error during data import and then reload data from the tables with
FK errors at the end of the import. If the admins are not warned, this
can conduct to duplicate entries or return an error.

--
Gilles Darold
Consultant PostgreSQL
http://dalibo.com - http://dalibo.org

On Thu, Feb 26, 2015 at 08:41:46PM +0900, Michael Paquier wrote:
> On Wed, Feb 25, 2015 at 7:46 AM, Gilles Darold <gilles(dot)darold(at)dalibo(dot)com> wrote:
> > This is a far better patch and the test to export/import of the
> > postgis_topology extension works great for me.
> >
> > Thanks for the work.
>
> Attached is a patch that uses an even better approach by querying
> only once all the FK dependencies of tables in extensions whose data
> is registered as dumpable by getExtensionMembership(). Could you
> check that it works correctly? My test case passes but an extra
> check would be a good nice. The patch footprint is pretty low so we
> may be able to backport this patch easily.

+1 for backporting. It's a real bug, and real people get hit by it if
they're using PostGIS, one of our most popular add-ons.

Cheers,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

On Sat, Feb 28, 2015 at 12:29 AM, Gilles Darold
<gilles(dot)darold(at)dalibo(dot)com> wrote:
> Le 26/02/2015 12:41, Michael Paquier a écrit :
>> On Wed, Feb 25, 2015 at 7:46 AM, Gilles Darold <gilles(dot)darold(at)dalibo(dot)com> wrote:
>>> This is a far better patch and the test to export/import of the
>>> postgis_topology extension works great for me.
>>>
>>> Thanks for the work.
>> Attached is a patch that uses an even better approach by querying only
>> once all the FK dependencies of tables in extensions whose data is
>> registered as dumpable by getExtensionMembership(). Could you check
>> that it works correctly? My test case passes but an extra check would
>> be a good nice. The patch footprint is pretty low so we may be able to
>> backport this patch easily.
>
> Works great too. I'm agree that this patch should be backported but I
> think we need to warn admins in the release note that their
> import/restore scripts may be broken.

Of course it makes sense to mention that in the release notes, this
behavior of pg_dump being broken since the creation of extensions.
Since it is working on your side as well, let's put it in the hands of
a committer then and I am marking it as such on the CF app. The test
case I sent on this thread can be used to reproduce the problem easily
with TAP tests...
--
Michael

Michael, all,

* Michael Paquier (michael(dot)paquier(at)gmail(dot)com) wrote:
> On Wed, Feb 25, 2015 at 7:46 AM, Gilles Darold <gilles(dot)darold(at)dalibo(dot)com> wrote:
> > This is a far better patch and the test to export/import of the
> > postgis_topology extension works great for me.
> >
> > Thanks for the work.
>
> Attached is a patch that uses an even better approach by querying only
> once all the FK dependencies of tables in extensions whose data is
> registered as dumpable by getExtensionMembership(). Could you check
> that it works correctly? My test case passes but an extra check would
> be a good nice. The patch footprint is pretty low so we may be able to
> backport this patch easily.

I've started looking at this and it looks pretty simple and definitely
something to backpatch (and mention in the release notes that existing
pg_dump exports might be broken..).

One thing that might be missing is what Jim brought up though- that this
won't be able to deal with circular dependencies. I'm not sure that we
need to care, but I *do* think we should document that in the extension
documentation as unsupported. Perhaps in the future we can improve on
this situation by setting up to drop and recreate the constraints,
though another thought I had was to require extensions with circular
dependencies to use deferrable constraints and then make sure we set
constraints to deferred. That isn't something we'd want to backpatch
though, so my plan is to push forward with this.

Thanks!

Stephen

Michael,

* Michael Paquier (michael(dot)paquier(at)gmail(dot)com) wrote:
> + /*
> + * Query all the foreign key dependencies for all the extension
> + * tables found previously. Only tables whose data need to be
> + * have to be tracked.
> + */
> + appendPQExpBuffer(query2,
> + "SELECT conrelid, confrelid "
> + "FROM pg_catalog.pg_constraint "
> + "WHERE contype = 'f' AND conrelid IN (");
> +
> + for (j = 0; j < nconfigitems; j++)
> + {

[...]

Instead of building up a (potentially) big list like this, couldn't we
simply join against pg_extension and check if conrelid = ANY (extconfig)
instead, based on the extension we're currently processing?

eg:

appendPQExpBuffer(query2,
"SELECT conrelid, confrelid "
"FROM pg_catalog.pg_constraint, pg_extension "
"WHERE contype = 'f' AND extname = '%s' AND conrelid = ANY (extconfig)",
fmtId(curext->dobj.name));

This seemed to work just fine for me, at least, and reduces the size of
the patch a bit further since we don't need the loop that builds up the
ids.

> Subject: [PATCH 2/3] Make prove_check install contents of current directory as well

This is really an independent thing, no? I don't see any particular
problem with it, for my part.

Thanks!

Stephen

On Sun, Mar 1, 2015 at 1:09 AM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> Michael,
>
> * Michael Paquier (michael(dot)paquier(at)gmail(dot)com) wrote:
>> + /*
>> + * Query all the foreign key dependencies for all the extension
>> + * tables found previously. Only tables whose data need to be
>> + * have to be tracked.
>> + */
>> + appendPQExpBuffer(query2,
>> + "SELECT conrelid, confrelid "
>> + "FROM pg_catalog.pg_constraint "
>> + "WHERE contype = 'f' AND conrelid IN (");
>> +
>> + for (j = 0; j < nconfigitems; j++)
>> + {
>
> [...]
>
> Instead of building up a (potentially) big list like this, couldn't we
> simply join against pg_extension and check if conrelid = ANY (extconfig)
> instead, based on the extension we're currently processing?
>
> eg:
>
> appendPQExpBuffer(query2,
> "SELECT conrelid, confrelid "
> "FROM pg_catalog.pg_constraint, pg_extension "
> "WHERE contype = 'f' AND extname = '%s' AND conrelid = ANY (extconfig)",
> fmtId(curext->dobj.name));
>
> This seemed to work just fine for me, at least, and reduces the size of
> the patch a bit further since we don't need the loop that builds up the
> ids.

Actually, I did a mistake in my last patch, see this comment:
+ * Now that all the TableInfoData objects have been created for
+ * all the extensions, check their FK dependencies and register
+ * them to ensure correct data ordering.

The thing is that we must absolutely wait for *all* the TableInfoData
of all the extensions to be created because we need to mark the
dependencies between them, and even my last patch, or even with what
you are proposing we may miss tracking of FK dependencies between
tables in different extensions. This has little chance to happen in
practice, but I think that we should definitely get things right.
Hence something like this query able to query all the FK dependencies
with tables in extensions is more useful, and it has no IN clause:
+ appendPQExpBuffer(query,
+ "SELECT conrelid, confrelid "
+ "FROM pg_constraint "
+ "LEFT JOIN pg_depend ON (objid = confrelid) "
+ "WHERE contype = 'f' "
+ "AND refclassid = 'pg_extension'::regclass "
+ "AND classid = 'pg_class'::regclass;");

>> Subject: [PATCH 2/3] Make prove_check install contents of current directory as well
>
> This is really an independent thing, no? I don't see any particular
> problem with it, for my part.

Yes, that's an independent thing, but my guess is that it would be
good to have a real test case this time to be sure that this does not
break again, at least on master where test/modules is an ideal place.

Attached are updated patches, the fix of pg_dump can be easily
cherry-picked down to 9.2. For 9.1 things are changed a bit because of
the way SQL queries are run, still patches are attached for all the
versions needed. I tested as well the fix down to this version 9.1
using the test case dump_test.
Thanks,
--
Michael

Michael,

* Michael Paquier (michael(dot)paquier(at)gmail(dot)com) wrote:
> The thing is that we must absolutely wait for *all* the TableInfoData
> of all the extensions to be created because we need to mark the
> dependencies between them, and even my last patch, or even with what
> you are proposing we may miss tracking of FK dependencies between
> tables in different extensions. This has little chance to happen in
> practice, but I think that we should definitely get things right.
> Hence something like this query able to query all the FK dependencies
> with tables in extensions is more useful, and it has no IN clause:

Ah, yes, extensions can depend on each other and so this could
definitely happen. The current issue is around postgis, which by itself
provides three different extensions.

> + appendPQExpBuffer(query,
> + "SELECT conrelid, confrelid "
> + "FROM pg_constraint "
> + "LEFT JOIN pg_depend ON (objid = confrelid) "
> + "WHERE contype = 'f' "
> + "AND refclassid = 'pg_extension'::regclass "
> + "AND classid = 'pg_class'::regclass;");

I'm trying to figure out why this is a left join..?

> >> Subject: [PATCH 2/3] Make prove_check install contents of current directory as well
> >
> > This is really an independent thing, no? I don't see any particular
> > problem with it, for my part.
>
> Yes, that's an independent thing, but my guess is that it would be
> good to have a real test case this time to be sure that this does not
> break again, at least on master where test/modules is an ideal place.

No objection to it, just pointing out that it's independent.

> Attached are updated patches, the fix of pg_dump can be easily
> cherry-picked down to 9.2. For 9.1 things are changed a bit because of
> the way SQL queries are run, still patches are attached for all the
> versions needed. I tested as well the fix down to this version 9.1
> using the test case dump_test.

Will review.

Thanks!

Stephen

Michael,

* Michael Paquier (michael(dot)paquier(at)gmail(dot)com) wrote:
> >> Subject: [PATCH 2/3] Make prove_check install contents of current directory as well
> >
> > This is really an independent thing, no? I don't see any particular
> > problem with it, for my part.
>
> Yes, that's an independent thing, but my guess is that it would be
> good to have a real test case this time to be sure that this does not
> break again, at least on master where test/modules is an ideal place.

I've been looking at this, but noticed the following in
src/test/Makefile:

# We want to recurse to all subdirs for all standard targets, except that
# installcheck and install should not recurse into the subdirectory "modules".

Also, adding a few more items to the Makefile makes the regression tests
pass:

+ MODULE_big = dump_test
+ REGRESS = dump_test

So I'm thinking this isn't really necessary?

I've not really looked into it further but my hunch is the difference is
a pgxs build vs. a non-pgxs build (which I think might be what the
regression suite runs..). One or both of the above might be necessary
to make non-pgxs builds work.

I've made a few other modifications to the test (basically, I wrapped
all the commands run in command_ok() since it wasn't actually failing
when I was expecting it to and plan to include it in the commit to
master.

Thanks!

Stephen

Michael,

* Stephen Frost (sfrost(at)snowman(dot)net) wrote:
> * Michael Paquier (michael(dot)paquier(at)gmail(dot)com) wrote:
> > >> Subject: [PATCH 2/3] Make prove_check install contents of current directory as well
> > >
> > > This is really an independent thing, no? I don't see any particular
> > > problem with it, for my part.
> >
> > Yes, that's an independent thing, but my guess is that it would be
> > good to have a real test case this time to be sure that this does not
> > break again, at least on master where test/modules is an ideal place.
>
> I've been looking at this, but noticed the following in
> src/test/Makefile:
>
> # We want to recurse to all subdirs for all standard targets, except that
> # installcheck and install should not recurse into the subdirectory "modules".

Hrmpf, that hadn't fixed it as I thought, I had another issue which was
making it appear to work.

The other modules work because they use pg_regress and pass it an
'--extra-install' option, so perhaps adding this makes sense after all,
though I'm a bit nervous that we're doing double-duty with this
approach as some things clearly do get installed by the first 'install'.

Peter, if you have a minute, could you take a look at this thread and
discussion of having TAP tests under src/test/modules which need to
install an extension? I think it's something we certainly want to
support but I'm not sure it's a good idea to just run install in every
directory that has a prove_check.

I'm going to move forward with the actual bug fix. We can certainly add
the test in later, once we've got this all sorted.

Thanks!

Stephen

Michael,

* Stephen Frost (sfrost(at)snowman(dot)net) wrote:
> * Michael Paquier (michael(dot)paquier(at)gmail(dot)com) wrote:
> > The thing is that we must absolutely wait for *all* the TableInfoData
> > of all the extensions to be created because we need to mark the
> > dependencies between them, and even my last patch, or even with what
> > you are proposing we may miss tracking of FK dependencies between
> > tables in different extensions. This has little chance to happen in
> > practice, but I think that we should definitely get things right.
> > Hence something like this query able to query all the FK dependencies
> > with tables in extensions is more useful, and it has no IN clause:
>
> Ah, yes, extensions can depend on each other and so this could
> definitely happen. The current issue is around postgis, which by itself
> provides three different extensions.
>
> > + appendPQExpBuffer(query,
> > + "SELECT conrelid, confrelid "
> > + "FROM pg_constraint "
> > + "LEFT JOIN pg_depend ON (objid = confrelid) "
> > + "WHERE contype = 'f' "
> > + "AND refclassid = 'pg_extension'::regclass "
> > + "AND classid = 'pg_class'::regclass;");
>
> I'm trying to figure out why this is a left join..?

Please see the latest version of this, attached. I've removed the left
join, re-used the 'query' buffer (instead of destroying and recreating
it), and added a bit of documentation, along with a note in the commit
message for the release notes.

Would be great if you could review it and let me know. As mentioned in
my other email, I'm happy to include the TAP test in master once we've
worked out the correct way to handle installing the extension for
testing in the Makefile system.

Thanks!

Stephen

On Mon, Mar 2, 2015 at 6:27 AM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> Please see the latest version of this, attached. I've removed the left
> join, re-used the 'query' buffer (instead of destroying and recreating
> it), and added a bit of documentation, along with a note in the commit
> message for the release notes.

Thanks, those modifications look good to me.

> Would be great if you could review it and let me know. As mentioned in
> my other email, I'm happy to include the TAP test in master once we've
> worked out the correct way to handle installing the extension for
> testing in the Makefile system.

Sure. As that's unrelated to this thread, perhaps we could discuss
this point on another thread with refreshed patches? I'd like to hear
some input from Peter on the matter as well.
--
Michael

* Michael Paquier (michael(dot)paquier(at)gmail(dot)com) wrote:
> On Mon, Mar 2, 2015 at 6:27 AM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> > Please see the latest version of this, attached. I've removed the left
> > join, re-used the 'query' buffer (instead of destroying and recreating
> > it), and added a bit of documentation, along with a note in the commit
> > message for the release notes.
>
> Thanks, those modifications look good to me.

Ok, I've pushed the pg_dump fix for all the branches it applies to.
Thanks for the help!

> > Would be great if you could review it and let me know. As mentioned in
> > my other email, I'm happy to include the TAP test in master once we've
> > worked out the correct way to handle installing the extension for
> > testing in the Makefile system.
>
> Sure. As that's unrelated to this thread, perhaps we could discuss
> this point on another thread with refreshed patches? I'd like to hear
> some input from Peter on the matter as well.

That's fine with me- feel free to start a new thread with new patches.

Thanks again!

Stephen

On Tue, Mar 3, 2015 at 4:57 AM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> * Michael Paquier (michael(dot)paquier(at)gmail(dot)com) wrote:
>> On Mon, Mar 2, 2015 at 6:27 AM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
>> > Please see the latest version of this, attached. I've removed the left
>> > join, re-used the 'query' buffer (instead of destroying and recreating
>> > it), and added a bit of documentation, along with a note in the commit
>> > message for the release notes.
>>
>> Thanks, those modifications look good to me.
>
> Ok, I've pushed the pg_dump fix for all the branches it applies to.

Thanks.

>> > Would be great if you could review it and let me know. As mentioned in
>> > my other email, I'm happy to include the TAP test in master once we've
>> > worked out the correct way to handle installing the extension for
>> > testing in the Makefile system.
>>
>> Sure. As that's unrelated to this thread, perhaps we could discuss
>> this point on another thread with refreshed patches? I'd like to hear
>> some input from Peter on the matter as well.
>
> That's fine with me- feel free to start a new thread with new patches.

Will do so.
--
Michael

On 3/1/15 2:17 PM, Stephen Frost wrote:
> Peter, if you have a minute, could you take a look at this thread and
> discussion of having TAP tests under src/test/modules which need to
> install an extension? I think it's something we certainly want to
> support but I'm not sure it's a good idea to just run install in every
> directory that has a prove_check.

I don't think the way the tests are set up is scalable. Over time, we
will surely want to test more and different extension dumping scenarios.
We don't want to have to create a new fully built-out extension for
each of those test cases, and we're going to run out of useful names for
the extensions, too.

Moreover, I would like to have tests of pg_dump in src/bin/pg_dump/, not
in remote areas of the code.

Here's what I would do:

- set up basic scaffolding for TAP tests in src/bin/pg_dump

- write a Perl function that can create an extension on the fly, given
name, C code, SQL code

- add to the proposed t/001_dump_test.pl code to write the extension

- add that test to the pg_dump test suite

Eventually, the dump-and-restore routine could also be refactored, but
as long as we only have one test case, that can wait.

On Wed, Mar 4, 2015 at 6:48 AM, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
> - set up basic scaffolding for TAP tests in src/bin/pg_dump

Agreed.

> - write a Perl function that can create an extension on the fly, given
> name, C code, SQL code

I am perplex about that. Where would the SQL code or C code be stored?
In the pl script itself? I cannot really see the advantage to generate
automatically the skeletton of an extension based on some C or SQL
code in comparison to store the extension statically as-is. Adding
those extensions in src/test/modules is out of scope to not bloat it,
so we could for example add such test extensions in t/extensions/ or
similar, and have prove_check scan this folder, then install those
extensions in the temporary installation.

> - add to the proposed t/001_dump_test.pl code to write the extension
> - add that test to the pg_dump test suite
> Eventually, the dump-and-restore routine could also be refactored, but
> as long as we only have one test case, that can wait.

Agreed on all those points.
--
Michael

On Wed, Mar 4, 2015 at 2:03 PM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> On Wed, Mar 4, 2015 at 6:48 AM, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
>> - set up basic scaffolding for TAP tests in src/bin/pg_dump
>
> Agreed.
>
>> - write a Perl function that can create an extension on the fly, given
>> name, C code, SQL code
>
> I am perplex about that. Where would the SQL code or C code be stored?
> In the pl script itself? I cannot really see the advantage to generate
> automatically the skeletton of an extension based on some C or SQL
> code in comparison to store the extension statically as-is. Adding
> those extensions in src/test/modules is out of scope to not bloat it,
> so we could for example add such test extensions in t/extensions/ or
> similar, and have prove_check scan this folder, then install those
> extensions in the temporary installation.
>
>> - add to the proposed t/001_dump_test.pl code to write the extension
>> - add that test to the pg_dump test suite
>> Eventually, the dump-and-restore routine could also be refactored, but
>> as long as we only have one test case, that can wait.
>
> Agreed on all those points.

Please note that I have created a new thread especially for this purpose here:
http://www.postgresql.org/message-id/CAB7nPqRx=zmBFJyjrWhGuhHqK__8M+wd+P95ceNJtMHxXR7RRg@mail.gmail.com
Perhaps we should move there this discussion as it is rather
independent of the problem that has been reported.

Regards,
--
Michael