Re: Reduce pinning in btree indexes

Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> wrote:

> The performance which your test shows looks great. The gain might
> comes from removing of buffer locking on _bt_next.

Yeah, I had been hoping that removing some buffer pins and locks
from the common path of scanning forward from one page to the next
might have some direct performance benefit; it's possible that this
will show up more dramatically at high concurrency levels.  The
main goal, of course, was to improve performance more indirectly,
by not having autovacuum worker processes blocked for extended
periods in some situations where that can now happen.

> In nbtutils.c, _bt_killitems:
>
> - This is not in charge of this patch, but the comment for
> _bt_killitems looks to have a trivial typo.

As you say, that problem was already there, but no reason not to
fix it in the patch as long as we're here.  Reworded.

> - The following comment looks somewhat wrong.
>
> > /* Since the else condition needs page, get it here, too. */
>
> "the else condition needs page" means "the page of the buffer
> is needed later"? But, I suppose the comment itself is not
> necessary.

I guess that comment isn't really worth the space it takes up; what
it says is pretty obvious from the code.  Removed.

> - If (BTScanPosIsPinned(so->currPos)).
>
> As I mention below for nbtsearch.c, the page aquired in the
> if-then block may be vacuumed so the LSN check done in the
> if-else block is need also in the if-then block. It will be
> accomplished only by changing the position of the end of the
> if-else block.

I'm not sure I agree on this.  If the page is pinned it should have
been pinned continuously since we initially read it, so the line
pointers we have could not have been removed by any vacuum process.
The tuples may have been pruned away in the heap, but that doesn't
matter.  Index entries may have been added and the index page may
have been split, but that was true before this patch and
_bt_killitems will deal with those things the same as it always
has.  I don't see any benefit to doing the LSN compare in this
case; if we've paid the costs of holding the pin through to this
point, we might as well flag any dead entries we can.

> - _bt_killitems is called without pin when rescanning from
> before, so I think the previous code has already considered the
> unpinned case ("if (ItemPointerEquals(&ituple..." does
> that). Vacuum rearranges line pointers after deleting LP_DEAD
> items so the previous check seems enough for the purpose. The
> previous code is more effeicient becuase the mathing items will
> be processed even after vacuum.

I'm not following you on this one; could you rephrase it?

> In nbtsearch.c
>
> - _bt_first(): It now releases the share lock on the page before
> the items to be killed is processed. This allows other backends
> to insert items into the page simultaneously. It seems to be
> dangerous alone, but _bt_killitems already considers the
> case. Anyway I think It'll be better to add a comment
> mentioning unlocking is not dangerous.

Well, _bt_killitems does acquire a shared (read) lock to flag index
tuples as LP_DEAD, and it has always been OK for the index page to
accept inserts and allow page splits before calling _bt_killitems.
I don't really see anything new with the patch in this regard.  I
do agree, though, that a lot of comments and at least two README
files refer to the pins blocking vacuum, and these must be found
and changed.

It doesn't seem worth posting to the list for the small changes
since the last version; I'll wait until I update the comments and
README files.  If you want review or test the latest, you can peek
at: https://github.com/kgrittn/postgres/tree/btnopin

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company