Kevin Grittner <kgrittn@ymail.com> wrote:
> Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> wrote:
>> 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. */
>> 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.
After thinking it over, I think that the "BTScanPos" part of the
macro name is enough of a hint that it is concerned with the
actions of this scan; it is the comment that needs the change.
I went with:
/*
* We have held the pin on this page since we read the index tuples,
* so all we need to do is lock it. The pin will have prevented
* re-use of any TID on the page, so there is no need to check the
* LSN.
*/
>> + 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.
That sentence was full of "passive voice", which didn't help any.
I changed it to:
| So that we can handle the cases where we attempt LP_DEAD flagging
| for a page after we have released its pin, we remember the LSN of
| the index page when we read the index tuples from it; we do not
| attempt to flag index tuples as dead if the we didn't hold the
| pin the entire time and the LSN has changed.
Do these changes seem clear?
Because these changes are just to a comment and a README file, I'm
posting a patch-on-patch v3a (to be applied on top of v3).
--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company