On Tue, Jan 20, 2015 at 8:06 PM, Gilles Darold <gilles.dar...@dalibo.com> wrote:
> Le 19/01/2015 14:41, Robert Haas a écrit :
>> On Thu, Jan 15, 2015 at 6:26 AM, Gilles Darold <gilles.dar...@dalibo.com>
>> wrote:
>>> I attach a patch that solves the issue in pg_dump, let me know if it might
>>> be included in Commit Fest or if the three other solutions are a better
>>> choice.
>> I think a fix in pg_dump is appropriate, so I'd encourage you to add
>> this to the next CommitFest.
>>
> Ok, thanks Robert. The patch have been added to next CommitFest under
> the Bug Fixes section.
>
> I've also made some review of the patch and more test. I've rewritten
> some comments and added a test when TableInfo is NULL to avoid potential
> pg_dump crash.
>
> New patch attached: pg_dump.c-extension-FK-v2.patch
So, I looked at your patch and I have some comments...
The approach taken by the patch looks correct to me as we cannot
create FK constraints after loading the data in the case of an
extension, something that can be done with a data-only dump.
Now, I think that the routine hasExtensionMember may impact
performance unnecessarily in the case of databases with many tables,
say thousands or more. And we can easily avoid this performance
problem by checking the content of each object dumped by doing the
legwork directly in getTableData. Also, most of the NULL pointer
checks are not needed as most of those code paths would crash if
tbinfo is NULL, and actually only keeping the one in dumpConstraint is
fine.
On top of those things, I have written a small extension able to
reproduce the problem that I have included as a test module in
src/test/modules. The dump/restore check is done using the TAP tests,
and I have hacked prove_check to install as well the contents of the
current folder to be able to use the TAP routines with an extension
easily. IMO, as we have no coverage of pg_dump with extensions I think
that it would be useful to ensure that this does not break again in
the future.
All the hacking I have done during the review results in the set of
patch attached:
- 0001 is your patch, Gilles, with some comment fixes as well as the
performance issue with hasExtensionMember fix
- 0002 is the modification of prove_check that makes TAP tests usable
with extensions
- 0003 is the test module covering the tests needed for pg_dump, at
least for the problem of this thread.
Gilles, how does that look to you?
(Btw, be sure to generate your patches directly with git next time. :) )
Regards,
--
Michael
From 8005cffd08c57b77564bb0294d8ebd4600bc1dcf 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
The same mechanism as data-only dumps ensuring that data is loaded
respecting foreign key constains is used if it at least one dumped
object is found as being part of an extension. This commit reinforces
as well a couple of code paths to not dump objects that are directly
part of an extension.
Patch by Gilles Darold.
---
src/bin/pg_dump/pg_dump.c | 99 +++++++++++++++++++++++++++++++++++++++--------
1 file changed, 83 insertions(+), 16 deletions(-)
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 7e92b74..c95ae3d 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -208,7 +208,8 @@ static void addBoundaryDependencies(DumpableObject **dobjs, int numObjs,
DumpableObject *boundaryObjs);
static void getDomainConstraints(Archive *fout, TypeInfo *tyinfo);
-static void getTableData(DumpOptions *dopt, TableInfo *tblinfo, int numTables, bool oids);
+static void getTableData(DumpOptions *dopt, TableInfo *tblinfo, int numTables,
+ bool oids, bool *has_ext_member);
static void makeTableDataInfo(DumpOptions *dopt, TableInfo *tbinfo, bool oids);
static void buildMatViewRefreshDependencies(Archive *fout);
static void getTableDataFKConstraints(void);
@@ -730,9 +731,12 @@ main(int argc, char **argv)
if (!dopt.schemaOnly)
{
- getTableData(&dopt, tblinfo, numTables, dopt.oids);
+ bool has_ext_member = false;
+
+ getTableData(&dopt, tblinfo, numTables, dopt.oids, &has_ext_member);
buildMatViewRefreshDependencies(fout);
- if (dopt.dataOnly)
+
+ if (dopt.dataOnly || has_ext_member)
getTableDataFKConstraints();
}
@@ -1866,7 +1870,8 @@ refreshMatViewData(Archive *fout, TableDataInfo *tdinfo)
* set up dumpable objects representing the contents of tables
*/
static void
-getTableData(DumpOptions *dopt, TableInfo *tblinfo, int numTables, bool oids)
+getTableData(DumpOptions *dopt, TableInfo *tblinfo, int numTables,
+ bool oids, bool *has_ext_member)
{
int i;
@@ -1874,6 +1879,8 @@ getTableData(DumpOptions *dopt, TableInfo *tblinfo, int numTables, bool oids)
{
if (tblinfo[i].dobj.dump)
makeTableDataInfo(dopt, &(tblinfo[i]), oids);
+ if (!(*has_ext_member) && tblinfo[i].dobj.ext_member)
+ *has_ext_member = true;
}
}
@@ -2052,13 +2059,15 @@ buildMatViewRefreshDependencies(Archive *fout)
* getTableDataFKConstraints -
* add dump-order dependencies reflecting foreign key constraints
*
- * This code is executed only in a data-only dump --- in schema+data dumps
- * we handle foreign key issues by not creating the FK constraints until
- * after the data is loaded. In a data-only dump, however, we want to
- * order the table data objects in such a way that a table's referenced
- * tables are restored first. (In the presence of circular references or
- * self-references this may be impossible; we'll detect and complain about
- * that during the dependency sorting step.)
+ * This code is executed only in a data-only dump or when an object dumped
+ * is part of an extension -- in schema+data dumps we handle foreign key
+ * issues by not creating the FK constraints until after the data is loaded.
+ * In a data-only dump or when there is an extension member to dump (schema
+ * dumps do not include extension objects, they are created with CREATE
+ * EXTENSION), we want to order the table data objects in such a way that
+ * a table's referenced tables are restored first. (In the presence of
+ * circular references or self-references this may be impossible; we'll
+ * detect and complain about that during the dependency sorting step.)
*/
static void
getTableDataFKConstraints(void)
@@ -2951,9 +2960,14 @@ dumpPolicy(Archive *fout, DumpOptions *dopt, PolicyInfo *polinfo)
PQExpBuffer delqry;
const char *cmd;
+ /* Policies are SCHEMA not data */
if (dopt->dataOnly)
return;
+ /* Ignore extension members */
+ if (tbinfo->dobj.ext_member)
+ return;
+
/*
* If polname is NULL, then this record is just indicating that ROW
* LEVEL SECURITY is enabled for the table. Dump as ALTER TABLE <table>
@@ -7910,6 +7924,10 @@ dumpTableComment(Archive *fout, DumpOptions *dopt, TableInfo *tbinfo,
if (dopt->dataOnly)
return;
+ /* Ignore extension members */
+ if (tbinfo->dobj.ext_member)
+ return;
+
/* Search for comments associated with relation, using table */
ncomments = findComments(fout,
tbinfo->dobj.catId.tableoid,
@@ -13118,6 +13136,10 @@ dumpTableSecLabel(Archive *fout, DumpOptions *dopt, TableInfo *tbinfo, const cha
if (dopt->dataOnly)
return;
+ /* Ignore extension members */
+ if (tbinfo->dobj.ext_member)
+ return;
+
/* Search for comments associated with relation, using table */
nlabels = findSecLabels(fout,
tbinfo->dobj.catId.tableoid,
@@ -13325,7 +13347,7 @@ collectSecLabels(Archive *fout, SecLabelItem **items)
static void
dumpTable(Archive *fout, DumpOptions *dopt, TableInfo *tbinfo)
{
- if (tbinfo->dobj.dump && !dopt->dataOnly)
+ if (tbinfo->dobj.dump && !dopt->dataOnly && !tbinfo->dobj.ext_member)
{
char *namecopy;
@@ -13463,6 +13485,10 @@ dumpTableSchema(Archive *fout, DumpOptions *dopt, TableInfo *tbinfo)
int j,
k;
+ /* Ignore extension members */
+ if (tbinfo->dobj.ext_member)
+ return;
+
/* Make sure we are in proper schema */
selectSourceSchema(fout, tbinfo->dobj.namespace->dobj.name);
@@ -14097,6 +14123,14 @@ dumpAttrDef(Archive *fout, DumpOptions *dopt, AttrDefInfo *adinfo)
if (!tbinfo->dobj.dump || dopt->dataOnly)
return;
+ /* Ignore extension members */
+ if (tbinfo->dobj.ext_member)
+ return;
+
+ /* Ignore extension members */
+ if (tbinfo->dobj.ext_member)
+ return;
+
/* Skip if not "separate"; it was dumped in the table's definition */
if (!adinfo->separate)
return;
@@ -14184,6 +14218,10 @@ dumpIndex(Archive *fout, DumpOptions *dopt, IndxInfo *indxinfo)
if (dopt->dataOnly)
return;
+ /* Ignore extension members */
+ if (tbinfo->dobj.ext_member)
+ return;
+
q = createPQExpBuffer();
delq = createPQExpBuffer();
labelq = createPQExpBuffer();
@@ -14273,6 +14311,10 @@ dumpConstraint(Archive *fout, DumpOptions *dopt, ConstraintInfo *coninfo)
if (!coninfo->dobj.dump || dopt->dataOnly)
return;
+ /* Ignore extension members */
+ if (tbinfo != NULL && tbinfo->dobj.ext_member)
+ return;
+
q = createPQExpBuffer();
delq = createPQExpBuffer();
@@ -14497,7 +14539,13 @@ static void
dumpTableConstraintComment(Archive *fout, DumpOptions *dopt, ConstraintInfo *coninfo)
{
TableInfo *tbinfo = coninfo->contable;
- PQExpBuffer labelq = createPQExpBuffer();
+ PQExpBuffer labelq;
+
+ /* Ignore extension members */
+ if (tbinfo->dobj.ext_member)
+ return;
+
+ labelq = createPQExpBuffer();
appendPQExpBuffer(labelq, "CONSTRAINT %s ",
fmtId(coninfo->dobj.name));
@@ -14574,9 +14622,17 @@ dumpSequence(Archive *fout, DumpOptions *dopt, TableInfo *tbinfo)
char bufm[100],
bufx[100];
bool cycled;
- PQExpBuffer query = createPQExpBuffer();
- PQExpBuffer delqry = createPQExpBuffer();
- PQExpBuffer labelq = createPQExpBuffer();
+ PQExpBuffer query;
+ PQExpBuffer delqry;
+ PQExpBuffer labelq;
+
+ /* Ignore extension members */
+ if (tbinfo->dobj.ext_member)
+ return;
+
+ query = createPQExpBuffer();
+ delqry = createPQExpBuffer();
+ labelq = createPQExpBuffer();
/* Make sure we are in proper schema */
selectSourceSchema(fout, tbinfo->dobj.namespace->dobj.name);
@@ -14841,6 +14897,10 @@ dumpTrigger(Archive *fout, DumpOptions *dopt, TriggerInfo *tginfo)
if (dopt->dataOnly)
return;
+ /* Ignore extension members */
+ if (tbinfo->dobj.ext_member)
+ return;
+
query = createPQExpBuffer();
delqry = createPQExpBuffer();
labelq = createPQExpBuffer();
@@ -15117,6 +15177,10 @@ dumpRule(Archive *fout, DumpOptions *dopt, RuleInfo *rinfo)
if (!rinfo->dobj.dump || dopt->dataOnly)
return;
+ /* Ignore extension members */
+ if (tbinfo->dobj.ext_member)
+ return;
+
/*
* If it is an ON SELECT rule that is created implicitly by CREATE VIEW,
* we do not want to dump it as a separate object.
@@ -15407,6 +15471,9 @@ getExtensionMembership(Archive *fout, DumpOptions *dopt, ExtensionInfo extinfo[]
if (dumpobj)
{
+ /* Mark member table as dumpable */
+ configtbl->dobj.dump = true;
+
/*
* Note: config tables are dumped without OIDs regardless
* of the --oids setting. This is because row filtering
--
2.3.0
From b2f34d2be7445e8e6ee74c64785d0ddae086e88c 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 66862bd62a43182e86bc6fb5f732b722f3741fb0 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 | 15 +++++++++++++++
src/test/modules/dump_test/dump_test.control | 5 +++++
src/test/modules/dump_test/t/001_dump_test.pl | 22 ++++++++++++++++++++++
7 files changed, 74 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..c987e28
--- /dev/null
+++ b/src/test/modules/dump_test/dump_test--1.0.sql
@@ -0,0 +1,15 @@
+/* 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 bb_tab_fkey (
+ id int PRIMARY KEY
+);
+
+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', '');
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..e9bfdb6
--- /dev/null
+++ b/src/test/modules/dump_test/t/001_dump_test.pl
@@ -0,0 +1,22 @@
+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 bb_tab_fkey VALUES (1)';
+psql 'postgres', 'INSERT INTO aa_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
