Thread: 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
Attachment
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
Attachment
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
Attachment
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
Attachment
Thanks for the prompt fix, patch [1] works for me. 1] https://postgr.es/m/20180712184537.5vjwgxlbuiomomqd@alvherre.pgsql Regards, Amul
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 Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services