Hi,
On 2019-08-05 20:58:05 +1200, Thomas Munro wrote:
> On Sat, Aug 3, 2019 at 11:56 AM Ashwin Agrawal <aagrawal@pivotal.io> wrote:
> > On Wed, Jul 31, 2019 at 2:06 PM Andres Freund <andres@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:
Thanks for digging!
> 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).
Hm. It's not at all sure that it's ok to report the non-root tuple tid
here. I.e. I'm fairly sure there was a reason to not just set it to the
actual tid. I think I might have written that up on the list at some
point. Let me dig in memory and list. Obviously possible that that was
also obsoleted by parallel changes.
> 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.
Yea, that sounds likely.
> 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.
Indeed.
Greetings,
Andres Freund