Re: simplifying foreign key/RI checks

On Mon, Mar 8, 2021 at 11:41 PM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
> On Thu, Mar 4, 2021 at 5:15 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > Lastly, ri_PerformCheck is pretty careful about not only which
> > snapshot it uses, but which *pair* of snapshots it uses, because
> > sometimes it needs to worry about data changes since the start
> > of the transaction. You've ignored all of that complexity AFAICS.
> > That's okay (I think) for RI_FKey_check which was passing
> > detectNewRows = false, but for sure it's not okay for
> > ri_Check_Pk_Match. (I kind of thought we had isolation tests
> > that would catch that, but apparently not.)
>
> Okay, let me closely check the ri_Check_Pk_Match() case and see if
> there's any live bug.

I checked, and AFAICS, the query invoked by ri_Check_Pk_Match() (that
is, without the patch) does not use the "crosscheck" snapshot at any
point during its execution. That snapshot is only used in the
table_update() and table_delete() routines, which are not involved in
the execution of ri_Check_Pk_Match()'s query.

I dug through git history and -hackers archives to understand the
origins of RI code's use of a crosscheck snapshot and came across this
discussion:

https://www.postgresql.org/message-id/20031001150510.U45145%40megazone.bigpanda.com

If I am reading the discussion and the details in subsequent commit
55d85f42a891a correctly, the crosscheck snapshot is only to be used to
ensure, under serializable isolation, that any attempts by the RI
query of updating/deleting rows that are not visible to the
transaction snapshot cause a serialization error. Use of the same
facilities in ri_Check_Pk_Match() was merely done as future-proofing,
with no particular use case to address, then and perhaps even now.

If that is indeed the case, it does not seem particularly incorrect
for ri_ReferencedKeyExists() added by the patch to not bother with
setting up a crosscheck snapshot, even when called from
ri_Check_Pk_Match(). Am I missing something?

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