Hello,
At Thu, 12 Mar 2015 15:27:37 -0700, Peter Geoghegan <pg@heroku.com> wrote in
<CAM3SWZQ5nwTB-y4ZOj=5ckMLce5GAEUnjKJ2=M1vMHfX_aYmCg@mail.gmail.com>
> On Sat, Feb 14, 2015 at 4:19 PM, Kevin Grittner <kgrittn@ymail.com> wrote:
> > At some point we could consider building on this patch to recheck
> > index conditions for heap access when a non-MVCC snapshot is used,
> > check the visibility map for referenced heap pages when the TIDs
> > are read for an index-only scan, and/or skip LP_DEAD hinting for
> > non-WAL-logged indexes. But all those are speculative future work;
> > this is a conservative implementation that just didn't modify
> > pinning where there were any confounding factors.
>
> I don't have the bandwidth for a full review, but I took a quick look at this.
>
> I think you should call out those "confounding factors" in the README.
> It's not hard to piece them together from
> _bt_drop_lock_and_maybe_pin(), but I think you should anyway.
>
> Don't use BUFFER_LOCK_SHARE -- use BT_READ, as the existing nbtree
> LockBuffer() callers do. You're inconsistent about that in V3.
I agree with you. It looks the only point where it is used.
Addition to that, the commnet just above the point methioned
above quizes me.
> /* XXX: Can walking left be lighter on the locking and pins? */
> if (BTScanPosIsPinned(so->currPos))
> LockBuffer(so->currPos.buf, BUFFER_LOCK_SHARE);
> else
> so->currPos.buf = _bt_getbuf(rel, so->currPos.currPage, BT_READ);
I'm happy if I could read the meaming of the comment more
clearly. I understand that it says that you want to remove the
locking (and pinning), but can't now because the simultaneous
splitting of the left page would break something. I'd like to see
it clearer even for me either I am correct or not..
regards,
--
Kyotaro Horiguchi
NTT Open Source Software Center