Re: Reduce pinning in btree indexes

Hello,

> It looks like the remaining performance regression was indeed a
> result of code alignment.  I found two "paranoia" assignments I had
> accidentally failed to put back with the rest of the mark/restore
> optimization; after that trivial change the patched version is
> ever-so-slightly faster than master on all tests.

The performance which your test shows looks great. The gain might
comes from removing of buffer locking on _bt_next. I also would
like to see this to come along with 9.5 but it is the matter for
committers.

Apart from it, I looked into the patch itself more
closely. Please reconcile as you like among contradicting
comments below:)


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.
 >  * _bt_killitems - set LP_DEAD state for items an indexscan caller has >  * told us were killed >  * >  * scan->so
containsinformation about the current page and killed tuples >  * thereon (generally, this should only be called if
so->numKilled> 0).
 
 I suppose "scan->so" should be "scan->opaque" and "so->numKilled" be "opaque->numKilled".


- 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
notnecessary.
 

- 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-elseblock is need also in the if-then block. It will be accomplished only by changing the position of the end of the
if-elseblock.
 

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


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
backendsto insert items into the page simultaneously. It seems to be dangerous alone, but _bt_killitems already
considersthe case. Anyway I think It'll be better to add a comment mentioning unlocking is not dangerous.
 


... Sorry, time's up for now.

regards,


> Average of 3 `make check-world` runs:
> master: 336.288 seconds
> patch:  332.237 seconds
> 
> Average of 6 `make check` runs:
> master: 25.409 seconds
> patch:  25.270 seconds
> 
> The following were all run 12 times, the worst 2 dropped, the rest
> averaged:
> 
> Kyotaro-san's 1m mark "worst case" benchmark:
> master: 962.581 ms, 6.765 stdev
> patch:  947.518 ms, 3.584 stdev
> 
> Kyotaro-san's 500k mark, 500k restore "worst case" benchmark:
> master: 1366.639 ms, 5.314 stdev
> patch:  1363.844 ms, 5.896 stdev
> 
> pgbench -i -s 16 pgbench / pgbench -c 16 -j 4 -T 500 pgbench
> master: 265.011 tps, 4.952 stdev
> patch:  267.164 tps, 9.094 stdev
> 
> How do people feel about the idea of me pushing this for 9.5 (after
> I clean up all the affected comments and README files)?  I know
> this first appeared in the last CF, but the footprint is fairly
> small and the only user-visible behavior change is that a btree
> index scan of a WAL-logged table using an MVCC snapshot no longer
> blocks a vacuum indefinitely.  (If there are objections I will move
> this to the first CF for the next release.)
> 
>  src/backend/access/nbtree/nbtree.c    |  50 +++++-------
>  src/backend/access/nbtree/nbtsearch.c | 141 +++++++++++++++++++++++++---------
>  src/backend/access/nbtree/nbtutils.c  |  58 ++++++++++----
>  src/include/access/nbtree.h           |  36 ++++++++-
>  4 files changed, 201 insertions(+), 84 deletions(-)

-- 
Kyotaro Horiguchi
NTT Open Source Software Center