Re: simplifying foreign key/RI checks

On Sat, Nov 13, 2021 at 5:43 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Amit Langote <amitlangote09(at)gmail(dot)com> writes:
> > On Fri, Nov 12, 2021 at 8:19 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> >> 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.
>
> Actually, it seems that DETACH PARTITION is broken for concurrent
> serializable/repeatable-read transactions quite independently of
> whether they attempt to make any FK checks [1]. If we do what
> I speculated about there, namely wait out all such xacts before
> detaching, it might be possible to fix (3) just by reverting the
> problematic change in ri_triggers.c. I'm thinking the wait would
> render it unnecessary to get FK checks to do anything weird about
> partition lookup. But I might well be missing something.

I wasn't able to make much inroads into how we might be able to get
rid of the DETACH-related partition descriptor hacks, the item (3),
though I made some progress on items (1) and (2).

For (1), the attached 0001 patch adds a new isolation suite
fk-snapshot.spec to exercise snapshot behaviors in the cases where we
no longer go through SPI. It helped find some problems with the
snapshot handling in the earlier versions of the patch, mainly with
partitioned PK tables. It also contains a test along the lines of the
example you showed upthread, which shows that the partition descriptor
hack requiring ActiveSnapshot to be set results in wrong results.
Patch includes the buggy output for that test case and marked as such
in a comment above the test.

In updated 0002, I fixed things such that the snapshot-setting
required by the partition descriptor hack is independent of
snapshot-setting of the RI query such that it no longer causes the PK
index scan to return rows that the RI query mustn't see. That fixes
the visibility bug illustrated in your example, and as mentioned, also
exercised in the new test suite.

I also moved find_leaf_pk_rel() into execPartition.c with a new name
and a new set of parameters.

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