Re: Cannot dump foreign key constraints on partitioned table
On 2018-Jul-13, amul sul wrote: > Thanks for the prompt fix, patch [1] works for me. > > 1] https://postgr.es/m/20180712184537.5vjwgxlbuiomomqd@alvherre.pgsql Thanks for checking, pushed. -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Cannot dump foreign key constraints on partitioned table
Thanks for the prompt fix, patch [1] works for me. 1] https://postgr.es/m/20180712184537.5vjwgxlbuiomomqd@alvherre.pgsql Regards, Amul
Re: Cannot dump foreign key constraints on partitioned table
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 signature.asc Description: PGP signature
Re: Cannot dump foreign key constraints on partitioned table
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 Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Cannot dump foreign key constraints on partitioned table
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 signature.asc Description: PGP signature
Re: Cannot dump foreign key constraints on partitioned table
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 Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>From 05b2d8912688ce6b1c613d7de8a0a3a874e21653 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera
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
Co-authored-by: Michaël Paquier
Co-authored-by: Álvaro Herrera
Discussion:
https://postgr.es/m/CAAJ_b94n=UsNVhgs97vCaWEZAMe-tGDRVuZ73oePQH=eajk...@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 = [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
Re: Cannot dump foreign key constraints on partitioned table
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 Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Cannot dump foreign key constraints on partitioned table
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
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 = [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;
signature.asc
Description: PGP signature
