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