On Fri, Nov 12, 2021 at 8:19 AM Tom Lane <t...@sss.pgh.pa.us> wrote: > Amit Langote <amitlangot...@gmail.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
