Re: partitioned tables referenced by FKs

On 2019-Feb-28, Amit Langote wrote:

Hello

> In the following case:
>
> create table pk (a int primary key) partition by list (a);
> create table pk1 (a int primary key);
> create table fk (a int references pk1);
>
> -- adds FK referencing pk1 via ATAddForeignKeyConstraint recursion
> alter table pk attach partition pk1 for values in (1);
> alter table fk add foreign key (a) references pk;
>
> or:
>
> -- adds FK referencing pk1 via CloneForeignKeyConstraints
> alter table fk add foreign key (a) references pk;
> alter table pk attach partition pk1 for values in (1);
>
> \d fk
> Table "public.fk"
> Column │ Type │ Collation │ Nullable │ Default
> ────────┼─────────┼───────────┼──────────┼─────────
> a │ integer │ │ │
> Foreign-key constraints:
> "fk_a_fkey" FOREIGN KEY (a) REFERENCES pk1(a)
> "fk_a_fkey1" FOREIGN KEY (a) REFERENCES pk(a)
> "fk_a_fkey2" FOREIGN KEY (a) REFERENCES pk1(a)
>
> Could we have avoided creating fk_a_fkey2 which must be same as fk_a_fkey
> as far as the user-visible behavior is concerned?

So I wrote the code that does as you describe, and it worked
beautifully.

Once I was finished, fixed bugs and tested it, I realized that that was
a stupid thing to have done -- because THOSE ARE DIFFERENT CONSTRAINTS.
When you say "fk (a) references pk1" you're saying that all the values
in fk(a) must appear in pk1. OTOH when you say "fk references pk" you
mean that the values might appear anywhere in pk, not just pk1. Have a
look at the triggers in pg_trigger that appear when you do "references
pk1" vs. when you do "references pk". The constraint itself might
appear identical, but the former adds check triggers that are not
present for the latter.

The main problem here is the way the constraints are presented to the
user in psql (fk_a_fkey2 ought not to appear at all, because it's an
implementation detail for fk_a_fkey), which as you know I have another
patch for.

> * Regarding 0003
>
> 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. Following functions have been introduced
> in the foreign key DDL code in the course of adding support for partitioning:
>
> addFkRecurseReferenced
> addFkRecurseReferencing
> CloneForeignKeyConstraints
> CloneFkReferencing
> CloneFkReferenced
> tryAttachPartitionForeignKey

I can understand how the lack of consistency in how we handle recursion
can be confusing. I'll see if I can move the recursion so that it's
handled similarly for both cases, but I'm not convinced that it can.

Note there's a single call of tryAttachPartitionForeignKey(). I
invented that function because in the original code pattern there were
two callers, but after some backpatched code restructuring to fix a bug,
I changed that and now that function is no longer necessary. I might
put the code back into CloneFkReferencing.

> The latter invokes tryAttachPartitionForeignKey to see if an existing
> foreign key constraint is functionally same as one being cloned to avoid
> creating a duplicate constraint.

Do note that "functionally the same" does not mean the same thing when
applied to the referencing side than when applied to the referenced
side.

> However, we have CloneFkReferenced() calling addFkRecurseReferenced, not
> the other way around. Neither of these functions attempts to reuse an
> existing, functionally equivalent, foreign key constraint referencing a
> given partition that's being recursed to.

You'll have to take back the assertion that those constraints are
funtionally equivalent, because they are not.

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

I'm looking into this code restructuring you suggest. I'm not 100% sold
on it yet ... it took me quite a while to arrive to a working
arrangement, although I admit the backpatched bugfixes dislodged things.

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