Cannot dump foreign key constraints on partitioned table
Hi,
On the master head, getConstraints() function skips FK constraints for
a partitioned table because of tbinfo->hastriggers is false.
While creating FK constraints on the partitioned table, the FK triggers are only
created on leaf partitions and skipped for the partitioned tables.
To fix this, either bypass the aforementioned condition of getConstraints() or
set pg_class.relhastriggers to true for the partitioned table which doesn't seem
to be the right solution, IMO. Thoughts?
Regards,
Amul
On Wed, Jul 11, 2018 at 03:49:59PM +0530, amul sul wrote:
On the master head, getConstraints() function skips FK constraints for
a partitioned table because of tbinfo->hastriggers is false.While creating FK constraints on the partitioned table, the FK triggers are only
created on leaf partitions and skipped for the partitioned tables.
Oops. Good catch.
To fix this, either bypass the aforementioned condition of getConstraints() or
set pg_class.relhastriggers to true for the partitioned table which doesn't seem
to be the right solution, IMO. Thoughts?
Changing pg_class.relhastriggers is out of scope because as far as I
know partitioned tables have no triggers, so the current value is
correct, and that would be a catalog change at this stage which would
cause any existing deployments of v11 to complain about the
inconsistency. I think that this should be fixed client-side as the
attached does.
I have just stolen this SQL set from Alvaro to check the consistency of
the dumps created:
create table prim (a int primary key);
create table partfk (a int references prim) partition by range (a);
create table partfk1 partition of partfk for values from (0) to (100);
create table partfk2 partition of partfk for values from (100) to (200);
Thoughts?
--
Michael
Attachments:
dump-partition-fk.patchtext/x-diff; charset=us-asciiDownload
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 463639208d..74a1270169 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -7131,7 +7131,12 @@ getConstraints(Archive *fout, TableInfo tblinfo[], int numTables)
{
TableInfo *tbinfo = &tblinfo[i];
- if (!tbinfo->hastriggers ||
+ /*
+ * For partitioned tables, foreign keys have no triggers so they
+ * must be included anyway in case some foreign keys are defined.
+ */
+ if ((!tbinfo->hastriggers &&
+ tbinfo->relkind != RELKIND_PARTITIONED_TABLE) ||
!(tbinfo->dobj.dump & DUMP_COMPONENT_DEFINITION))
continue;
On 2018-Jul-12, Michael Paquier wrote:
On Wed, Jul 11, 2018 at 03:49:59PM +0530, amul sul wrote:
On the master head, getConstraints() function skips FK constraints for
a partitioned table because of tbinfo->hastriggers is false.While creating FK constraints on the partitioned table, the FK triggers are only
created on leaf partitions and skipped for the partitioned tables.Oops. Good catch.
Looking at it now.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2018-Jul-12, Michael Paquier wrote:
Changing pg_class.relhastriggers is out of scope because as far as I
know partitioned tables have no triggers, so the current value is
correct, and that would be a catalog change at this stage which would
cause any existing deployments of v11 to complain about the
inconsistency. I think that this should be fixed client-side as the
attached does.
Thanks, looks good. I propose to add following pg_dump test to ensure
this stays fixed.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachments:
0001-Dump-foreign-keys-on-partitioned-tables.patchtext/plain; charset=iso-8859-1Download
From 05b2d8912688ce6b1c613d7de8a0a3a874e21653 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
Date: Thu, 12 Jul 2018 14:36:11 -0400
Subject: [PATCH] Dump foreign keys on partitioned tables
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
My patch that ended up as commit 3de241dba86f ("Foreign keys on
partitioned tables") lacked pg_dump tests, so the pg_dump code that was
there to support it inadvertently stopped working when I hacked the
backend code to not emit pg_trigger rows for the partitioned table
itself.
Bug analysis and code fix is by Michaël. I (Ãlvaro) merely added the
test.
Reported-by: amul sul <sulamul@gmail.com>
Co-authored-by: Michaël Paquier <michael@paquier.xyz>
Co-authored-by: Ãlvaro Herrera <alvherre@alvh.no-ip.org>
Discussion: https://postgr.es/m/CAAJ_b94n=UsNVhgs97vCaWEZAMe-tGDRVuZ73oePQH=eaJKGSA@mail.gmail.com
---
src/bin/pg_dump/pg_dump.c | 7 ++++++-
src/bin/pg_dump/t/002_pg_dump.pl | 18 ++++++++++++++++++
2 files changed, 24 insertions(+), 1 deletion(-)
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 463639208d..74a1270169 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -7131,7 +7131,12 @@ getConstraints(Archive *fout, TableInfo tblinfo[], int numTables)
{
TableInfo *tbinfo = &tblinfo[i];
- if (!tbinfo->hastriggers ||
+ /*
+ * For partitioned tables, foreign keys have no triggers so they
+ * must be included anyway in case some foreign keys are defined.
+ */
+ if ((!tbinfo->hastriggers &&
+ tbinfo->relkind != RELKIND_PARTITIONED_TABLE) ||
!(tbinfo->dobj.dump & DUMP_COMPONENT_DEFINITION))
continue;
diff --git a/src/bin/pg_dump/t/002_pg_dump.pl b/src/bin/pg_dump/t/002_pg_dump.pl
index 7eee870259..8860928df1 100644
--- a/src/bin/pg_dump/t/002_pg_dump.pl
+++ b/src/bin/pg_dump/t/002_pg_dump.pl
@@ -631,6 +631,24 @@ my %tests = (
},
},
+ 'ALTER TABLE (partitioned) ADD CONSTRAINT ... FOREIGN KEY' => {
+ create_order => 4,
+ create_sql => 'CREATE TABLE dump_test.test_table_fk (
+ col1 int references dump_test.test_table)
+ PARTITION BY RANGE (col1);',
+ regexp => qr/
+ \QADD CONSTRAINT test_table_fk_col1_fkey FOREIGN KEY (col1) REFERENCES dump_test.test_table\E
+ /xm,
+ like => {
+ %full_runs,
+ %dump_test_schema_runs,
+ section_post_data => 1,
+ },
+ unlike => {
+ exclude_dump_test_schema => 1,
+ },
+ },
+
'ALTER TABLE ONLY test_table ALTER COLUMN col1 SET STATISTICS 90' => {
create_order => 93,
create_sql =>
--
2.11.0
On Thu, Jul 12, 2018 at 02:45:37PM -0400, Alvaro Herrera wrote:
Thanks, looks good. I propose to add following pg_dump test to ensure
this stays fixed.
Thanks for adding the test. I was looking at a good way to add a test
but could not come up with something which can be summed up with one
query for create_sql, so what you have here is nice. Could you add an
extra test with a partition of dump_test.test_table_fk? Children should
have the FK defined as well with relhastriggers set to true, still when
I tested if the partitioned was not scanned for its FK, then its
children partition also missed it. So I think that it is important to
check that the FK is defined for all members of the partition tree.
I am fine to add the test myself and to push if you need help. Of
course feel free to do it yourself if you want. Either way is fine for
me.
--
Michael
On 2018-Jul-13, Michael Paquier wrote:
On Thu, Jul 12, 2018 at 02:45:37PM -0400, Alvaro Herrera wrote:
Thanks, looks good. I propose to add following pg_dump test to ensure
this stays fixed.Thanks for adding the test. I was looking at a good way to add a test
but could not come up with something which can be summed up with one
query for create_sql, so what you have here is nice. Could you add an
extra test with a partition of dump_test.test_table_fk? Children should
have the FK defined as well with relhastriggers set to true, still when
I tested if the partitioned was not scanned for its FK, then its
children partition also missed it. So I think that it is important to
check that the FK is defined for all members of the partition tree.
Hmm. The pg_dump tests make it easy to create a partition (in fact I
had already modified the test to add one after submitting):
diff --git a/src/bin/pg_dump/t/002_pg_dump.pl b/src/bin/pg_dump/t/002_pg_dump.pl
index 8860928df1..666760c0d8 100644
--- a/src/bin/pg_dump/t/002_pg_dump.pl
+++ b/src/bin/pg_dump/t/002_pg_dump.pl
@@ -635,7 +635,10 @@ my %tests = (
create_order => 4,
create_sql => 'CREATE TABLE dump_test.test_table_fk (
col1 int references dump_test.test_table)
- PARTITION BY RANGE (col1);',
+ PARTITION BY RANGE (col1);
+ CREATE TABLE dump_test.test_table_fk_1
+ PARTITION OF dump_test.test_table_fk
+ FOR VALUES FROM (0) TO (10);',
regexp => qr/
\QADD CONSTRAINT test_table_fk_col1_fkey FOREIGN KEY (col1) REFERENCES dump_test.test_table\E
/xm,I'm not sure what to *do* with the partition, though :-) I don't think
there's a nice way to verify that the FK actually exists, or that
catalog rows are set in such-and-such way, after restoring this.
The pg_restore tests are marked TODO in the suite. I think that'll have
to wait.
I am fine to add the test myself and to push if you need help. Of
course feel free to do it yourself if you want. Either way is fine for
me.
No worries -- I'll push it tomorrow morning.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Thu, Jul 12, 2018 at 11:34:43PM -0400, Alvaro Herrera wrote:
I'm not sure what to *do* with the partition, though :-) I don't think
there's a nice way to verify that the FK actually exists, or that
catalog rows are set in such-and-such way, after restoring this.
The pg_restore tests are marked TODO in the suite. I think that'll have
to wait.
Ouch, right. My eyes are betraying me. I swear when I tested that I
saw an ALTER TABLE ADD CONSTRAINT command generated as well for each
partition on top of the parent :)
But only the parent needs to have the definition, so your test is
sufficient. Sorry for the noise.
--
Michael
Thanks for the prompt fix, patch [1] works for me.
1] /messages/by-id/20180712184537.5vjwgxlbuiomomqd@alvherre.pgsql
Regards,
Amul
On 2018-Jul-13, amul sul wrote:
Thanks for the prompt fix, patch [1] works for me.
1] /messages/by-id/20180712184537.5vjwgxlbuiomomqd@alvherre.pgsql
Thanks for checking, pushed.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services