On Sat, Aug 3, 2019 at 11:56 AM Ashwin Agrawal <aagra...@pivotal.io> wrote: > On Wed, Jul 31, 2019 at 2:06 PM Andres Freund <and...@anarazel.de> wrote: >> I'm also a bit confused why we don't need to pass in the offset of the >> current tuple, rather than the HOT root tuple here. That's not related >> to this patch. But aren't we locking the wrong tuple here, in case of >> HOT? > > Yes, root is being locked here instead of the HOT. But I don't have full > context on the same. If we wish to fix it though, can be easily done now with > the patch by passing "tid" instead of &(heapTuple->t_self).
Here are three relevant commits: 1. Commit dafaa3efb75 "Implement genuine serializable isolation level." (2011) locked the root tuple, because it set t_self to *tid. Possibly due to confusion about the effect of the preceding line ItemPointerSetOffsetNumber(tid, offnum). 2. Commit commit 81fbbfe3352 "Fix bugs in SSI tuple locking." (2013) fixed that by adding ItemPointerSetOffsetNumber(&heapTuple->t_self, offnum). 3. Commit b89e151054a "Introduce logical decoding." (2014) also did ItemPointerSet(&(heapTuple->t_self), BufferGetBlockNumber(buffer), offnum), for the benefit of historical MVCC snapshots (unnecessarily, considering the change in the commit #2), but then, intending to "reset to original, non-redirected, tid", clobbered it, reintroducing the bug fixed by #2. My first guess is that commit #3 above was developed before commit #2, and finished up clobbering it. In fact, both logical decoding and SSI want offnum, so we should be able to just remove the "reset" bit (perhaps like in the attached sketch, not really tested, though it passes). This must be in want of an isolation test, but I haven't yet tried to get my head around how to write a test that would show the difference. -- Thomas Munro https://enterprisedb.com
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c index 94309949fa..107605d276 100644 --- a/src/backend/access/heap/heapam.c +++ b/src/backend/access/heap/heapam.c @@ -1576,7 +1576,6 @@ heap_hot_search_buffer(ItemPointer tid, Relation relation, Buffer buffer, heapTuple->t_data = (HeapTupleHeader) PageGetItem(dp, lp); heapTuple->t_len = ItemIdGetLength(lp); heapTuple->t_tableOid = RelationGetRelid(relation); - ItemPointerSetOffsetNumber(&heapTuple->t_self, offnum); /* * Shouldn't see a HEAP_ONLY tuple at chain start. @@ -1608,15 +1607,14 @@ heap_hot_search_buffer(ItemPointer tid, Relation relation, Buffer buffer, * of the root tuple of the HOT chain. This is important because * the *Satisfies routine for historical mvcc snapshots needs the * correct tid to decide about the visibility in some cases. + * SSI also needs offnum. */ - ItemPointerSet(&(heapTuple->t_self), BufferGetBlockNumber(buffer), offnum); + ItemPointerSetOffsetNumber(&heapTuple->t_self, offnum); /* If it's visible per the snapshot, we must return it */ valid = HeapTupleSatisfiesVisibility(heapTuple, snapshot, buffer); CheckForSerializableConflictOut(valid, relation, heapTuple, buffer, snapshot); - /* reset to original, non-redirected, tid */ - heapTuple->t_self = *tid; if (valid) {