Re: simplifying foreign key/RI checks

Amit Langote <amitlangote09(at)gmail(dot)com> writes:
> Rebased patches attached.

I've spent some more time digging into the snapshot-management angle.
I think you are right that the crosscheck_snapshot isn't really an
issue because the executor pays no attention to it for SELECT, but
that doesn't mean that there's no problem, because the test_snapshot
behavior is different too. By my reading of it, the intention of the
existing code is to insist that when IsolationUsesXactSnapshot()
is true and we *weren't* saying detectNewRows, the query should be
restricted to only see rows visible to the transaction snapshot.
Which I think is proper: an RR transaction shouldn't be allowed to
insert referencing rows that depend on a referenced row it can't see.
On the other hand, it's reasonable for ri_Check_Pk_Match to use
detectNewRows=true, because in that case what we're doing is allowing
an RR transaction to depend on the continued existence of a PK value
that was deleted and replaced since the start of its transaction.

It appears to me that commit 71f4c8c6f (DETACH PARTITION CONCURRENTLY)
broke the semantics here, because now things work differently with a
partitioned PK table than with a plain table, thanks to not bothering
to distinguish questions of how to handle partition detachment from
questions of visibility of individual data tuples. We evidently
haven't got test coverage for this :-(, which is perhaps not so
surprising because all this behavior long predates the isolationtester
infrastructure that would've allowed us to test it mechanically.

Anyway, I think that (1) we should write some more test cases around
this behavior, (2) you need to establish the snapshot to use in two
different ways for the RI_FKey_check and ri_Check_Pk_Match cases,
and (3) something's going to have to be done to repair the behavior
in v14 (unless we want to back-patch this into v14, which seems a
bit scary).

It looks like you've addressed the other complaints I raised back in
March, so that's forward progress anyway. I do still find myself a
bit dissatisfied with the code factorization, because it seems like
find_leaf_pk_rel() doesn't belong here but rather in some partitioning
module. OTOH, if that means exposing RI_ConstraintInfo to the world,
that wouldn't be nice either.

regards, tom lane