Re: [HACKERS] Improving test coverage of extensions with pg_dump

2015-12-01 Thread Michael Paquier
On Tue, Sep 29, 2015 at 9:39 PM, Michael Paquier wrote:
> Perhaps you did not look at the last patch I sent on this thread, but
> I changed it so as a schedule is used with a call to pg_regress.
> That's a more scalable approach as you were concerned about as we can
> plug in more easily new modules and new regression tests without
> modifying the perl script used to kick the tests, though it does not
> run the main regression test suite and it actually cannot run it,
> except with a modified schedule or with a filter of the child-parent
> tables. Patch is attached for consideration, which looks like a good
> base for future support, feel free to disagree :)

It is a bit sad to say, but this patch aiming at adding a test case
for an uncovered code path of pg_dump has not reached an agreement.
The most simple way to reduce the overhead of this test would be to
include something in the main regression test suite and let pg_dump
call incorporated in pg_upgrade test do the work. This has been
suggested upthread already as one way to do things.
Doing diff comparision of pg_dump/restore on the regression database
data set would have been nice, but the column ordering of inherited
tables when a column is added to a child is a can of worms, so that's
not something this patch should try to address.
So marking it as returned with feedback or moving it to next CF?
-- 
Michael


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


Re: [HACKERS] Improving test coverage of extensions with pg_dump

2015-09-29 Thread Michael Paquier
On Sat, Sep 26, 2015 at 10:22 PM, Andres Freund  wrote:
> On 2015-09-26 21:54:46 +0900, Michael Paquier wrote:
>> On Wed, Sep 23, 2015 at 1:04 AM, Alvaro Herrera 
>> wrote:
>> > We discussed this in some other thread, not long ago.  I looked briefly
>> > in the archives but couldn't find it.  I think the conclusion was
>> > something along the lines of "hmm, tough".
>
>> That's hours, even days of fun ahead for sure.
>
> To me that's a somewhat serious bug, and something that we probably need
> to address at some point.

Well, addressing it at the root of the problem is... Let's say...
Really tough. I have put my head on this stuff for a couple of hours
and tried to draw up a couple of ways to fix this. First, even if
pg_dump relies heavily on the assumption that attributes need to be
ordered by attnum, I thought that it would have been possible to use
attinhcount to order  the columns in the same way when taking the dump
from a source db and a target (where dump of source has been restored
to). This would work if there is no more than 1 level of child
relations, but with grand-child relations the order gets messed up
again.

Locating a fix on the backend side would make things for pg_dump
easier, an idea would be to simply reorder the attribute numbers when
a column is added to a parent table. For example with something like
that:
CREATE TABLE test_parent (c1 integer, c2 integer);
CREATE TABLE test_child_1 (c3 integer) inherits (test_parent);
CREATE TABLE test_child_2 (c4 integer) inherits (test_child_1);
ALTER TABLE test_parent ADD COLUMN c5 integer;
We get the following attribute ordering:
=# SELECT c.relname, a.attname, a.attnum
FROM pg_attribute a
  JOIN pg_class c ON (a.attrelid = c.oid)
WHERE a.attrelid IN ('test_parent'::regclass,
 'test_child_1'::regclass,
 'test_child_2'::regclass)
  AND a.attnum > 0 ORDER BY c.relname, a.attnum;
   relname| attname | attnum
--+-+
 test_child_1 | c1  |  1
 test_child_1 | c2  |  2
 test_child_1 | c3  |  3
 test_child_1 | c5  |  4
 test_child_2 | c1  |  1
 test_child_2 | c2  |  2
 test_child_2 | c3  |  3
 test_child_2 | c4  |  4
 test_child_2 | c5  |  5
 test_parent  | c1  |  1
 test_parent  | c2  |  2
 test_parent  | c5  |  3
(12 rows)

Now, what we could do on a child relation when a new attribute in its
parent relation is to increment the attnum of the existing columns
with attinhcount = 0 by 1, and insert the new column at the end of the
existing ones where attinhcount > 0, to give something like that:
   relname| attname | attnum
--+-+
 test_child_1 | c1  |  1
 test_child_1 | c2  |  2
 test_child_1 | c5  |  3
 test_child_1 | c3  |  4
 test_child_2 | c1  |  1
 test_child_2 | c2  |  2
 test_child_2 | c3  |  3
 test_child_2 | c5  |  4
 test_child_2 | c4  |  5
 test_parent  | c1  |  1
 test_parent  | c2  |  2
 test_parent  | c5  |  3
(12 rows)
Looking at tablecmds.c, this could be intuitively done as a part of
ATExecAddColumn by scanning the attributes of the child relation from
the end. But it is of course not that simple, a lot of code paths rely
on the attnum of the current attributes. One is CookedConstraint, but
that's a can of worms for back branches.

Another bandaid solution, that would be less invasive for a backpatch
is to reproduce the sequence of DDL commands within the dump itself:
we would need to dump attinhcount in getTableAttrs and use it to guess
what are the columns on the child relations that have been added on a
parent relation after the children have been created. This would not
solve the case of data-only dumps containing INSERT queries that have
no column names on databases restored from past schema dumps though.

Perhaps you did not look at the last patch I sent on this thread, but
I changed it so as a schedule is used with a call to pg_regress.
That's a more scalable approach as you were concerned about as we can
plug in more easily new modules and new regression tests without
modifying the perl script used to kick the tests, though it does not
run the main regression test suite and it actually cannot run it,
except with a modified schedule or with a filter of the child-parent
tables. Patch is attached for consideration, which looks like a good
base for future support, feel free to disagree :)
Thanks,
-- 
Michael
From a95e5bc29bebb2017399c777aee0123f5d4c8a15 Mon Sep 17 00:00:00 2001
From: Michael Paquier 
Date: Tue, 29 Sep 2015 21:33:50 +0900
Subject: [PATCH] Add test facility to check dump/restore with extensions

The test added compares a dump taken from a source database containing a set
of extensions and a target database after dumping the contents from source and
restore them 

Re: [HACKERS] Improving test coverage of extensions with pg_dump

2015-09-26 Thread Michael Paquier
On Wed, Sep 23, 2015 at 1:04 AM, Alvaro Herrera 
wrote:

> Euler Taveira wrote:
> > On 17-09-2015 14:21, Michael Paquier wrote:
> > >pg_dump relies on attnum to define the column ordering, so one
> > >possibility would be to do things more consistently at backend level.
>
> We discussed this in some other thread, not long ago.  I looked briefly
> in the archives but couldn't find it.  I think the conclusion was
> something along the lines of "hmm, tough".
>

That's hours, even days of fun ahead for sure.


> > Someone can say that we could assign an attnum for column "d" considering
> > all of the inheritance tree. However, attnum is used as an index to
> arrays
> > (we could bloat some of those) and some logic rely on it to count the
> number
> > of columns. It would become tablecmds.c into an spaghetti.
>

That was my first impression, but that's just a band-aid. Switching the
logic in stable branches to assign a correct attnum for a child table,
while switching on the way the attnum referring to the parent entries is a
scary idea. I would suspect that this would break many other things for
fixing a narrow case, and it is still possible for the user to get away by
using COPY or INSERT with the list of columns listed when taking a dump.

We don't need any more spaghetti there, thanks!
>

Agreed.


> > IMHO a possible way to solve it is adding support for logical column
> > ordering. An ALTER TABLE command (emitted if a parameter was informed)
> > during dump could handle it. BTW, last thread [1] about logical column
> > ordering seems to have died a few months ago. Alvaro?
>
> Tomas Vondra also worked a bit on this patch, and we eventually gave up
> on it due to lack of time.  We might be able to get back on it someday,
> but do not hold your breath.  If you want the current bug fixed, do not
> wait for logical column numbering.
>

Honestly I have the feeling that the discussion of this thread gets
unproductive, let's not forget that the patch presented on this thread is
just aiming at adding one test case to ensure that extensions using
dumpable relations with FKs get correctly dumped, which is to ensure that
we do not reintroduce a bug that existed in the extension facility since
its introduction in 9.1. That being said, the patch is just fine for this
purpose, but that's just my opinion.
-- 
Michael


Re: [HACKERS] Improving test coverage of extensions with pg_dump

2015-09-26 Thread Andres Freund
On 2015-09-26 21:54:46 +0900, Michael Paquier wrote:
> On Wed, Sep 23, 2015 at 1:04 AM, Alvaro Herrera 
> wrote:
> > We discussed this in some other thread, not long ago.  I looked briefly
> > in the archives but couldn't find it.  I think the conclusion was
> > something along the lines of "hmm, tough".

> That's hours, even days of fun ahead for sure.

To me that's a somewhat serious bug, and something that we probably need
to address at some point.

> Honestly I have the feeling that the discussion of this thread gets
> unproductive, let's not forget that the patch presented on this thread is
> just aiming at adding one test case to ensure that extensions using
> dumpable relations with FKs get correctly dumped, which is to ensure that
> we do not reintroduce a bug that existed in the extension facility since
> its introduction in 9.1. That being said, the patch is just fine for this
> purpose, but that's just my opinion.

It's an unsustainable test model. Adding own test runners, directories,
initdb etc. for a simple regression test of a couple lines won't hurt
for maybe the first five but after that it starts to get unmaintainable.

Greetings,

Andres Freund


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


Re: [HACKERS] Improving test coverage of extensions with pg_dump

2015-09-22 Thread Alvaro Herrera
Euler Taveira wrote:
> On 17-09-2015 14:21, Michael Paquier wrote:
> >pg_dump relies on attnum to define the column ordering, so one
> >possibility would be to do things more consistently at backend level.

We discussed this in some other thread, not long ago.  I looked briefly
in the archives but couldn't find it.  I think the conclusion was
something along the lines of "hmm, tough".

> Someone can say that we could assign an attnum for column "d" considering
> all of the inheritance tree. However, attnum is used as an index to arrays
> (we could bloat some of those) and some logic rely on it to count the number
> of columns. It would become tablecmds.c into an spaghetti.

We don't need any more spaghetti there, thanks!

> IMHO a possible way to solve it is adding support for logical column
> ordering. An ALTER TABLE command (emitted if a parameter was informed)
> during dump could handle it. BTW, last thread [1] about logical column
> ordering seems to have died a few months ago. Alvaro?

Tomas Vondra also worked a bit on this patch, and we eventually gave up
on it due to lack of time.  We might be able to get back on it someday,
but do not hold your breath.  If you want the current bug fixed, do not
wait for logical column numbering.

-- 
Álvaro Herrerahttp://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


Re: [HACKERS] Improving test coverage of extensions with pg_dump

2015-09-20 Thread Euler Taveira

On 17-09-2015 14:21, Michael Paquier wrote:

pg_dump relies on attnum to define the column ordering, so one
possibility would be to do things more consistently at backend level.
Thoughts?

According to your example, problem is the columns from the parent table 
"aa" are added _before_ declaring the inherited table "bb". Then, an 
attnum from column "d" (part of parent table "aa") is assigned to a 
lower number than in the original table "bb".


Someone can say that we could assign an attnum for column "d" 
considering all of the inheritance tree. However, attnum is used as an 
index to arrays (we could bloat some of those) and some logic rely on it 
to count the number of columns. It would become tablecmds.c into an 
spaghetti.


IMHO a possible way to solve it is adding support for logical column 
ordering. An ALTER TABLE command (emitted if a parameter was informed) 
during dump could handle it. BTW, last thread [1] about logical column 
ordering seems to have died a few months ago. Alvaro?



[1] 
http://www.postgresql.org/message-id/20141209174146.gp1...@alvh.no-ip.org



--
   Euler Taveira   Timbira - http://www.timbira.com.br/
   PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento


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


Re: [HACKERS] Improving test coverage of extensions with pg_dump

2015-09-17 Thread Michael Paquier
On Thu, Sep 17, 2015 at 7:24 AM, Andres Freund wrote:
> On 2015-09-16 22:18:55 -0700, Michael Paquier wrote:
>> Problem is similar with --column-inserts, --inserts and COPY. We could
>> use --exclude-table like in the patch attached when taking the dump
>> from source database but that's grotty, or we could improve pg_dump
>> itself, though it may not be worth it for just this purpose.
>> Thoughts?
>
> Hm. To me that sounds like a moderately serious bug worth fixing. I mean
> if you have a COPY command that worked before a dump/restore but that's
> wrong afterwards, it's pretty ugly, no?

COPY or INSERT include the column list in dumps, so that's not really
an issue here, what is potentially a problem (looking at that freshly)
is --inserts with data-only dumps though. So yes we had better fix it
and perhaps consider a backpatch...
-- 
Michael


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


Re: [HACKERS] Improving test coverage of extensions with pg_dump

2015-09-17 Thread Andres Freund
On 2015-09-16 22:18:55 -0700, Michael Paquier wrote:
> So, here we go.

Cool.

> I have found something quite interesting when playing with the patch
> attached: dump does not guarantee the column ordering across databases
> for some inherited tables, see that example from the main regression
> test suite the following diff between a dump taken from a source
> database and a target database where the source dump has been restored
> in first:
> -INSERT INTO b_star (class, aa, bb, a) VALUES ('b', 3, 'mumble', NULL);
> -INSERT INTO b_star (class, aa, bb, a) VALUES ('b', 4, NULL, NULL);
> -INSERT INTO b_star (class, aa, bb, a) VALUES ('b', NULL, 'bumble', NULL);
> -INSERT INTO b_star (class, aa, bb, a) VALUES ('b', NULL, NULL, NULL);
> +INSERT INTO b_star (class, aa, a, bb) VALUES ('b', 3, NULL, 'mumble');
> +INSERT INTO b_star (class, aa, a, bb) VALUES ('b', 4, NULL, NULL);
> +INSERT INTO b_star (class, aa, a, bb) VALUES ('b', NULL, NULL, 'bumble');
> +INSERT INTO b_star (class, aa, a, bb) VALUES ('b', NULL, NULL, NULL);
> 
> Problem is similar with --column-inserts, --inserts and COPY. We could
> use --exclude-table like in the patch attached when taking the dump
> from source database but that's grotty, or we could improve pg_dump
> itself, though it may not be worth it for just this purpose.
> Thoughts?

Hm. To me that sounds like a moderately serious bug worth fixing. I mean
if you have a COPY command that worked before a dump/restore but that's
wrong afterwards, it's pretty ugly, no?

Greetings,

Andres Freund


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


Re: [HACKERS] Improving test coverage of extensions with pg_dump

2015-09-17 Thread Michael Paquier
On Thu, Sep 17, 2015 at 9:47 AM, Michael Paquier wrote:
> COPY or INSERT include the column list in dumps, so that's not really
> an issue here, what is potentially a problem (looking at that freshly)
> is --inserts with data-only dumps though. So yes we had better fix it
> and perhaps consider a backpatch...

When adding a column to a parent table, attnum gets confused on the
child table... Take this example:
=# create table aa (a int, b int);
CREATE TABLE
=# create table bb (c int) inherits (aa);
CREATE TABLE
=# alter table aa add column d text;
ALTER TABLE
=# select relname, attname, attnum from pg_attribute join pg_class on
(attrelid = oid) where attrelid in( 'bb'::regclass, 'aa'::regclass)
and attnum > 0;
 relname | attname | attnum
-+-+
 aa  | d   |  3
 aa  | b   |  2
 aa  | a   |  1
 bb  | d   |  4
 bb  | c   |  3
 bb  | b   |  2
 bb  | a   |  1
(7 rows)

When this is dumped and restored on another database the ordering gets
different, c and d are switched for child relation bb here:
 relname | attname | attnum
-+-+
 aa  | d   |  3
 aa  | b   |  2
 aa  | a   |  1
 bb  | c   |  4
 bb  | d   |  3
 bb  | b   |  2
 bb  | a   |  1
(7 rows)

pg_dump relies on attnum to define the column ordering, so one
possibility would be to do things more consistently at backend level.
Thoughts?
-- 
Michael


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


Re: [HACKERS] Improving test coverage of extensions with pg_dump

2015-09-16 Thread Andres Freund
On 2015-09-09 10:48:24 +0900, Michael Paquier wrote:
> On Wed, Sep 9, 2015 at 3:33 AM, Andres Freund  wrote:
> > On 2015-09-07 22:55:50 +0900, Michael Paquier wrote:
> >> So, to summarize the state of this patch whose status is now "Waiting
> >> on Author", we have the following possibilities:
> >> 1) Keep the test as-is, as an independent test of src/test/modules.
> >
> > I find that a bad option. A test that takes this long and has that much
> > setup for such a marginal amount of coverage just doesn't seem worth it.
> >
> >> 2) Integrate it in the test suite of src/test/regress and let
> >> pg_upgrade make the work with dump/restore.
> >
> > 2b) ... and create a single src/test/modules/pg_dumprestore test that
> > initdbs, runs installcheck, dumps, loads and compares a repated dump.
> >
> > I think 2b) is by far the best choice.
> 
> And I guess that the extensions tested should be located directly in
> this path, or we would need again to tweak the MSVC scripts...
> Attached is an attempt to solve things by converting the previous
> patch as proposed by Andres. A source and a target databases are used
> on a cluster, and their respective dumps are compared with
> File::Compare (available down to perl 5.8.8) at the end.

Unless I miss something this isn't 2b) as it does *not* actually run
the actual installcheck. Therefore the actual pg_dump/restore coverage
is neglegible.

Greetings,

Andres Freund


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


Re: [HACKERS] Improving test coverage of extensions with pg_dump

2015-09-16 Thread Michael Paquier
On Wed, Sep 16, 2015 at 8:00 PM, Michael Paquier wrote:
> Hm. OK. I didn't get your message correctly, sorry for that. Would you
> be fine then to have a pg_regress command using parallel_schedule + an
> extra schedule launching tests related to the extensions in
> src/test/modules/pg_dumprestore then? The choice of parallel_schedule
> is based on what Windows does, aka this schedule is used in the
> equivalent of make check in vcregress.pl. The TAP script then simply
> initializes the cluster, runs pg_regress, and does the dump/restore
> job. There is no real need to worry about setval as dump is not taken
> from a standby..

So, here we go.

I have found something quite interesting when playing with the patch
attached: dump does not guarantee the column ordering across databases
for some inherited tables, see that example from the main regression
test suite the following diff between a dump taken from a source
database and a target database where the source dump has been restored
in first:
-INSERT INTO b_star (class, aa, bb, a) VALUES ('b', 3, 'mumble', NULL);
-INSERT INTO b_star (class, aa, bb, a) VALUES ('b', 4, NULL, NULL);
-INSERT INTO b_star (class, aa, bb, a) VALUES ('b', NULL, 'bumble', NULL);
-INSERT INTO b_star (class, aa, bb, a) VALUES ('b', NULL, NULL, NULL);
+INSERT INTO b_star (class, aa, a, bb) VALUES ('b', 3, NULL, 'mumble');
+INSERT INTO b_star (class, aa, a, bb) VALUES ('b', 4, NULL, NULL);
+INSERT INTO b_star (class, aa, a, bb) VALUES ('b', NULL, NULL, 'bumble');
+INSERT INTO b_star (class, aa, a, bb) VALUES ('b', NULL, NULL, NULL);

Problem is similar with --column-inserts, --inserts and COPY. We could
use --exclude-table like in the patch attached when taking the dump
from source database but that's grotty, or we could improve pg_dump
itself, though it may not be worth it for just this purpose.
Thoughts?
-- 
Michael
From 636f5dbb3738aab44558d9c65ee4bc61f66aaf07 Mon Sep 17 00:00:00 2001
From: Michael Paquier 
Date: Wed, 9 Sep 2015 10:41:01 +0900
Subject: [PATCH] Add test facility to check dump/restore with extensions

The test added compares a dump taken from a source database containing a set
of extensions and a target database after dumping the contents from source and
restore them to target.

For now this test facility includes one extension to test dumpable objects
with foreign keys, but could be extended with more.
---
 src/test/modules/Makefile  |  1 +
 src/test/modules/pg_dumprestore/.gitignore |  9 +++
 src/test/modules/pg_dumprestore/Makefile   | 24 
 src/test/modules/pg_dumprestore/README |  5 ++
 .../modules/pg_dumprestore/expected/tables_fk.out  | 14 +
 src/test/modules/pg_dumprestore/extension_schedule |  3 +
 src/test/modules/pg_dumprestore/sql/tables_fk.sql  | 18 ++
 .../pg_dumprestore/t/001_dump_restore_test.pl  | 70 ++
 src/test/modules/pg_dumprestore/tables_fk--1.0.sql | 20 +++
 src/test/modules/pg_dumprestore/tables_fk.control  |  5 ++
 src/tools/msvc/Mkvcbuild.pm|  3 +-
 src/tools/msvc/vcregress.pl|  3 +
 12 files changed, 174 insertions(+), 1 deletion(-)
 create mode 100644 src/test/modules/pg_dumprestore/.gitignore
 create mode 100644 src/test/modules/pg_dumprestore/Makefile
 create mode 100644 src/test/modules/pg_dumprestore/README
 create mode 100644 src/test/modules/pg_dumprestore/expected/tables_fk.out
 create mode 100644 src/test/modules/pg_dumprestore/extension_schedule
 create mode 100644 src/test/modules/pg_dumprestore/sql/tables_fk.sql
 create mode 100644 src/test/modules/pg_dumprestore/t/001_dump_restore_test.pl
 create mode 100644 src/test/modules/pg_dumprestore/tables_fk--1.0.sql
 create mode 100644 src/test/modules/pg_dumprestore/tables_fk.control

diff --git a/src/test/modules/Makefile b/src/test/modules/Makefile
index 9b96654..633dc6f 100644
--- a/src/test/modules/Makefile
+++ b/src/test/modules/Makefile
@@ -8,6 +8,7 @@ SUBDIRS = \
 		  brin \
 		  commit_ts \
 		  dummy_seclabel \
+		  pg_dumprestore \
 		  test_ddl_deparse \
 		  test_parser \
 		  test_rls_hooks \
diff --git a/src/test/modules/pg_dumprestore/.gitignore b/src/test/modules/pg_dumprestore/.gitignore
new file mode 100644
index 000..2519efc
--- /dev/null
+++ b/src/test/modules/pg_dumprestore/.gitignore
@@ -0,0 +1,9 @@
+/tmp_check/
+/results/
+/sql/constraints.sql
+/sql/copy.sql
+/sql/create_function_1.sql
+/sql/create_function_2.sql
+/sql/largeobject.sql
+/sql/misc.sql
+/sql/tablespace.sql
diff --git a/src/test/modules/pg_dumprestore/Makefile b/src/test/modules/pg_dumprestore/Makefile
new file mode 100644
index 000..611f23a
--- /dev/null
+++ b/src/test/modules/pg_dumprestore/Makefile
@@ -0,0 +1,24 @@
+# src/test/modules/pg_dumprestore/Makefile
+
+EXTENSION = tables_fk
+DATA = tables_fk--1.0.sql
+PGFILEDESC = "pg_dumprestore - set of extensions dumped and restored"
+
+ifdef USE_PGXS
+PG_CONFIG = pg_config
+PGXS := 

Re: [HACKERS] Improving test coverage of extensions with pg_dump

2015-09-16 Thread Michael Paquier
On Wed, Sep 16, 2015 at 3:42 PM, Andres Freund  wrote:
> On 2015-09-09 10:48:24 +0900, Michael Paquier wrote:
>> On Wed, Sep 9, 2015 at 3:33 AM, Andres Freund  wrote:
>> > On 2015-09-07 22:55:50 +0900, Michael Paquier wrote:
>> >> So, to summarize the state of this patch whose status is now "Waiting
>> >> on Author", we have the following possibilities:
>> >> 1) Keep the test as-is, as an independent test of src/test/modules.
>> >
>> > I find that a bad option. A test that takes this long and has that much
>> > setup for such a marginal amount of coverage just doesn't seem worth it.
>> >
>> >> 2) Integrate it in the test suite of src/test/regress and let
>> >> pg_upgrade make the work with dump/restore.
>> >
>> > 2b) ... and create a single src/test/modules/pg_dumprestore test that
>> > initdbs, runs installcheck, dumps, loads and compares a repated dump.
>> >
>> > I think 2b) is by far the best choice.
>>
>> And I guess that the extensions tested should be located directly in
>> this path, or we would need again to tweak the MSVC scripts...
>> Attached is an attempt to solve things by converting the previous
>> patch as proposed by Andres. A source and a target databases are used
>> on a cluster, and their respective dumps are compared with
>> File::Compare (available down to perl 5.8.8) at the end.
>
> Unless I miss something this isn't 2b) as it does *not* actually run
> the actual installcheck. Therefore the actual pg_dump/restore coverage
> is neglegible.

Hm. OK. I didn't get your message correctly, sorry for that. Would you
be fine then to have a pg_regress command using parallel_schedule + an
extra schedule launching tests related to the extensions in
src/test/modules/pg_dumprestore then? The choice of parallel_schedule
is based on what Windows does, aka this schedule is used in the
equivalent of make check in vcregress.pl. The TAP script then simply
initializes the cluster, runs pg_regress, and does the dump/restore
job. There is no real need to worry about setval as dump is not taken
from a standby..
-- 
Michael


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


Re: [HACKERS] Improving test coverage of extensions with pg_dump

2015-09-08 Thread Andres Freund
On 2015-09-07 22:55:50 +0900, Michael Paquier wrote:
> So, to summarize the state of this patch whose status is now "Waiting
> on Author", we have the following possibilities:
> 1) Keep the test as-is, as an independent test of src/test/modules.

I find that a bad option. A test that takes this long and has that much
setup for such a marginal amount of coverage just doesn't seem worth it.

> 2) Integrate it in the test suite of src/test/regress and let
> pg_upgrade make the work with dump/restore.

2b) ... and create a single src/test/modules/pg_dumprestore test that
initdbs, runs installcheck, dumps, loads and compares a repated dump.

I think 2b) is by far the best choice.

> I would still go for 1), the infrastructures included by the other
> proposals may become useful depending on the types of tests that are
> added in the future, but it is still unclear what those tests are, and
> they may even need something else than what listed here (see that as
> an example http://www.postgresql.org/message-id/54f62c3f.8070...@gmx.net)
> to run properly.

By that argument we're going to add more and more isolated tests and
never merge anything.

I fail to see a single benefit of this approach.

Greetings,

Andres Freund


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


Re: [HACKERS] Improving test coverage of extensions with pg_dump

2015-09-08 Thread Bruce Momjian
On Wed, Sep  2, 2015 at 02:30:33PM -0300, Alvaro Herrera wrote:
> > I think we should rather add *one* test that does dump/restore over the
> > normal regression test database. Something similar to the pg_upgrade
> > tests. And then work at adding more content to the regression test
> > database - potentially sourcing from src/test/modules.
> 
> That's another option, but we've had this idea for many years now and it
> hasn't materialized.  As I recall, Andrew Dunstan has a module that
> tests cross-version pg_upgrade and one thing he does is dump both and
> compare; the problem is that there are differences, so he keeps a count
> of how many lines he expect to differ between any two releases.  Or
> something like that.  While it's a good enough start, I don't think it's
> robust enough to be in core.  How would we do it?

I have shell scripts that test pg_dump restore/upgrade of every
supported PG version.  I also have expected pg_dump output files for
every major version.  This is explained in src/bin/pg_upgrade/TESTING.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

  + Everyone has their own god. +


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


Re: [HACKERS] Improving test coverage of extensions with pg_dump

2015-09-08 Thread Michael Paquier
On Wed, Sep 9, 2015 at 3:33 AM, Andres Freund  wrote:
> On 2015-09-07 22:55:50 +0900, Michael Paquier wrote:
>> So, to summarize the state of this patch whose status is now "Waiting
>> on Author", we have the following possibilities:
>> 1) Keep the test as-is, as an independent test of src/test/modules.
>
> I find that a bad option. A test that takes this long and has that much
> setup for such a marginal amount of coverage just doesn't seem worth it.
>
>> 2) Integrate it in the test suite of src/test/regress and let
>> pg_upgrade make the work with dump/restore.
>
> 2b) ... and create a single src/test/modules/pg_dumprestore test that
> initdbs, runs installcheck, dumps, loads and compares a repated dump.
>
> I think 2b) is by far the best choice.

And I guess that the extensions tested should be located directly in
this path, or we would need again to tweak the MSVC scripts...
Attached is an attempt to solve things by converting the previous
patch as proposed by Andres. A source and a target databases are used
on a cluster, and their respective dumps are compared with
File::Compare (available down to perl 5.8.8) at the end.
Regards,
-- 
Michael
From 54f98d264b7ac463a5373950c548636bf9653690 Mon Sep 17 00:00:00 2001
From: Michael Paquier 
Date: Wed, 9 Sep 2015 10:41:01 +0900
Subject: [PATCH] Add test facility to check dump/restore with extensions

The test added compares a dump taken from a source database containing a set
of extensions and a target database after dumping the contents from source and
restore them to target.

For now this test facility includes one extension to test dumpable objects
with foreign keys, but could be extended with more.
---
 src/test/modules/Makefile  |  1 +
 src/test/modules/pg_dumprestore/.gitignore |  1 +
 src/test/modules/pg_dumprestore/Makefile   | 24 
 src/test/modules/pg_dumprestore/README |  5 +++
 .../pg_dumprestore/t/001_dump_restore_test.pl  | 43 ++
 src/test/modules/pg_dumprestore/tables_fk--1.0.sql | 20 ++
 src/test/modules/pg_dumprestore/tables_fk.control  |  5 +++
 src/tools/msvc/Mkvcbuild.pm|  3 +-
 src/tools/msvc/vcregress.pl|  3 ++
 9 files changed, 104 insertions(+), 1 deletion(-)
 create mode 100644 src/test/modules/pg_dumprestore/.gitignore
 create mode 100644 src/test/modules/pg_dumprestore/Makefile
 create mode 100644 src/test/modules/pg_dumprestore/README
 create mode 100644 src/test/modules/pg_dumprestore/t/001_dump_restore_test.pl
 create mode 100644 src/test/modules/pg_dumprestore/tables_fk--1.0.sql
 create mode 100644 src/test/modules/pg_dumprestore/tables_fk.control

diff --git a/src/test/modules/Makefile b/src/test/modules/Makefile
index 9b96654..633dc6f 100644
--- a/src/test/modules/Makefile
+++ b/src/test/modules/Makefile
@@ -8,6 +8,7 @@ SUBDIRS = \
 		  brin \
 		  commit_ts \
 		  dummy_seclabel \
+		  pg_dumprestore \
 		  test_ddl_deparse \
 		  test_parser \
 		  test_rls_hooks \
diff --git a/src/test/modules/pg_dumprestore/.gitignore b/src/test/modules/pg_dumprestore/.gitignore
new file mode 100644
index 000..b6a2a01
--- /dev/null
+++ b/src/test/modules/pg_dumprestore/.gitignore
@@ -0,0 +1 @@
+/tmp_check/
diff --git a/src/test/modules/pg_dumprestore/Makefile b/src/test/modules/pg_dumprestore/Makefile
new file mode 100644
index 000..611f23a
--- /dev/null
+++ b/src/test/modules/pg_dumprestore/Makefile
@@ -0,0 +1,24 @@
+# src/test/modules/pg_dumprestore/Makefile
+
+EXTENSION = tables_fk
+DATA = tables_fk--1.0.sql
+PGFILEDESC = "pg_dumprestore - set of extensions dumped and restored"
+
+ifdef USE_PGXS
+PG_CONFIG = pg_config
+PGXS := $(shell $(PG_CONFIG) --pgxs)
+include $(PGXS)
+else
+subdir = src/test/modules/pg_dumprestore
+top_builddir = ../../../..
+include $(top_builddir)/src/Makefile.global
+include $(top_srcdir)/contrib/contrib-global.mk
+endif
+
+check: all
+	$(prove_check)
+
+installcheck: install
+	$(prove_installcheck)
+
+temp-install: EXTRA_INSTALL=src/test/modules/pg_dumprestore
diff --git a/src/test/modules/pg_dumprestore/README b/src/test/modules/pg_dumprestore/README
new file mode 100644
index 000..6932c7e
--- /dev/null
+++ b/src/test/modules/pg_dumprestore/README
@@ -0,0 +1,5 @@
+pg_dumprestore
+==
+
+Facility to test dump and restore of a PostgreSQL instance using a set
+of extensions.
diff --git a/src/test/modules/pg_dumprestore/t/001_dump_restore_test.pl b/src/test/modules/pg_dumprestore/t/001_dump_restore_test.pl
new file mode 100644
index 000..1b57c34
--- /dev/null
+++ b/src/test/modules/pg_dumprestore/t/001_dump_restore_test.pl
@@ -0,0 +1,43 @@
+use strict;
+use warnings;
+use File::Compare;
+use TestLib;
+use Test::More tests => 4;
+
+my $tempdir = tempdir;
+
+start_test_server $tempdir;
+
+my $source_db = 'foobar1';
+my $target_db = 'foobar2';
+my $source_dump = $tempdir . '/dump_' . $source_db . '.sql';
+my 

Re: [HACKERS] Improving test coverage of extensions with pg_dump

2015-09-07 Thread Michael Paquier
On Thu, Sep 3, 2015 at 10:35 AM, Michael Paquier
 wrote:
> On Thu, Sep 3, 2015 at 12:38 AM, Andres Freund wrote:
>> Isn't a full test with a separate initdb, create extension etc. a really
>> heavyhanded way to test this? I mean that's a test where the setup takes
>> up to 10s, whereas the actual runtime is in the millisecond range?
>>
>> Adding tests in this manner doesn't seem scalable to me.
>
> How to include such kind of tests is in the heart of the discussion
> since this patch has showed up. I think we are now close to 5
> different opinions with 4 different hackers on the matter, the Riggs'
> theorem applies nicely.
>
>> I think we should rather add *one* test that does dump/restore over the
>> normal regression test database. Something similar to the pg_upgrade
>> tests. And then work at adding more content to the regression test
>> database - potentially sourcing from src/test/modules.
>
> If you are worrying about run time, pg_upgrade already exactly does
> that. What would be the point to double the amount of time to do the
> same thing in two different places?

So, to summarize the state of this patch whose status is now "Waiting
on Author", we have the following possibilities:
1) Keep the test as-is, as an independent test of src/test/modules.
2) Integrate it in the test suite of src/test/regress and let
pg_upgrade make the work with dump/restore.
3) Create a new facility as src/test/modules/extensions that does a
run of the regression test suite with a set of extensions, and then a
dump/restore. For now the only extension added in the installation is
tables_fk. Future ones would be added here as well, though it is not
clear to me what they are and if we'd have some (there may be... Or
not).
4) Include it as a test of pg_dump in src/bin/pg_dump/t, with the
extension located in for example t/extensions and extend prove_check
to install as well any extensions in t/extensions.

I would still go for 1), the infrastructures included by the other
proposals may become useful depending on the types of tests that are
added in the future, but it is still unclear what those tests are, and
they may even need something else than what listed here (see that as
an example http://www.postgresql.org/message-id/54f62c3f.8070...@gmx.net)
to run properly.
Regards,
-- 
Michael


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


Re: [HACKERS] Improving test coverage of extensions with pg_dump

2015-09-02 Thread Alvaro Herrera
Andres Freund wrote:

> > As I recall, Andrew Dunstan has a module that
> > tests cross-version pg_upgrade and one thing he does is dump both and
> > compare; the problem is that there are differences, so he keeps a count
> > of how many lines he expect to differ between any two releases.
> 
> I'm not suggesting to do anything cross-release - that'd indeed be
> another ballpark.

Ah, you're right.

> Just that instead of a pg_dump test that tests some individual things we
> have one that tests the whole regression test output and then does a
> diff?

Sorry if I'm slow -- A diff against what?

-- 
Álvaro Herrerahttp://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


Re: [HACKERS] Improving test coverage of extensions with pg_dump

2015-09-02 Thread Alvaro Herrera
Andres Freund wrote:

> Isn't a full test with a separate initdb, create extension etc. a really
> heavyhanded way to test this? I mean that's a test where the setup takes
> up to 10s, whereas the actual runtime is in the millisecond range?

I spent some time looking over this patch yesterday, and that was one
concern I had too.  I was thinking that if we turn this into a single
module that can contain several extensions, or several pg_dump-related
tests, and then test multiple features without requiring an initdb for
each, it would be more palatable.

> Adding tests in this manner doesn't seem scalable to me.

I was thinking in having this be renamed src/test/modules/extensions/
and then the extension contained here would be renamed ext001_fk_tables
or something like that; later we could ext002_something for testing some
other angle of extensions, not necessarily pg_dump related.

> I think we should rather add *one* test that does dump/restore over the
> normal regression test database. Something similar to the pg_upgrade
> tests. And then work at adding more content to the regression test
> database - potentially sourcing from src/test/modules.

That's another option, but we've had this idea for many years now and it
hasn't materialized.  As I recall, Andrew Dunstan has a module that
tests cross-version pg_upgrade and one thing he does is dump both and
compare; the problem is that there are differences, so he keeps a count
of how many lines he expect to differ between any two releases.  Or
something like that.  While it's a good enough start, I don't think it's
robust enough to be in core.  How would we do it?

-- 
Álvaro Herrerahttp://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


Re: [HACKERS] Improving test coverage of extensions with pg_dump

2015-09-02 Thread Andres Freund
On 2015-09-02 14:30:33 -0300, Alvaro Herrera wrote:
> I was thinking in having this be renamed src/test/modules/extensions/
> and then the extension contained here would be renamed ext001_fk_tables
> or something like that; later we could ext002_something for testing some
> other angle of extensions, not necessarily pg_dump related.

The largest dataset we have for this is the current regression test
database, it seems a waste not to include it...

> That's another option, but we've had this idea for many years now and it
> hasn't materialized.

But that's just minimally more complex than what's currently done in
that test and in the pg_upgrade test?

> As I recall, Andrew Dunstan has a module that
> tests cross-version pg_upgrade and one thing he does is dump both and
> compare; the problem is that there are differences, so he keeps a count
> of how many lines he expect to differ between any two releases.

I'm not suggesting to do anything cross-release - that'd indeed be
another ballpark.

Just that instead of a pg_dump test that tests some individual things we
have one that tests the whole regression test output and then does a
diff?

Greetings,

Andres Freund


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


Re: [HACKERS] Improving test coverage of extensions with pg_dump

2015-09-02 Thread Andres Freund
On 2015-08-02 19:15:58 -0700, Michael Paquier wrote:
> +psql 'postgres', 'CREATE EXTENSION tables_fk';
> +
> +# Insert some data before running the dump, this is needed to check
> +# consistent data dump of tables with foreign key dependencies
> +psql 'postgres', 'INSERT INTO cc_tab_fkey VALUES (1)';
> +psql 'postgres', 'INSERT INTO bb_tab_fkey VALUES (1)';
> +psql 'postgres', 'INSERT INTO aa_tab_fkey VALUES (1)';
> +
> +# Create a table depending on a FK defined in the extension
> +psql 'postgres', 'CREATE TABLE dd_tab_fkey (id int REFERENCES 
> bb_tab_fkey(id))';
> +psql 'postgres', 'INSERT INTO dd_tab_fkey VALUES (1)';
> +
> +# Take a dump then re-deploy it to a new database
> +command_ok(['pg_dump', '-d', 'postgres', '-f', "$tempdir/dump.sql"],
> +'taken dump with installed extension');
> +command_ok(['createdb', 'foobar1'], 'created new database to redeploy dump');
> +command_ok(['psql', '--set=ON_ERROR_STOP=on', '-d', 'foobar1', '-f',
> + "$tempdir/dump.sql"], 'dump restored');
> +
> +# And check its content
> +my $result = psql 'foobar1',
> + q{SELECT id FROM aa_tab_fkey UNION ALL
> +SELECT id FROM bb_tab_fkey UNION ALL
> +SELECT id FROM cc_tab_fkey UNION ALL
> +SELECT id FROM dd_tab_fkey};
> +is($result, qq(1
> +1
> +1
> +1),
> + 'consistency of data restored');
> diff --git a/src/test/modules/tables_fk/tables_fk--1.0.sql 
> b/src/test/modules/tables_fk/tables_fk--1.0.sql
> new file mode 100644
> index 000..e424610
> --- /dev/null
> +++ b/src/test/modules/tables_fk/tables_fk--1.0.sql
> @@ -0,0 +1,20 @@
> +/* src/test/modules/tables_fk/tables_fk--1.0.sql */
> +
> +-- complain if script is sourced in psql, rather than via CREATE EXTENSION
> +\echo Use "CREATE EXTENSION tables_fk" to load this file. \quit
> +
> +CREATE TABLE IF NOT EXISTS cc_tab_fkey (
> + id int PRIMARY KEY
> +);
> +
> +CREATE TABLE IF NOT EXISTS bb_tab_fkey (
> + id int PRIMARY KEY REFERENCES cc_tab_fkey(id)
> +);
> +
> +CREATE TABLE IF NOT EXISTS aa_tab_fkey (
> + id int REFERENCES bb_tab_fkey(id)
> +);
> +
> +SELECT pg_catalog.pg_extension_config_dump('aa_tab_fkey', '');
> +SELECT pg_catalog.pg_extension_config_dump('bb_tab_fkey', '');
> +SELECT pg_catalog.pg_extension_config_dump('cc_tab_fkey', '');
> diff --git a/src/test/modules/tables_fk/tables_fk.control 
> b/src/test/modules/tables_fk/tables_fk.control

Isn't a full test with a separate initdb, create extension etc. a really
heavyhanded way to test this? I mean that's a test where the setup takes
up to 10s, whereas the actual runtime is in the millisecond range?

Adding tests in this manner doesn't seem scalable to me.

I think we should rather add *one* test that does dump/restore over the
normal regression test database. Something similar to the pg_upgrade
tests. And then work at adding more content to the regression test
database - potentially sourcing from src/test/modules.

Greetings,

Andres Freund


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


Re: [HACKERS] Improving test coverage of extensions with pg_dump

2015-09-02 Thread Michael Paquier
On Thu, Sep 3, 2015 at 2:30 AM, Alvaro Herrera wrote:
> Andres Freund wrote:
>> Isn't a full test with a separate initdb, create extension etc. a really
>> heavyhanded way to test this? I mean that's a test where the setup takes
>> up to 10s, whereas the actual runtime is in the millisecond range?
>
> I spent some time looking over this patch yesterday, and that was one
> concern I had too.  I was thinking that if we turn this into a single
> module that can contain several extensions, or several pg_dump-related
> tests, and then test multiple features without requiring an initdb for
> each, it would be more palatable.

Yeah sure. That's a statement you could generalize for all the TAP
tests, per se for example pg_rewind tests.

>> Adding tests in this manner doesn't seem scalable to me.
>
> I was thinking in having this be renamed src/test/modules/extensions/
> and then the extension contained here would be renamed ext001_fk_tables
> or something like that; later we could ext002_something for testing some
> other angle of extensions, not necessarily pg_dump related.

So, if I am following here correctly, what you are suggesting is:
1) create src/test/modules/extensions/, and include fk_tables in it for now
2) offer the possibility to run make check in this path, doing
complicated tests on a single instance, pg_dump on an instance is one
of them.
Note that for now the TAP test machinery does not allow to share a
single cluster instance across multiple pl scripts, and it seems to me
that's what you are meaning here (in my opinion that's out of sight of
this patch, and I don't think either we should do it this will make
error handling a nightmare). And we can now have a single TAP script
doing all the tests on a single instance. So which one are you
suggesting?

The patch proposed here is actually doing the second, initializing an
instance and performing pg_dump on it. If we change it your way we
should just rename the test script to something like
001_test_extensions.pl and mention in it that new tests for extensions
should be included in it. Still I would imagine that each extension
are actually going to need slightly differ test patterns to work or
cover what they are intended to cover. See for example the set of
tests added in the CREATE EXTENSION CASCADE patch: those are not
adapted to run with the TAP machinery. I may be of course wrong, we
may have other extensions in the future using that, still I am seeing
none around for now.

If you are worrying about run time, we could as well have make check
add an EXTRA_INSTALL pointing to src/test/modules/extensions/, with an
additional test, say extensions.sql that creates data based on it.
Then we let pg_upgrade do the dump/restore checks. This way the run
time of make check-world is minimally impacted, and the code paths we
want tested are done by the buildfarm. That's ugly, still it won't
impact performance.

>> I think we should rather add *one* test that does dump/restore over the
>> normal regression test database. Something similar to the pg_upgrade
>> tests. And then work at adding more content to the regression test
>> database - potentially sourcing from src/test/modules.
>
> That's another option, but we've had this idea for many years now and it
> hasn't materialized.  As I recall, Andrew Dunstan has a module that
> tests cross-version pg_upgrade and one thing he does is dump both and
> compare; the problem is that there are differences, so he keeps a count
> of how many lines he expect to differ between any two releases.  Or
> something like that.  While it's a good enough start, I don't think it's
> robust enough to be in core.  How would we do it?

That does not seem plausible to me for an integration in core, you
would need to keep switching between branches in a given git tree if
the test is being done within a git repository, and that would be just
not doable from a raw tarball which just stores one given version.
-- 
Michael


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


Re: [HACKERS] Improving test coverage of extensions with pg_dump

2015-09-02 Thread Michael Paquier
On Thu, Sep 3, 2015 at 12:38 AM, Andres Freund wrote:
> Isn't a full test with a separate initdb, create extension etc. a really
> heavyhanded way to test this? I mean that's a test where the setup takes
> up to 10s, whereas the actual runtime is in the millisecond range?
>
> Adding tests in this manner doesn't seem scalable to me.

How to include such kind of tests is in the heart of the discussion
since this patch has showed up. I think we are now close to 5
different opinions with 4 different hackers on the matter, the Riggs'
theorem applies nicely.

> I think we should rather add *one* test that does dump/restore over the
> normal regression test database. Something similar to the pg_upgrade
> tests. And then work at adding more content to the regression test
> database - potentially sourcing from src/test/modules.

If you are worrying about run time, pg_upgrade already exactly does
that. What would be the point to double the amount of time to do the
same thing in two different places?
-- 
Michael


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


Re: [HACKERS] Improving test coverage of extensions with pg_dump

2015-08-02 Thread Michael Paquier
On Thu, Jul 30, 2015 at 5:35 PM, Michael Paquier
michael.paqu...@gmail.com wrote:
 Note as well that I will be fine with any decision taken by the
 committer who picks up this, this test case is useful by itself in any
 case.

And I just recalled that I actually did no tests for this thing on
Windows. As this uses the TAP facility, I think that it makes most
sense to run it with tapcheck instead of modulescheck in vcregress.pl
because of its dependency with IPC::Run. The compilation with MSVC is
fixed as well.
-- 
Michael
From a56e2ed296533e30bff47df11f68f0f415ae8a3f Mon Sep 17 00:00:00 2001
From: Michael Paquier mich...@otacoo.com
Date: Sun, 2 Aug 2015 19:11:58 -0700
Subject: [PATCH] Add TAP test for pg_dump checking data dump of extension 
 tables

The test added checks the data dump ordering of tables added in an extension
linked among them with foreign keys. In order to do that, a test extension in
the set of TAP tests of pg_dump, combined with a TAP test script making use
of it.
---
 src/test/modules/Makefile |  1 +
 src/test/modules/tables_fk/.gitignore |  1 +
 src/test/modules/tables_fk/Makefile   | 24 +
 src/test/modules/tables_fk/README |  5 
 src/test/modules/tables_fk/t/001_dump_test.pl | 39 +++
 src/test/modules/tables_fk/tables_fk--1.0.sql | 20 ++
 src/test/modules/tables_fk/tables_fk.control  |  5 
 src/tools/msvc/Mkvcbuild.pm   |  2 +-
 src/tools/msvc/vcregress.pl   |  3 +++
 9 files changed, 99 insertions(+), 1 deletion(-)
 create mode 100644 src/test/modules/tables_fk/.gitignore
 create mode 100644 src/test/modules/tables_fk/Makefile
 create mode 100644 src/test/modules/tables_fk/README
 create mode 100644 src/test/modules/tables_fk/t/001_dump_test.pl
 create mode 100644 src/test/modules/tables_fk/tables_fk--1.0.sql
 create mode 100644 src/test/modules/tables_fk/tables_fk.control

diff --git a/src/test/modules/Makefile b/src/test/modules/Makefile
index 8213e23..683e9cb 100644
--- a/src/test/modules/Makefile
+++ b/src/test/modules/Makefile
@@ -7,6 +7,7 @@ include $(top_builddir)/src/Makefile.global
 SUBDIRS = \
 		  commit_ts \
 		  dummy_seclabel \
+		  tables_fk \
 		  test_ddl_deparse \
 		  test_parser \
 		  test_rls_hooks \
diff --git a/src/test/modules/tables_fk/.gitignore b/src/test/modules/tables_fk/.gitignore
new file mode 100644
index 000..b6a2a01
--- /dev/null
+++ b/src/test/modules/tables_fk/.gitignore
@@ -0,0 +1 @@
+/tmp_check/
diff --git a/src/test/modules/tables_fk/Makefile b/src/test/modules/tables_fk/Makefile
new file mode 100644
index 000..d018517
--- /dev/null
+++ b/src/test/modules/tables_fk/Makefile
@@ -0,0 +1,24 @@
+# src/test/modules/tables_fk/Makefile
+
+EXTENSION = tables_fk
+DATA = tables_fk--1.0.sql
+PGFILEDESC = tables_fk - set of dumpable tables linked by foreign keys
+
+ifdef USE_PGXS
+PG_CONFIG = pg_config
+PGXS := $(shell $(PG_CONFIG) --pgxs)
+include $(PGXS)
+else
+subdir = src/test/modules/tables_fk
+top_builddir = ../../../..
+include $(top_builddir)/src/Makefile.global
+include $(top_srcdir)/contrib/contrib-global.mk
+endif
+
+check: all
+	$(prove_check)
+
+installcheck: install
+	$(prove_installcheck)
+
+temp-install: EXTRA_INSTALL=src/test/modules/tables_fk
diff --git a/src/test/modules/tables_fk/README b/src/test/modules/tables_fk/README
new file mode 100644
index 000..049b75c
--- /dev/null
+++ b/src/test/modules/tables_fk/README
@@ -0,0 +1,5 @@
+tables_fk
+=
+
+Simple module used to check data dump ordering of pg_dump with tables
+linked with foreign keys using TAP tests.
diff --git a/src/test/modules/tables_fk/t/001_dump_test.pl b/src/test/modules/tables_fk/t/001_dump_test.pl
new file mode 100644
index 000..9d3d5ff
--- /dev/null
+++ b/src/test/modules/tables_fk/t/001_dump_test.pl
@@ -0,0 +1,39 @@
+use strict;
+use warnings;
+use TestLib;
+use Test::More tests = 4;
+
+my $tempdir = tempdir;
+
+start_test_server $tempdir;
+
+psql 'postgres', 'CREATE EXTENSION tables_fk';
+
+# Insert some data before running the dump, this is needed to check
+# consistent data dump of tables with foreign key dependencies
+psql 'postgres', 'INSERT INTO cc_tab_fkey VALUES (1)';
+psql 'postgres', 'INSERT INTO bb_tab_fkey VALUES (1)';
+psql 'postgres', 'INSERT INTO aa_tab_fkey VALUES (1)';
+
+# Create a table depending on a FK defined in the extension
+psql 'postgres', 'CREATE TABLE dd_tab_fkey (id int REFERENCES bb_tab_fkey(id))';
+psql 'postgres', 'INSERT INTO dd_tab_fkey VALUES (1)';
+
+# Take a dump then re-deploy it to a new database
+command_ok(['pg_dump', '-d', 'postgres', '-f', $tempdir/dump.sql],
+		   'taken dump with installed extension');
+command_ok(['createdb', 'foobar1'], 'created new database to redeploy dump');
+command_ok(['psql', '--set=ON_ERROR_STOP=on', '-d', 'foobar1', '-f',
+			$tempdir/dump.sql], 'dump restored');
+
+# And check its content
+my $result = psql 'foobar1',
+	q{SELECT id 

Re: [HACKERS] Improving test coverage of extensions with pg_dump

2015-07-30 Thread Michael Paquier
On Fri, Jul 31, 2015 at 6:41 AM, Andreas Karlsson andr...@proxel.se wrote:
 The comment is good, but what I personally do not like is that the path to
 the test suite is non-obvious and not self explanatory I would not expect
 src/test/tables_fk/t/ to test pg_dump and extensions. I find it to confusing
 to regard the test suite as testing an extension called tables_fk since in
 my mind that is just a necessary tool built to test something else.

 This is obviously a subjective thing, and I see that for example Heikki
 likes it the way it is. I am fine with setting this as ready for committer
 and and let a committer weigh in on the naming.

Well, honestly, I would just have it in src/test/modules and call it a
deal. Because it is now the only extension that has such interactions
with perl tests. And because if modules/ gets bloated later on, we
could extend prove_check to install modules from extra paths, just
that it does not seem worth it to do it now for one module, and there
is no guarantee that we'll have that many around. Of course there is
no guarantee either that modules/ is not going to get bloated.

Note as well that I will be fine with any decision taken by the
committer who picks up this, this test case is useful by itself in any
case.
-- 
Michael


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


Re: [HACKERS] Improving test coverage of extensions with pg_dump

2015-07-30 Thread Andreas Karlsson

On 07/30/2015 04:48 AM, Michael Paquier wrote:

On Thu, Jul 30, 2015 at 5:54 AM, Andreas Karlsson andr...@proxel.se wrote:

What I do not like though is how the path src/test/tables_fk/t/ tells us
nothing about what features are of PostgreSQL are tested here. For this I
personally prefer the earlier versions where I think that was clear.


+Simple module used to check data dump ordering of pg_dump with tables
+linked with foreign keys.
The README mentions that this is to test pg_dump, perhaps referring to
the TAP tests makes sense as well?


The comment is good, but what I personally do not like is that the path 
to the test suite is non-obvious and not self explanatory I would not 
expect src/test/tables_fk/t/ to test pg_dump and extensions. I find it 
to confusing to regard the test suite as testing an extension called 
tables_fk since in my mind that is just a necessary tool built to test 
something else.


This is obviously a subjective thing, and I see that for example Heikki 
likes it the way it is. I am fine with setting this as ready for 
committer and and let a committer weigh in on the naming.



Another thought: would it be worthwhile to also add an assertion to check if
the data really was restored properly or would that just be redundant code?


That seems worth doing, hence added something for it. Thanks for the suggestion.

At the same time I have added an entry in .gitignore for the generated
tmp_check/.


Nice changes.

--
Andreas



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


Re: [HACKERS] Improving test coverage of extensions with pg_dump

2015-07-29 Thread Andreas Karlsson
I have reviewed this patch and it compiles runs and the new test case 
passes. The code is also clean and the test seems like a useful 
regression test.


What I do not like though is how the path src/test/tables_fk/t/ tells us 
nothing about what features are of PostgreSQL are tested here. For this 
I personally prefer the earlier versions where I think that was clear.


Another though: would it be worthwhile to also add an assertion to check 
if the data really was restored properly or would that just be redundant 
code?


--
Andreas


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


Re: [HACKERS] Improving test coverage of extensions with pg_dump

2015-07-29 Thread Michael Paquier
On Thu, Jul 30, 2015 at 5:54 AM, Andreas Karlsson andr...@proxel.se wrote:
 What I do not like though is how the path src/test/tables_fk/t/ tells us
 nothing about what features are of PostgreSQL are tested here. For this I
 personally prefer the earlier versions where I think that was clear.

+Simple module used to check data dump ordering of pg_dump with tables
+linked with foreign keys.
The README mentions that this is to test pg_dump, perhaps referring to
the TAP tests makes sense as well?

 Another thought: would it be worthwhile to also add an assertion to check if
 the data really was restored properly or would that just be redundant code?

That seems worth doing, hence added something for it. Thanks for the suggestion.

At the same time I have added an entry in .gitignore for the generated
tmp_check/.
Regards,
-- 
Michael
From 26d507360aa8780ca51884fe3a8d82e5ae967481 Mon Sep 17 00:00:00 2001
From: Michael Paquier mich...@otacoo.com
Date: Thu, 30 Jul 2015 11:46:54 +0900
Subject: [PATCH] Add TAP test for pg_dump checking data dump of extension
 tables

The test added checks the data dump ordering of tables added in an extension
linked among them with foreign keys. In order to do that, a test extension in
the set of TAP tests of pg_dump, combined with a TAP test script making use
of it.
---
 src/test/modules/Makefile |  1 +
 src/test/modules/tables_fk/.gitignore |  1 +
 src/test/modules/tables_fk/Makefile   | 24 +
 src/test/modules/tables_fk/README |  5 
 src/test/modules/tables_fk/t/001_dump_test.pl | 39 +++
 src/test/modules/tables_fk/tables_fk--1.0.sql | 20 ++
 src/test/modules/tables_fk/tables_fk.control  |  5 
 7 files changed, 95 insertions(+)
 create mode 100644 src/test/modules/tables_fk/.gitignore
 create mode 100644 src/test/modules/tables_fk/Makefile
 create mode 100644 src/test/modules/tables_fk/README
 create mode 100644 src/test/modules/tables_fk/t/001_dump_test.pl
 create mode 100644 src/test/modules/tables_fk/tables_fk--1.0.sql
 create mode 100644 src/test/modules/tables_fk/tables_fk.control

diff --git a/src/test/modules/Makefile b/src/test/modules/Makefile
index 8213e23..683e9cb 100644
--- a/src/test/modules/Makefile
+++ b/src/test/modules/Makefile
@@ -7,6 +7,7 @@ include $(top_builddir)/src/Makefile.global
 SUBDIRS = \
 		  commit_ts \
 		  dummy_seclabel \
+		  tables_fk \
 		  test_ddl_deparse \
 		  test_parser \
 		  test_rls_hooks \
diff --git a/src/test/modules/tables_fk/.gitignore b/src/test/modules/tables_fk/.gitignore
new file mode 100644
index 000..b6a2a01
--- /dev/null
+++ b/src/test/modules/tables_fk/.gitignore
@@ -0,0 +1 @@
+/tmp_check/
diff --git a/src/test/modules/tables_fk/Makefile b/src/test/modules/tables_fk/Makefile
new file mode 100644
index 000..d018517
--- /dev/null
+++ b/src/test/modules/tables_fk/Makefile
@@ -0,0 +1,24 @@
+# src/test/modules/tables_fk/Makefile
+
+EXTENSION = tables_fk
+DATA = tables_fk--1.0.sql
+PGFILEDESC = tables_fk - set of dumpable tables linked by foreign keys
+
+ifdef USE_PGXS
+PG_CONFIG = pg_config
+PGXS := $(shell $(PG_CONFIG) --pgxs)
+include $(PGXS)
+else
+subdir = src/test/modules/tables_fk
+top_builddir = ../../../..
+include $(top_builddir)/src/Makefile.global
+include $(top_srcdir)/contrib/contrib-global.mk
+endif
+
+check: all
+	$(prove_check)
+
+installcheck: install
+	$(prove_installcheck)
+
+temp-install: EXTRA_INSTALL=src/test/modules/tables_fk
diff --git a/src/test/modules/tables_fk/README b/src/test/modules/tables_fk/README
new file mode 100644
index 000..049b75c
--- /dev/null
+++ b/src/test/modules/tables_fk/README
@@ -0,0 +1,5 @@
+tables_fk
+=
+
+Simple module used to check data dump ordering of pg_dump with tables
+linked with foreign keys using TAP tests.
diff --git a/src/test/modules/tables_fk/t/001_dump_test.pl b/src/test/modules/tables_fk/t/001_dump_test.pl
new file mode 100644
index 000..9d3d5ff
--- /dev/null
+++ b/src/test/modules/tables_fk/t/001_dump_test.pl
@@ -0,0 +1,39 @@
+use strict;
+use warnings;
+use TestLib;
+use Test::More tests = 4;
+
+my $tempdir = tempdir;
+
+start_test_server $tempdir;
+
+psql 'postgres', 'CREATE EXTENSION tables_fk';
+
+# Insert some data before running the dump, this is needed to check
+# consistent data dump of tables with foreign key dependencies
+psql 'postgres', 'INSERT INTO cc_tab_fkey VALUES (1)';
+psql 'postgres', 'INSERT INTO bb_tab_fkey VALUES (1)';
+psql 'postgres', 'INSERT INTO aa_tab_fkey VALUES (1)';
+
+# Create a table depending on a FK defined in the extension
+psql 'postgres', 'CREATE TABLE dd_tab_fkey (id int REFERENCES bb_tab_fkey(id))';
+psql 'postgres', 'INSERT INTO dd_tab_fkey VALUES (1)';
+
+# Take a dump then re-deploy it to a new database
+command_ok(['pg_dump', '-d', 'postgres', '-f', $tempdir/dump.sql],
+		   'taken dump with installed extension');
+command_ok(['createdb', 'foobar1'], 'created new database to redeploy 

Re: [HACKERS] Improving test coverage of extensions with pg_dump

2015-07-07 Thread Michael Paquier
On Wed, Jul 8, 2015 at 1:32 AM, Heikki Linnakangas hlinn...@iki.fi wrote:
 Hmm. I think it'd be better to put the tables_fk extension into
 src/test/modules, and the test case under src/test/tables_fk/t/. I'm
 inclined to think of this as a test case for an extension that contains a
 table, which includes testing that pg_dump/restore works, rather than as a
 test of pg_dump itself.

The first version of this patch actually did that:
http://www.postgresql.org/message-id/cab7npqqrxhy3+wvuma69kjxirppv5qwji-3cln3zjgbyqe_...@mail.gmail.com
The reason why it has been changed to this way of doing is that there
were concerns about bloating src/test/modules with many similar tests
in the future, like imagine pg_dump_test_1, pg_dump_test_2.

Attached is an updated version that can be used in src/test/modules as
well. Makefile needed also an extra EXTRA_INSTALL pointing to
src/test/modules/tables_fk. Nothing amazing. I have also reworded
one-two things at the same time while looking at that again.
-- 
Michael
From d992811a7657253acce6063595899fba9a8ef102 Mon Sep 17 00:00:00 2001
From: Michael Paquier mich...@otacoo.com
Date: Wed, 8 Jul 2015 13:26:26 +0900
Subject: [PATCH] Add TAP test for pg_dump checking data dump of extension
 tables

The test added checks the data dump ordering of tables added in an extension
linked among them with foreign keys. In order to do that, a test extension in
the set of TAP tests of pg_dump, combined with a TAP test script making use
of it.
---
 src/test/modules/Makefile |  1 +
 src/test/modules/tables_fk/Makefile   | 24 
 src/test/modules/tables_fk/README |  5 +
 src/test/modules/tables_fk/t/001_dump_test.pl | 27 +++
 src/test/modules/tables_fk/tables_fk--1.0.sql | 20 
 src/test/modules/tables_fk/tables_fk.control  |  5 +
 6 files changed, 82 insertions(+)
 create mode 100644 src/test/modules/tables_fk/Makefile
 create mode 100644 src/test/modules/tables_fk/README
 create mode 100644 src/test/modules/tables_fk/t/001_dump_test.pl
 create mode 100644 src/test/modules/tables_fk/tables_fk--1.0.sql
 create mode 100644 src/test/modules/tables_fk/tables_fk.control

diff --git a/src/test/modules/Makefile b/src/test/modules/Makefile
index 8213e23..683e9cb 100644
--- a/src/test/modules/Makefile
+++ b/src/test/modules/Makefile
@@ -7,6 +7,7 @@ include $(top_builddir)/src/Makefile.global
 SUBDIRS = \
 		  commit_ts \
 		  dummy_seclabel \
+		  tables_fk \
 		  test_ddl_deparse \
 		  test_parser \
 		  test_rls_hooks \
diff --git a/src/test/modules/tables_fk/Makefile b/src/test/modules/tables_fk/Makefile
new file mode 100644
index 000..d018517
--- /dev/null
+++ b/src/test/modules/tables_fk/Makefile
@@ -0,0 +1,24 @@
+# src/test/modules/tables_fk/Makefile
+
+EXTENSION = tables_fk
+DATA = tables_fk--1.0.sql
+PGFILEDESC = tables_fk - set of dumpable tables linked by foreign keys
+
+ifdef USE_PGXS
+PG_CONFIG = pg_config
+PGXS := $(shell $(PG_CONFIG) --pgxs)
+include $(PGXS)
+else
+subdir = src/test/modules/tables_fk
+top_builddir = ../../../..
+include $(top_builddir)/src/Makefile.global
+include $(top_srcdir)/contrib/contrib-global.mk
+endif
+
+check: all
+	$(prove_check)
+
+installcheck: install
+	$(prove_installcheck)
+
+temp-install: EXTRA_INSTALL=src/test/modules/tables_fk
diff --git a/src/test/modules/tables_fk/README b/src/test/modules/tables_fk/README
new file mode 100644
index 000..9914f1f
--- /dev/null
+++ b/src/test/modules/tables_fk/README
@@ -0,0 +1,5 @@
+tables_fk
+=
+
+Simple module used to check data dump ordering of pg_dump with tables
+linked with foreign keys.
diff --git a/src/test/modules/tables_fk/t/001_dump_test.pl b/src/test/modules/tables_fk/t/001_dump_test.pl
new file mode 100644
index 000..0953bf5
--- /dev/null
+++ b/src/test/modules/tables_fk/t/001_dump_test.pl
@@ -0,0 +1,27 @@
+use strict;
+use warnings;
+use TestLib;
+use Test::More tests = 3;
+
+my $tempdir = tempdir;
+
+start_test_server $tempdir;
+
+psql 'postgres', 'CREATE EXTENSION tables_fk';
+
+# Insert some data before running the dump, this is needed to check
+# consistent data dump of tables with foreign key dependencies
+psql 'postgres', 'INSERT INTO cc_tab_fkey VALUES (1)';
+psql 'postgres', 'INSERT INTO bb_tab_fkey VALUES (1)';
+psql 'postgres', 'INSERT INTO aa_tab_fkey VALUES (1)';
+
+# Create a table depending on a FK defined in the extension
+psql 'postgres', 'CREATE TABLE dd_tab_fkey (id int REFERENCES bb_tab_fkey(id))';
+psql 'postgres', 'INSERT INTO dd_tab_fkey VALUES (1)';
+
+# Take a dump then re-deploy it to a new database
+command_ok(['pg_dump', '-d', 'postgres', '-f', $tempdir/dump.sql],
+		   'taken dump with installed extension');
+command_ok(['createdb', 'foobar1'], 'created new database to redeploy dump');
+command_ok(['psql', '--set=ON_ERROR_STOP=on', '-d', 'foobar1', '-f',
+			$tempdir/dump.sql], 'dump restored');
diff --git 

Re: [HACKERS] Improving test coverage of extensions with pg_dump

2015-03-07 Thread Michael Paquier
On Tue, Mar 3, 2015 at 2:40 PM, Michael Paquier
michael.paqu...@gmail.com wrote:
 Those patches are really simple, but then perhaps there are better or
 simpler ways than what is attached, so feel free to comment if you
 have any ideas.

Attached are new patches somewhat based on the comments provided by
Peter Eisentraut
(http://www.postgresql.org/message-id/54f62c3f.8070...@gmx.net).
- 0001 makes prove_check install the contents of the path located at
$(CURDIR)/t/extra if present, which would be the path that test
developers could use to store extensions, plugins or modules that
would then be usable by a given set of TAP tests as they are installed
in the temporary installation before launching the tests.
- 0002 is the test for pg_dump checking extensions containing FK
tables, this time integrated as a TAP test in src/bin/pg_dump, with
the extension in src/bin/pg_dump/t/extra.
IMO, this approach is scalable enough thinking long-term. And I think
that the location of those extra extensions is fine in t/ as an
hardcoded subfolder (any fresh idea being of course welcome) as this
makes the stuff in extra/ dedicated to only on set of TAP tests, and
it is even possible to set multiple extensions/modules.
Regards,
-- 
Michael
From e3eec3c8f8af72d3c85e217441009842e09ce1fc Mon Sep 17 00:00:00 2001
From: Michael Paquier michael@otacoo.com
Date: Sat, 7 Mar 2015 21:13:15 +
Subject: [PATCH 1/2] Make prove_check install contents t/extra if present

This gives module developers the possibility to add a set of extensions
and/or tools that will be available in the temporary installation of a
given path, making them available for TAP test.
---
 src/Makefile.global.in | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/Makefile.global.in b/src/Makefile.global.in
index 7c39d82..e856ca8 100644
--- a/src/Makefile.global.in
+++ b/src/Makefile.global.in
@@ -323,6 +323,8 @@ endef
 define prove_check
 $(MKDIR_P) tmp_check/log
 $(MAKE) -C $(top_builddir) DESTDIR='$(CURDIR)'/tmp_check/install install '$(CURDIR)'/tmp_check/log/install.log 21
+# Install extra set of test utilities if present
+@if test -d $(CURDIR)/t/extra; then $(MAKE) -C $(CURDIR)/t/extra DESTDIR='$(CURDIR)'/tmp_check/install install '$(CURDIR)'/tmp_check/log/install.log 21; fi
 cd $(srcdir)  TESTDIR='$(CURDIR)' PATH=$(CURDIR)/tmp_check/install$(bindir):$$PATH $(call add_to_path,$(ld_library_path_var),$(CURDIR)/tmp_check/install$(libdir)) top_builddir='$(CURDIR)/$(top_builddir)' PGPORT='6$(DEF_PGPORT)' $(PROVE) $(PG_PROVE_FLAGS) $(PROVE_FLAGS) t/*.pl
 endef
 
-- 
2.3.1

From 0c7a45348d9a699aeeef6cbc911beca6e6e45235 Mon Sep 17 00:00:00 2001
From: Michael Paquier michael@otacoo.com
Date: Sat, 7 Mar 2015 21:18:58 +
Subject: [PATCH 2/2] Add TAP test for pg_dump checking data dump of extension
 tables

The test added checks the data dump ordering of tables added in an extension
linked among them with foreign keys. In order to do that, a test extension in
the set of TAP tests of pg_dump, combined with a TAP test script making use
of it.
---
 src/bin/pg_dump/.gitignore |  2 ++
 src/bin/pg_dump/Makefile   |  6 +
 src/bin/pg_dump/t/001_dump_test.pl | 27 ++
 src/bin/pg_dump/t/extra/Makefile   | 14 +++
 src/bin/pg_dump/t/extra/tables_fk/Makefile | 16 +
 src/bin/pg_dump/t/extra/tables_fk/README   |  5 
 .../pg_dump/t/extra/tables_fk/tables_fk--1.0.sql   | 20 
 .../pg_dump/t/extra/tables_fk/tables_fk.control|  5 
 8 files changed, 95 insertions(+)
 create mode 100644 src/bin/pg_dump/t/001_dump_test.pl
 create mode 100644 src/bin/pg_dump/t/extra/Makefile
 create mode 100644 src/bin/pg_dump/t/extra/tables_fk/Makefile
 create mode 100644 src/bin/pg_dump/t/extra/tables_fk/README
 create mode 100644 src/bin/pg_dump/t/extra/tables_fk/tables_fk--1.0.sql
 create mode 100644 src/bin/pg_dump/t/extra/tables_fk/tables_fk.control

diff --git a/src/bin/pg_dump/.gitignore b/src/bin/pg_dump/.gitignore
index c2c8677..02666f9 100644
--- a/src/bin/pg_dump/.gitignore
+++ b/src/bin/pg_dump/.gitignore
@@ -3,3 +3,5 @@
 /pg_dump
 /pg_dumpall
 /pg_restore
+
+/tmp_check
diff --git a/src/bin/pg_dump/Makefile b/src/bin/pg_dump/Makefile
index e890a62..1a3f2ca 100644
--- a/src/bin/pg_dump/Makefile
+++ b/src/bin/pg_dump/Makefile
@@ -49,5 +49,11 @@ installdirs:
 uninstall:
 	rm -f $(addprefix '$(DESTDIR)$(bindir)'/, pg_dump$(X) pg_restore$(X) pg_dumpall$(X))
 
+check: all
+	$(prove_check)
+
+installcheck: install
+	$(prove_installcheck)
+
 clean distclean maintainer-clean:
 	rm -f pg_dump$(X) pg_restore$(X) pg_dumpall$(X) $(OBJS) pg_dump.o common.o pg_dump_sort.o pg_restore.o pg_dumpall.o kwlookup.c $(KEYWRDOBJS)
diff --git a/src/bin/pg_dump/t/001_dump_test.pl b/src/bin/pg_dump/t/001_dump_test.pl
new file mode 100644
index 000..0953bf5
--- /dev/null
+++ b/src/bin/pg_dump/t/001_dump_test.pl
@@ -0,0 +1,27 @@
+use strict;
+use