Re: simplifying foreign key/RI checks

On Fri, Nov 12, 2021 at 8:19 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Amit Langote <amitlangote09(at)gmail(dot)com> writes:
> > Rebased patches attached.
>
> I've spent some more time digging into the snapshot-management angle.

Thanks for looking at this.

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

Okay, I'll look into getting 1 and 2 done for this patch and I guess
work with Alvaro on 3.

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

Hm yeah, fair point about the undesirability of putting partitioning
details into ri_triggers.c, so will look into refactoring to avoid
that.

--
Amit Langote
EDB: http://www.enterprisedb.com