Kyotaro HORIGUCHI <horiguchi.kyot...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers