Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> wrote:
> 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.
Thanks for the reviews!
>> 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?
I see your point, although those first person singular pronouns
used like that make me a little uncomfortable; I'll change the
comment and/or macro name, but I'll work on the name some more.
> 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.
Will reword that part to try to make it clearer.
Thanks!
--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company