Re: Reduce pinning in btree indexes

Thank you for rewriting.

I was satisfied with the current patch (Head of btnopin on your
github repos). I have no further comment on the functions. Peter
might have a comment on the README description but I suppose I
can push this to the committers.

I attached the your latest patch to this mail as
bt-nopin-v4.patch for now. Please check that there's no problem
in it.

- By this patch, index scan becomes to release buffer pins while
fetching index tuples in a page, so it should reduce the chance
of index scans with long duration to block vacuum, because
vacuums now can easily overtake the current position of an
index scan. I didn't actually measured how effective it is,
though.

- It looks to work correctly for scan in both direction with
simultaneous page splitting and vacuum.

- It makes no performance deterioration, on the contrary it
accelerates index scans. It seems to be because of removal of
lock and unlock surrounding _bt_steppage in bt_next.

- It applies cleanly on the current head on the master branch.

- It has enough intelligible comments and README descriptions.

- An inrelevant typo fix is made in buffers/README by this
patch. But it doesn't seem necessary to be separated.

regards,

At Fri, 13 Mar 2015 15:08:25 +0000 (UTC), Kevin Grittner <kgrittn(at)ymail(dot)com> wrote in <848652045(dot)4044965(dot)1426259305233(dot)JavaMail(dot)yahoo(at)mail(dot)yahoo(dot)com>
> Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> > At Thu, 12 Mar 2015 15:27:37 -0700, Peter Geoghegan <pg(at)heroku(dot)com> wrote:
> >> On Sat, Feb 14, 2015 at 4:19 PM, Kevin Grittner <kgrittn(at)ymail(dot)com> wrote:

> >> 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.
>
> OK:
>
> https://github.com/kgrittn/postgres/commit/f5f59ded30b114ac83b90a00ba1fa5ef490b994e

It looks perfect for me. Do you have any further request on this,
Peter?

> >> 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.
>
> OK:
>
> https://github.com/kgrittn/postgres/commit/76118b58be819ed5e68569c926d0222bc41640ea
>
> > 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..
>
> Does this clear it up?:
>
> https://github.com/kgrittn/postgres/commit/22066fc161a092e800e4c1e853136c4513f8771b

Thank you for the labor. It also looks perfect.

> Since there are no changes that would affect the compiled code
> here, I'm not posting a new patch yet. I'll do that once things
> seem to have settled down a bit more.

--
Kyotaro Horiguchi
NTT Open Source Software Center