Обсуждение: 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

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

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]  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