On Sun, Mar 1, 2015 at 1:09 AM, Stephen Frost <sfr...@snowman.net> wrote:
> Michael,
>
> * Michael Paquier (michael.paqu...@gmail.com) wrote:
>> + /*
>> + * Query all the foreign key dependencies for all the
>> extension
>> + * tables found previously. Only tables whose data
>> need to be
>> + * have to be tracked.
>> + */
>> + appendPQExpBuffer(query2,
>> + "SELECT conrelid, confrelid "
>> + "FROM pg_catalog.pg_constraint "
>> + "WHERE contype = 'f' AND conrelid IN
>> (");
>> +
>> + for (j = 0; j < nconfigitems; j++)
>> + {
>
> [...]
>
> Instead of building up a (potentially) big list like this, couldn't we
> simply join against pg_extension and check if conrelid = ANY (extconfig)
> instead, based on the extension we're currently processing?
>
> eg:
>
> appendPQExpBuffer(query2,
> "SELECT conrelid, confrelid "
> "FROM pg_catalog.pg_constraint,
> pg_extension "
> "WHERE contype = 'f' AND extname = '%s' AND
> conrelid = ANY (extconfig)",
> fmtId(curext->dobj.name));
>
> This seemed to work just fine for me, at least, and reduces the size of
> the patch a bit further since we don't need the loop that builds up the
> ids.
Actually, I did a mistake in my last patch, see this comment:
+ * Now that all the TableInfoData objects have been created for
+ * all the extensions, check their FK dependencies and register
+ * them to ensure correct data ordering.
The thing is that we must absolutely wait for *all* the TableInfoData
of all the extensions to be created because we need to mark the
dependencies between them, and even my last patch, or even with what
you are proposing we may miss tracking of FK dependencies between
tables in different extensions. This has little chance to happen in
practice, but I think that we should definitely get things right.
Hence something like this query able to query all the FK dependencies
with tables in extensions is more useful, and it has no IN clause:
+ appendPQExpBuffer(query,
+ "SELECT conrelid, confrelid "
+ "FROM pg_constraint "
+ "LEFT JOIN pg_depend ON (objid = confrelid) "
+ "WHERE contype = 'f' "
+ "AND refclassid = 'pg_extension'::regclass "
+ "AND classid = 'pg_class'::regclass;");
>> Subject: [PATCH 2/3] Make prove_check install contents of current directory
>> as well
>
> This is really an independent thing, no? I don't see any particular
> problem with it, for my part.
Yes, that's an independent thing, but my guess is that it would be
good to have a real test case this time to be sure that this does not
break again, at least on master where test/modules is an ideal place.
Attached are updated patches, the fix of pg_dump can be easily
cherry-picked down to 9.2. For 9.1 things are changed a bit because of
the way SQL queries are run, still patches are attached for all the
versions needed. I tested as well the fix down to this version 9.1
using the test case dump_test.
Thanks,
--
Michael
From 9ef14f2f67cbec9df2a1d30978efdeda059fa12f Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@otacoo.com>
Date: Fri, 27 Feb 2015 12:23:42 +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.
---
src/bin/pg_dump/pg_dump.c | 58 +++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 56 insertions(+), 2 deletions(-)
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 2b53c72..e210f88 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 relations interacting with the ones in extensions.
*/
void
getExtensionMembership(Archive *fout, DumpOptions *dopt, ExtensionInfo extinfo[],
@@ -15249,7 +15250,9 @@ getExtensionMembership(Archive *fout, DumpOptions *dopt, ExtensionInfo extinfo[]
int i_classid,
i_objid,
i_refclassid,
- i_refobjid;
+ i_refobjid,
+ i_conrelid,
+ i_confrelid;
DumpableObject *dobj,
*refdobj;
@@ -15431,6 +15434,57 @@ getExtensionMembership(Archive *fout, DumpOptions *dopt, ExtensionInfo extinfo[]
}
destroyPQExpBuffer(query);
+
+ /*
+ * 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().
+ */
+ query = createPQExpBuffer();
+
+ appendPQExpBuffer(query,
+ "SELECT conrelid, confrelid "
+ "FROM pg_constraint "
+ "LEFT JOIN pg_depend ON (objid = confrelid) "
+ "WHERE contype = 'f' "
+ "AND refclassid = 'pg_extension'::regclass "
+ "AND classid = 'pg_class'::regclass;");
+
+ res = ExecuteSqlQuery(fout, query->data, PGRES_TUPLES_OK);
+ ntups = PQntuples(res);
+
+ i_conrelid = PQfnumber(res, "conrelid");
+ i_confrelid = PQfnumber(res, "confrelid");
+
+ /* Now get the dependencies and register them */
+ for (i = 0; i < ntups; i++)
+ {
+ Oid conrelid, confrelid;
+ TableInfo *reftable, *contable;
+
+ conrelid = atooid(PQgetvalue(res, i, i_conrelid));
+ confrelid = atooid(PQgetvalue(res, i, i_confrelid));
+ contable = findTableByOid(conrelid);
+ reftable = findTableByOid(confrelid);
+
+ if (reftable == NULL ||
+ reftable->dataObj == NULL ||
+ contable == NULL ||
+ contable->dataObj == NULL)
+ continue;
+
+ /*
+ * Make referencing TABLE_DATA object depend on the
+ * referenced table's TABLE_DATA object.
+ */
+ addObjectDependency(&contable->dataObj->dobj,
+ reftable->dataObj->dobj.dumpId);
+ }
+ resetPQExpBuffer(query);
}
/*
--
2.3.1
From cd429895eab802310df5b2b9a14ca236c9f3f395 Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@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.1
From e0dd2f7dffe8f3c1a551130c9b44ec6e79f11190 Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@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.1
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index fd47059..7e17df0 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -13738,7 +13738,8 @@ dumpRule(Archive *fout, RuleInfo *rinfo)
}
/*
- * getExtensionMembership --- obtain extension membership data
+ * getExtensionMembership --- obtain extension membership data and check FK
+ * dependencies among relations interacting with the ones in extensions.
*/
void
getExtensionMembership(ExtensionInfo extinfo[], int numExtensions)
@@ -13750,7 +13751,9 @@ getExtensionMembership(ExtensionInfo extinfo[], int numExtensions)
int i_classid,
i_objid,
i_refclassid,
- i_refobjid;
+ i_refobjid,
+ i_conrelid,
+ i_confrelid;
DumpableObject *dobj,
*refdobj;
@@ -13930,6 +13933,59 @@ getExtensionMembership(ExtensionInfo extinfo[], int numExtensions)
}
destroyPQExpBuffer(query);
+
+ /*
+ * 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().
+ */
+ query = createPQExpBuffer();
+
+ appendPQExpBuffer(query,
+ "SELECT conrelid, confrelid "
+ "FROM pg_constraint "
+ "LEFT JOIN pg_depend ON (objid = confrelid) "
+ "WHERE contype = 'f' "
+ "AND refclassid = 'pg_extension'::regclass "
+ "AND classid = 'pg_class'::regclass;");
+
+ res = PQexec(g_conn, query->data);
+ check_sql_result(res, g_conn, query->data, PGRES_TUPLES_OK);
+
+ ntups = PQntuples(res);
+
+ i_conrelid = PQfnumber(res, "conrelid");
+ i_confrelid = PQfnumber(res, "confrelid");
+
+ /* Now get the dependencies and register them */
+ for (i = 0; i < ntups; i++)
+ {
+ Oid conrelid, confrelid;
+ TableInfo *reftable, *contable;
+
+ conrelid = atooid(PQgetvalue(res, i, i_conrelid));
+ confrelid = atooid(PQgetvalue(res, i, i_confrelid));
+ contable = findTableByOid(conrelid);
+ reftable = findTableByOid(confrelid);
+
+ if (reftable == NULL ||
+ reftable->dataObj == NULL ||
+ contable == NULL ||
+ contable->dataObj == NULL)
+ continue;
+
+ /*
+ * Make referencing TABLE_DATA object depend on the
+ * referenced table's TABLE_DATA object.
+ */
+ addObjectDependency(&contable->dataObj->dobj,
+ reftable->dataObj->dobj.dumpId);
+ }
+ resetPQExpBuffer(query);
}
/*
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
