Re: partitioned tables referenced by FKs

On 2019-Feb-28, Amit Langote wrote:

> I'd like to hear your thoughts on some suggestions to alter the structure
> of the reorganized code around foreign key addition/cloning. With this
> patch adding support for foreign keys to reference partitioned tables, the
> code now has to consider various cases due to the possibility of having
> partitioned tables on both sides of a foreign key, which is reflected in
> the complexity of the new code.

I spent a few hours studying this and my conclusion is the opposite of
yours: we should make addFkRecurseReferencing the recursive one, and
CloneFkReferencing a non-recursive caller of that. So we end up with
both addFkRecurseReferenced and addFkRecurseReferencing as recursive
routines, and CloneFkReferenced and CloneFkReferencing being
non-recursive callers of those. With this structure change there is one
more call to CreateConstraintEntry than before, and now there are two
calls of tryAttachPartitionForeignKey instead of one; I think with this
new structure things are much simpler. I also changed
CloneForeignKeyConstraints's API: instead of returning a list of cloned
constraint giving its caller the responsibility of adding FK checks to
phase 3, we now give CloneForeignKeyConstraints the 'wqueue' list, so
that it can add the FK checks itself. It seems much cleaner this way.

> Also, it seems a bit confusing that there is a CreateConstraintEntry call
> in addFkRecurseReferenced() which is creating a constraint on the
> *referencing* relation, which I think should be in
> ATAddForeignKeyConstraint, the caller. I know we need to create a copy of
> the constraint to reference the partitions of the referenced table, but we
> might as well put it in CloneFkReferenced and reverse who calls who --
> make addFkRecurseReferenced() call CloneFkReferenced and have the code to
> create the cloned constraint and action triggers in the latter. That will
> make the code to handle different sides of foreign key look similar, and
> imho, easier to follow.

Well, if you think about it, *all* the constraints created by all these
routines are in the referencing relations. The only question here is
*why* we create those tuples; in the case of the ...Referenced routines,
it's because of the partitions in the referenced side; in the case of
the ...Referencing routines, it's because of partitions in the
referencing side. I think changing it the way you suggest would be even
more confusing.

As discussed in the other subthread, I'm not making any effort to reuse
an existing constraint defined in a partition of the referenced side; as
far as I can tell that's a nonsensical transformation.

A pretty silly bug remains here. Watch:

create table pk (a int primary key) partition by list (a);
create table pk1 partition of pk for values in (1);
create table fk (a int references pk);
insert into pk values (1);
insert into fk values (1);
drop table pk cascade;

Note that the DROP of the main PK table is pretty aggressive: since it
cascades, you want it to drop the constraint on the FK side. This would
work without a problem if 'pk' was a non-partitioned table, but in this
case it fails:

alvherre=# drop table pk cascade;
NOTICE: drop cascades to constraint fk_a_fkey on table fk
ERROR: removing partition "pk1" violates foreign key constraint "fk_a_fkey1"
DETALLE: Key (a)=(1) still referenced from table "fk".

The reason is that the "pre drop check" that's done before allow a drop
of the partition doesn't realize that the constraint is also being
dropped (and so the check ought to be skipped). If you were to do just
"DROP TABLE pk1" then this complaint would be correct, since it would
leave the constraint in an invalid state. But here, it's bogus and
annoying. You can DELETE the matching values from table FK and then the
DROP works. Here's another problem caused by the same misbehavior:

alvherre=# drop table pk, fk;
ERROR: removing partition "pk1" violates foreign key constraint "fk_a_fkey1"
DETALLE: Key (a)=(1) still referenced from table "fk".

Note here we want to get rid of table 'fk' completely. If you split it
up in a DROP of fk, followed by a DROP of pk, it works.

I'm not yet sure what's the best way to attack this. Maybe the
"pre-drop check" needs a completely different approach. The simplest
approach is to prohibit a table drop or detach for any partitioned table
that's referenced by a foreign key, but that seems obnoxious and
inconvenient.

I still haven't put back the code in "#if 0".

FWIW I think we should add an index on pg_constraint.confrelid now.

--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services