On Sat, Sep 26, 2015 at 10:22 PM, Andres Freund <and...@anarazel.de> wrote:
> On 2015-09-26 21:54:46 +0900, Michael Paquier wrote:
>> On Wed, Sep 23, 2015 at 1:04 AM, Alvaro Herrera <alvhe...@2ndquadrant.com>
>> 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 <mich...@otacoo.com>
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 to target.
For now this test facility includes one extension to test dumpable objects
with foreign keys, but could be extended with more stuff in the future to
test pg_dump and its restoration.
---
src/test/modules/Makefile | 1 +
src/test/modules/pg_dumprestore/.gitignore | 2 ++
src/test/modules/pg_dumprestore/Makefile | 24 +++++++++++++
src/test/modules/pg_dumprestore/README | 5 +++
src/test/modules/pg_dumprestore/dump_schedule | 3 ++
.../modules/pg_dumprestore/expected/tables_fk.out | 14 ++++++++
src/test/modules/pg_dumprestore/sql/tables_fk.sql | 18 ++++++++++
.../pg_dumprestore/t/001_dump_restore_test.pl | 42 ++++++++++++++++++++++
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, 139 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/dump_schedule
create mode 100644 src/test/modules/pg_dumprestore/expected/tables_fk.out
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 0000000..6cc525b
--- /dev/null
+++ b/src/test/modules/pg_dumprestore/.gitignore
@@ -0,0 +1,2 @@
+/tmp_check/
+/results/
diff --git a/src/test/modules/pg_dumprestore/Makefile b/src/test/modules/pg_dumprestore/Makefile
new file mode 100644
index 0000000..ec88186
--- /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 - test suite with extensions for dump and restore"
+
+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 0000000..49c861e
--- /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 and a dedicated regression suite.
diff --git a/src/test/modules/pg_dumprestore/dump_schedule b/src/test/modules/pg_dumprestore/dump_schedule
new file mode 100644
index 0000000..ecefc2d
--- /dev/null
+++ b/src/test/modules/pg_dumprestore/dump_schedule
@@ -0,0 +1,3 @@
+# src/test/modules/pg_dumprestore/extension_schedule
+# Schedule for extensions in this test module
+test: tables_fk
diff --git a/src/test/modules/pg_dumprestore/expected/tables_fk.out b/src/test/modules/pg_dumprestore/expected/tables_fk.out
new file mode 100644
index 0000000..61cad6b
--- /dev/null
+++ b/src/test/modules/pg_dumprestore/expected/tables_fk.out
@@ -0,0 +1,14 @@
+--
+-- TABLES_FK
+--
+CREATE EXTENSION tables_fk;
+-- Insert some data before running the dump on tables of tables_fk, this
+-- is needed to check consistent data dump of tables with foreign key
+-- dependencies.
+INSERT INTO cc_tab_fkey VALUES (1);
+INSERT INTO bb_tab_fkey VALUES (1);
+INSERT INTO aa_tab_fkey VALUES (1);
+-- Create a table depending on a FK defined in tables_fk.
+CREATE TABLE dd_tab_fkey (id int REFERENCES bb_tab_fkey(id));
+INSERT INTO dd_tab_fkey VALUES (1);
+-- Do not drop objects at the end of test
diff --git a/src/test/modules/pg_dumprestore/sql/tables_fk.sql b/src/test/modules/pg_dumprestore/sql/tables_fk.sql
new file mode 100644
index 0000000..063ad88
--- /dev/null
+++ b/src/test/modules/pg_dumprestore/sql/tables_fk.sql
@@ -0,0 +1,18 @@
+--
+-- TABLES_FK
+--
+
+CREATE EXTENSION tables_fk;
+
+-- Insert some data before running the dump on tables of tables_fk, this
+-- is needed to check consistent data dump of tables with foreign key
+-- dependencies.
+INSERT INTO cc_tab_fkey VALUES (1);
+INSERT INTO bb_tab_fkey VALUES (1);
+INSERT INTO aa_tab_fkey VALUES (1);
+
+-- Create a table depending on a FK defined in tables_fk.
+CREATE TABLE dd_tab_fkey (id int REFERENCES bb_tab_fkey(id));
+INSERT INTO dd_tab_fkey VALUES (1);
+
+-- Do not drop objects at the end of test
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 0000000..01e02a1
--- /dev/null
+++ b/src/test/modules/pg_dumprestore/t/001_dump_restore_test.pl
@@ -0,0 +1,42 @@
+use strict;
+use warnings;
+use Cwd;
+use File::Compare;
+use TestLib;
+use Test::More tests => 4;
+
+my $tempdir = tempdir;
+
+start_test_server $tempdir;
+
+my $source_db = 'regress_source';
+my $target_db = 'regress_target';
+my $source_dump = $tempdir . '/dump_' . $source_db . '.sql';
+my $target_dump = $tempdir . '/dump_' . $target_db . '.sql';
+system_or_bail 'createdb', $source_db;
+system_or_bail 'createdb', $target_db;
+
+# And finish with the extra schedule using extensions
+system_or_bail($ENV{PG_REGRESS},
+ "--dlpath=.",
+ "--dbname=$source_db",
+ "--use-existing",
+ "--dlpath=.",
+ "--bindir=../../../../src/bin/psql",
+ "--schedule=./dump_schedule",
+ "--no-locale");
+
+# Take a dump from source then re-deploy it to target database.
+command_ok(['pg_dump',
+ '-d', $source_db, '-f', $source_dump],
+ 'dump taken from source database');
+command_ok(['psql', '--set=ON_ERROR_STOP=on', '-d', $target_db, '-f',
+ $source_dump], 'dump of source restored on target database');
+
+# Finish by taking a dump of the target database and then compare it
+# with the one from source.
+command_ok(['pg_dump', '-d', $target_db, '-f', $target_dump],
+ 'dump taken from target database');
+
+my $result = compare($source_dump, $target_dump) == 0;
+ok($result, "Diff between source and target dump");
diff --git a/src/test/modules/pg_dumprestore/tables_fk--1.0.sql b/src/test/modules/pg_dumprestore/tables_fk--1.0.sql
new file mode 100644
index 0000000..e424610
--- /dev/null
+++ b/src/test/modules/pg_dumprestore/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/pg_dumprestore/tables_fk.control b/src/test/modules/pg_dumprestore/tables_fk.control
new file mode 100644
index 0000000..b9f31ee
--- /dev/null
+++ b/src/test/modules/pg_dumprestore/tables_fk.control
@@ -0,0 +1,5 @@
+# tables_fk extension
+comment = 'tables_fk - dumpable tables linked with foreign keys'
+default_version = '1.0'
+module_pathname = '$libdir/tables_fk'
+relocatable = true
diff --git a/src/tools/msvc/Mkvcbuild.pm b/src/tools/msvc/Mkvcbuild.pm
index 3abbb4c..71e0749 100644
--- a/src/tools/msvc/Mkvcbuild.pm
+++ b/src/tools/msvc/Mkvcbuild.pm
@@ -41,7 +41,8 @@ my $contrib_extrasource = {
'seg' => [ 'contrib/seg/segscan.l', 'contrib/seg/segparse.y' ], };
my @contrib_excludes = (
'commit_ts', 'hstore_plperl', 'hstore_plpython', 'intagg',
- 'ltree_plpython', 'pgcrypto', 'sepgsql', 'brin');
+ 'ltree_plpython', 'pgcrypto', 'sepgsql', 'brin',
+ 'pg_dumprestore');
# Set of variables for frontend modules
my $frontend_defines = { 'initdb' => 'FRONTEND' };
diff --git a/src/tools/msvc/vcregress.pl b/src/tools/msvc/vcregress.pl
index d3d736b..c1bd98b 100644
--- a/src/tools/msvc/vcregress.pl
+++ b/src/tools/msvc/vcregress.pl
@@ -344,6 +344,9 @@ sub modulescheck
my $mstat = 0;
foreach my $module (glob("*"))
{
+ # Already tested by tapcheck
+ next if ($module eq "pg_dumprestore");
+
subdircheck("$topdir/src/test/modules", $module);
my $status = $? >> 8;
$mstat ||= $status;
--
2.5.3
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
