On Tue, Feb 24, 2015 at 2:17 AM, Gilles Darold <gilles.dar...@dalibo.com>
wrote:
> Looks great to me, I have tested with the postgis_topology extension
> everything works fine.
>
Actually, after looking more in depth at the internals of pg_dump I think
that both patches are wrong (did that yesterday night for another patch).
First this patch marks a table in an extension as always dumpable:
+ /* Mark member table as dumpable */
+ configtbl->dobj.dump = true;
And then many checks on ext_member are added in many code paths to ensure
that objects in extensions have their definition never dumped.
But actually this assumption is not true all the time per this code in
getExtensionMemberShip:
if (!dopt->binary_upgrade)
dobj->dump = false;
else
dobj->dump = refdobj->dump;
So this patch would break binary upgrade where some extension objects
should be dumped (one reason why I haven't noticed that before is that
pg_upgrade tests do not include extensions).
Hence, one idea coming to my mind to fix the problem would be to add some
extra processing directly in getExtensionMembership() after building the
objects DO_TABLE_DATA with makeTableDataInfo() by checking the FK
dependencies and add a dependency link with addObjectDependency. The good
part with that is that even user tables that reference extension tables
with a FK can be restored correctly because their constraint is added
*after* loading the data. I noticed as well that with this patch the
--data-only mode was dumping tables in the correct order.
Speaking of which, patches implementing this idea are attached. The module
test case has been improved as well with a check using a table not in an
extension linked with a FK to a table in an extension.
--
Michael
From 7fb84d68df023d913c448f2498987ca4f0a70595 Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@otacoo.com>
Date: Tue, 17 Feb 2015 07:39:23 +0000
Subject: [PATCH 1/3] Fix ordering of tables part of extensions linked with
constraints
Additional checks on FK constraints potentially linking between them
extension objects are done and data dump ordering is ensured. Note
that this does not take into account foreign keys of tables that are
not part of an extension linking to an extension table.
---
src/bin/pg_dump/pg_dump.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 55 insertions(+), 1 deletion(-)
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 2b53c72..5b1b240 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -15236,7 +15236,8 @@ dumpRule(Archive *fout, DumpOptions *dopt, RuleInfo *rinfo)
}
/*
- * getExtensionMembership --- obtain extension membership data
+ * getExtensionMembership --- obtain extension membership data and check FK
+ * dependencies among extension tables.
*/
void
getExtensionMembership(Archive *fout, DumpOptions *dopt, ExtensionInfo extinfo[],
@@ -15423,6 +15424,59 @@ getExtensionMembership(Archive *fout, DumpOptions *dopt, ExtensionInfo extinfo[]
}
}
}
+
+ /*
+ * Now that all the TableInfoData objects have been created for
+ * all the extensions, check their FK dependencies and register
+ * them to ensure correct data ordering. Note that this is not
+ * a problem for user tables not included in an extension
+ * referencing with a FK tables in extensions as their constraint
+ * is declared after dumping their data. In --data-only mode the
+ * table ordering is ensured as well thanks to
+ * getTableDataFKConstraints().
+ */
+ for (j = 0; j < nconfigitems; j++)
+ {
+ int i_confrelid, k;
+ PQExpBuffer query2 = createPQExpBuffer();
+ TableInfo *configtbl;
+ Oid configtbloid = atooid(extconfigarray[j]);
+
+ configtbl = findTableByOid(configtbloid);
+ if (configtbl == NULL || configtbl->dataObj == NULL)
+ continue;
+
+ appendPQExpBuffer(query2,
+ "SELECT confrelid "
+ "FROM pg_catalog.pg_constraint "
+ "WHERE conrelid = '%u' "
+ "AND contype = 'f'",
+ configtbloid);
+
+ res = ExecuteSqlQuery(fout, query2->data, PGRES_TUPLES_OK);
+ ntups = PQntuples(res);
+ i_confrelid = PQfnumber(res, "confrelid");
+
+ for (k = 0; k < ntups; k++)
+ {
+ Oid confrelid;
+ TableInfo *reftable;
+
+ confrelid = atooid(PQgetvalue(res, k, i_confrelid));
+ reftable = findTableByOid(confrelid);
+
+ if (reftable == NULL || reftable->dataObj == NULL)
+ continue;
+
+ /*
+ * Make referencing TABLE_DATA object depend on the
+ * referenced table's TABLE_DATA object.
+ */
+ addObjectDependency(&configtbl->dataObj->dobj,
+ reftable->dataObj->dobj.dumpId);
+ }
+ resetPQExpBuffer(query2);
+ }
}
if (extconfigarray)
free(extconfigarray);
--
2.3.0
From ad5cd360243b44e735195c5e94df3c21e8f18e07 Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@otacoo.com>
Date: Tue, 17 Feb 2015 22:29:28 +0900
Subject: [PATCH 2/3] Make prove_check install contents of current directory as
well
This is useful for example for TAP tests in extensions.
---
src/Makefile.global.in | 1 +
1 file changed, 1 insertion(+)
diff --git a/src/Makefile.global.in b/src/Makefile.global.in
index 7c39d82..7c15423 100644
--- a/src/Makefile.global.in
+++ b/src/Makefile.global.in
@@ -323,6 +323,7 @@ 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 2>&1
+$(MAKE) -C $(CURDIR) DESTDIR='$(CURDIR)'/tmp_check/install install >>'$(CURDIR)'/tmp_check/log/install.log 2>&1
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.0
From 5c7eeb2b1ca1afcf4630a5128979d2d87af19721 Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@otacoo.com>
Date: Tue, 17 Feb 2015 22:36:49 +0900
Subject: [PATCH 3/3] Add dump_test, test module to check pg_dump with
extensions
It works with TAP tests.
---
src/test/modules/Makefile | 1 +
src/test/modules/dump_test/.gitignore | 4 ++++
src/test/modules/dump_test/Makefile | 22 ++++++++++++++++++++++
src/test/modules/dump_test/README | 5 +++++
src/test/modules/dump_test/dump_test--1.0.sql | 20 ++++++++++++++++++++
src/test/modules/dump_test/dump_test.control | 5 +++++
src/test/modules/dump_test/t/001_dump_test.pl | 27 +++++++++++++++++++++++++++
7 files changed, 84 insertions(+)
create mode 100644 src/test/modules/dump_test/.gitignore
create mode 100644 src/test/modules/dump_test/Makefile
create mode 100644 src/test/modules/dump_test/README
create mode 100644 src/test/modules/dump_test/dump_test--1.0.sql
create mode 100644 src/test/modules/dump_test/dump_test.control
create mode 100644 src/test/modules/dump_test/t/001_dump_test.pl
diff --git a/src/test/modules/Makefile b/src/test/modules/Makefile
index 93d93af..6ad3b4e 100644
--- a/src/test/modules/Makefile
+++ b/src/test/modules/Makefile
@@ -6,6 +6,7 @@ include $(top_builddir)/src/Makefile.global
SUBDIRS = \
commit_ts \
+ dump_test \
worker_spi \
dummy_seclabel \
test_shm_mq \
diff --git a/src/test/modules/dump_test/.gitignore b/src/test/modules/dump_test/.gitignore
new file mode 100644
index 0000000..5dcb3ff
--- /dev/null
+++ b/src/test/modules/dump_test/.gitignore
@@ -0,0 +1,4 @@
+# Generated subdirectories
+/log/
+/results/
+/tmp_check/
diff --git a/src/test/modules/dump_test/Makefile b/src/test/modules/dump_test/Makefile
new file mode 100644
index 0000000..08cd215
--- /dev/null
+++ b/src/test/modules/dump_test/Makefile
@@ -0,0 +1,22 @@
+# src/test/modules/dump_test/Makefile
+
+EXTENSION = dump_test
+DATA = dump_test--1.0.sql
+PGFILEDESC = "dump_test - test pg_dump with extensions"
+
+ifdef USE_PGXS
+PG_CONFIG = pg_config
+PGXS := $(shell $(PG_CONFIG) --pgxs)
+include $(PGXS)
+else
+subdir = src/test/modules/dump_test
+top_builddir = ../../../..
+include $(top_builddir)/src/Makefile.global
+include $(top_srcdir)/contrib/contrib-global.mk
+endif
+
+check: all
+ $(prove_check)
+
+installcheck: install
+ $(prove_installcheck)
diff --git a/src/test/modules/dump_test/README b/src/test/modules/dump_test/README
new file mode 100644
index 0000000..9ebfa19
--- /dev/null
+++ b/src/test/modules/dump_test/README
@@ -0,0 +1,5 @@
+dump_test
+=========
+
+Simple module used to test consistency of pg_dump with objects in
+extensions.
diff --git a/src/test/modules/dump_test/dump_test--1.0.sql b/src/test/modules/dump_test/dump_test--1.0.sql
new file mode 100644
index 0000000..d684855
--- /dev/null
+++ b/src/test/modules/dump_test/dump_test--1.0.sql
@@ -0,0 +1,20 @@
+/* src/test/modules/dump_test/dump_test--1.0.sql */
+
+-- complain if script is sourced in psql, rather than via CREATE EXTENSION
+\echo Use "CREATE EXTENSION dump_test" to load this file. \quit
+
+CREATE TABLE IF NOT EXISTS 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/dump_test/dump_test.control b/src/test/modules/dump_test/dump_test.control
new file mode 100644
index 0000000..b32c99c
--- /dev/null
+++ b/src/test/modules/dump_test/dump_test.control
@@ -0,0 +1,5 @@
+# dump_test extension
+comment = 'dump_test - test pg_dump with extensions'
+default_version = '1.0'
+module_pathname = '$libdir/dump_test'
+relocatable = true
diff --git a/src/test/modules/dump_test/t/001_dump_test.pl b/src/test/modules/dump_test/t/001_dump_test.pl
new file mode 100644
index 0000000..18a1135
--- /dev/null
+++ b/src/test/modules/dump_test/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 dump_test';
+
+# 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');
--
2.3.0
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
