Re: problems with foreign keys on partitioned tables - Mailing list pgsql-hackers

On 2019/01/18 7:54, Alvaro Herrera wrote:
> On 2019-Jan-09, Amit Langote wrote:
> 
>> 1. Foreign keys of partitions stop working correctly after being detached
>> from the parent table
> 
>> This happens because the action triggers defined on the PK relation (pk)
>> refers to p as the referencing relation.  On detaching p1 from p, p1's
>> data is no longer accessible to that trigger.
> 
> Ouch.
> 
>> To fix this problem, we need create action triggers on PK relation
>> that refer to p1 when it's detached (unless such triggers already
>> exist which might be true in some cases).  Attached patch 0001 shows
>> this approach.
> 
> Hmm, okay.  I'm not in love with the idea that such triggers might
> already exist -- seems unclean.  We should remove redundant action
> triggers when we attach a table as a partition, no?

OK, I agree.  I have updated the patch to make things work that way.  With
the patch:

create table pk (a int primary key);
create table p (a int references pk) partition by list (a);

-- this query shows the action triggers on the referenced rel ('pk'), name
-- of the constraint that the trigger is part of and the foreign key rel
-- ('p', etc.)

select tgrelid::regclass as pkrel, c.conname as fkconname,
tgconstrrelid::regclass as fkrel from pg_trigger t, pg_constraint c where
tgrelid = 'pk'::regclass and tgconstraint = c.oid;
 pkrel │ fkconname │ fkrel
───────┼───────────┼───────
 pk    │ p_a_fkey  │ p
 pk    │ p_a_fkey  │ p
(2 rows)

create table p1 (
   a int references pk,
   foreign key (a) references pk (a) ON UPDATE CASCADE ON DELETE CASCADE
DEFERRABLE,
   foreign key (a) references pk (a) MATCH FULL ON UPDATE CASCADE ON
DELETE CASCADE
) partition by list (a);

-- p1_a_fkey on 'p1' is equivalent to p_a_fkey on 'p', but they're not
-- attached yet

select tgrelid::regclass as pkrel, conname as fkconname,
tgconstrrelid::regclass as fkrel from pg_trigger t, pg_constraint c where
tgrelid = 'pk'::regclass and tgconstraint = c.oid;
 pkrel │ fkconname  │ fkrel
───────┼────────────┼───────
 pk    │ p_a_fkey   │ p
 pk    │ p_a_fkey   │ p
 pk    │ p1_a_fkey  │ p1
 pk    │ p1_a_fkey  │ p1
 pk    │ p1_a_fkey1 │ p1
 pk    │ p1_a_fkey1 │ p1
 pk    │ p1_a_fkey2 │ p1
 pk    │ p1_a_fkey2 │ p1
(8 rows)

create table p11 (like p1, foreign key (a) references pk);

-- again, p11_a_fkey, p1_a_fkey, and p_a_fkey are equivalent

select tgrelid::regclass as pkrel, conname as fkconname,
tgconstrrelid::regclass as fkrel from pg_trigger t, pg_constraint c where
tgrelid = 'pk'::regclass and tgconstraint = c.oid;
 pkrel │ fkconname  │ fkrel
───────┼────────────┼───────
 pk    │ p_a_fkey   │ p
 pk    │ p_a_fkey   │ p
 pk    │ p1_a_fkey  │ p1
 pk    │ p1_a_fkey  │ p1
 pk    │ p1_a_fkey1 │ p1
 pk    │ p1_a_fkey1 │ p1
 pk    │ p1_a_fkey2 │ p1
 pk    │ p1_a_fkey2 │ p1
 pk    │ p11_a_fkey │ p11
 pk    │ p11_a_fkey │ p11
(10 rows)


alter table p1 attach partition p11 for values in (1);

-- p1_a_fkey and p11_a_fkey merged, so triggers for the latter dropped

select tgrelid::regclass as pkrel, conname as fkconname,
tgconstrrelid::regclass as fkrel from pg_trigger t, pg_constraint c where
tgrelid = 'pk'::regclass and tgconstraint = c.oid;
 pkrel │ fkconname  │ fkrel
───────┼────────────┼───────
 pk    │ p_a_fkey   │ p
 pk    │ p_a_fkey   │ p
 pk    │ p1_a_fkey  │ p1
 pk    │ p1_a_fkey  │ p1
 pk    │ p1_a_fkey1 │ p1
 pk    │ p1_a_fkey1 │ p1
 pk    │ p1_a_fkey2 │ p1
 pk    │ p1_a_fkey2 │ p1
(8 rows)

-- p_a_fkey and p1_a_fkey merged, so triggers for the latter dropped

alter table p attach partition p1 for values in (1);

select tgrelid::regclass as pkrel, conname as fkconname,
tgconstrrelid::regclass as fkrel from pg_trigger t, pg_constraint c where
tgrelid = 'pk'::regclass and tgconstraint = c.oid;
 pkrel │ fkconname  │ fkrel
───────┼────────────┼───────
 pk    │ p_a_fkey   │ p
 pk    │ p_a_fkey   │ p
 pk    │ p1_a_fkey1 │ p1
 pk    │ p1_a_fkey1 │ p1
 pk    │ p1_a_fkey2 │ p1
 pk    │ p1_a_fkey2 │ p1
(6 rows)


alter table p detach partition p1;

-- p1_a_fkey needs its own triggers again

select tgrelid::regclass as pkrel, conname as fkconname,
tgconstrrelid::regclass as fkrel from pg_trigger t, pg_constraint c where
tgrelid = 'pk'::regclass and tgconstraint = c.oid;
 pkrel │ fkconname  │ fkrel
───────┼────────────┼───────
 pk    │ p_a_fkey   │ p
 pk    │ p_a_fkey   │ p
 pk    │ p1_a_fkey1 │ p1
 pk    │ p1_a_fkey1 │ p1
 pk    │ p1_a_fkey2 │ p1
 pk    │ p1_a_fkey2 │ p1
 pk    │ p1_a_fkey  │ p1
 pk    │ p1_a_fkey  │ p1
(8 rows)

alter table p1 detach partition p11;

-- p11_a_fkey needs its own triggers again

select tgrelid::regclass as pkrel, conname as fkconname,
tgconstrrelid::regclass as fkrel from pg_trigger t, pg_constraint c where
tgrelid = 'pk'::regclass and tgconstraint = c.oid;
 pkrel │ fkconname  │ fkrel
───────┼────────────┼───────
 pk    │ p_a_fkey   │ p
 pk    │ p_a_fkey   │ p
 pk    │ p1_a_fkey1 │ p1
 pk    │ p1_a_fkey1 │ p1
 pk    │ p1_a_fkey2 │ p1
 pk    │ p1_a_fkey2 │ p1
 pk    │ p1_a_fkey  │ p1
 pk    │ p1_a_fkey  │ p1
 pk    │ p11_a_fkey │ p11
 pk    │ p11_a_fkey │ p11
 pk    │ p1_a_fkey1 │ p11
 pk    │ p1_a_fkey1 │ p11
 pk    │ p1_a_fkey2 │ p11
 pk    │ p1_a_fkey2 │ p11
(14 rows)

-- try again

alter table p1 attach partition p11 for values in (1);

select tgrelid::regclass as pkrel, conname as fkconname,
tgconstrrelid::regclass as fkrel from pg_trigger t, pg_constraint c where
tgrelid = 'pk'::regclass and tgconstraint = c.oid;
 pkrel │ fkconname  │ fkrel
───────┼────────────┼───────
 pk    │ p_a_fkey   │ p
 pk    │ p_a_fkey   │ p
 pk    │ p1_a_fkey1 │ p1
 pk    │ p1_a_fkey1 │ p1
 pk    │ p1_a_fkey2 │ p1
 pk    │ p1_a_fkey2 │ p1
 pk    │ p1_a_fkey  │ p1
 pk    │ p1_a_fkey  │ p1
(8 rows)


alter table p attach partition p1 for values in (1);

select tgrelid::regclass as pkrel, conname as fkconname,
tgconstrrelid::regclass as fkrel from pg_trigger t, pg_constraint c where
tgrelid = 'pk'::regclass and tgconstraint = c.oid;
 pkrel │ fkconname  │ fkrel
───────┼────────────┼───────
 pk    │ p_a_fkey   │ p
 pk    │ p_a_fkey   │ p
 pk    │ p1_a_fkey1 │ p1
 pk    │ p1_a_fkey1 │ p1
 pk    │ p1_a_fkey2 │ p1
 pk    │ p1_a_fkey2 │ p1
(6 rows)


By the way, I also noticed that there's duplicated code in
clone_fk_constraints() which 0001 gets rid of:

        datum = fastgetattr(tuple, Anum_pg_constraint_conpfeqop,
                            tupdesc, &isnull);
        if (isnull)
            elog(ERROR, "null conpfeqop");
        arr = DatumGetArrayTypeP(datum);
        nelem = ARR_DIMS(arr)[0];
        if (ARR_NDIM(arr) != 1 ||
            nelem < 1 ||
            nelem > INDEX_MAX_KEYS ||
            ARR_HASNULL(arr) ||
            ARR_ELEMTYPE(arr) != OIDOID)
            elog(ERROR, "conpfeqop is not a 1-D OID array");
        memcpy(conpfeqop, ARR_DATA_PTR(arr), nelem * sizeof(Oid));

-        datum = fastgetattr(tuple, Anum_pg_constraint_conpfeqop,
-                            tupdesc, &isnull);
-        if (isnull)
-            elog(ERROR, "null conpfeqop");
-        arr = DatumGetArrayTypeP(datum);
-        nelem = ARR_DIMS(arr)[0];
-        if (ARR_NDIM(arr) != 1 ||
-            nelem < 1 ||
-            nelem > INDEX_MAX_KEYS ||
-            ARR_HASNULL(arr) ||
-            ARR_ELEMTYPE(arr) != OIDOID)
-            elog(ERROR, "conpfeqop is not a 1-D OID array");
-        memcpy(conpfeqop, ARR_DATA_PTR(arr), nelem * sizeof(Oid));
-

I know you're working on a bug fix in the thread on pgsql-bugs which is
related to the patch 0002 here, but attaching it here anyway, because it
proposes to get rid of the needless dependencies which I didn't see
mentioned on the other thread.  Also, updated 0001 needed it to be rebased.

Like the last time, I've also attached the patches that can be applied
PG11 branch.

Thanks,
Amit