Re: Reduce pinning in btree indexes - Mailing list pgsql-hackers

Hello,

I found no other problem including the performance issue in the
patch attached to the last mail as far as I can understand. So
I'll mark this as ready for commit after a few days with no
objection after this comments is addressed.


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

Sorry, it is largely because of my poor composition.

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

Yes. I think so.

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

I thought of the case that the buffer has been pinned by another
backend after this backend unpinned it. I looked again the
definition of BTScanPosIsPinned and BTScanPosUnpinIfPinned, and
understood that the pin should be mine if BTScanPosIsPinned.

Would you mind rewriting the comment there like this?

-  /* The buffer is still pinned, but not locked.  Lock it now. */
+  /* I still hold the pin on the buffer, but not locked.  Lock it now. */
|   LockBuffer(so->currPos.buf, BT_READ);


Or would you mind renaming the macro as "BTScanPosIsPinnedByMe"
or something like, or anyway to notify the reader that the pin
should be mine there?


> > - _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?

Sorry, I read btrescan incorrectly that it calls _bt_killitems()
*after* releaseing the buffer. Please forget it.


Finally, I'd like to timidly comment on this..

+ To handle the cases where it is safe to release the pin after
+ reading the index page, the LSN of the index page is read along
+ with the index entries before the shared lock is released, and we
+ do not attempt to flag index tuples as dead if the page is not
+ pinned and the LSN does not match.


I suppose that the sentence like following is more directly
describing about the mechanism and easier to find the
correnponsing code, if it is correct.

>  To handle the cases where a index page has unpinned when
>  trying to mark the unused index tuples on the page as dead,
>  the LSN of the index page is remembered when reading the index
>  page for index tuples, and we do not attempt to flag index
>  tuples as dead if the page is not pinned and the LSN does not
>  match.


regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center