From: | Andres Freund <andres(at)anarazel(dot)de> |
---|---|
To: | Thomas Munro <thomas(dot)munro(at)gmail(dot)com> |
Cc: | Ashwin Agrawal <aagrawal(at)pivotal(dot)io>, PostgreSQL mailing lists <pgsql-hackers(at)postgresql(dot)org>, Kuntal Ghosh <kuntalghosh(dot)2007(at)gmail(dot)com> |
Subject: | Re: Remove HeapTuple and Buffer dependency for predicate locking functions |
Date: | 2019-08-05 16:35:17 |
Message-ID: | 20190805163517.qk54alierkihsuzo@alap3.anarazel.de |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Hi,
On 2019-08-05 20:58:05 +1200, Thomas Munro wrote:
> On Sat, Aug 3, 2019 at 11:56 AM Ashwin Agrawal <aagrawal(at)pivotal(dot)io> wrote:
> > On Wed, Jul 31, 2019 at 2:06 PM Andres Freund <andres(at)anarazel(dot)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
From | Date | Subject | |
---|---|---|---|
Next Message | Andres Freund | 2019-08-05 16:42:16 | Re: POC: Cleaning up orphaned files using undo logs |
Previous Message | Robert Haas | 2019-08-05 16:31:00 | Re: [PATCH] Stop ALTER SYSTEM from making bad assumptions |