Re: heap_hot_search_buffer refactoring

On Sat, Jun 25, 2011 at 6:24 AM, Jeff Davis <pgsql@j-davis.com> wrote:
> On Fri, 2011-06-24 at 15:32 -0400, Robert Haas wrote:
>> On Sun, Jun 19, 2011 at 2:16 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>> > New patch attached, with that one-line change.
>>
>> Jeff, are you planning to review this further?  Do you think it's OK to commit?
>
> 1. Patch does not apply to master cleanly, and it's in unified format
> (so I can't compare it against the old patch very easily). This review
> is for the first patch, disregarding the "skip = !first_call" issue that
> you already fixed. If you had other changes in the latest version,
> please repost the patch.

That is strange, because it applies for me.  But I had no other changes.

> 2. Comment above heap_hot_search_buffer should be updated to document
> that heapTuple is an out-parameter and document the behavior of
> first_call
>
> 3. The logic around "skip" is slightly confusing to me. Here's my
> description: if it's not an MVCC snapshot and it's not the first call,
> then you don't actually want to fetch the tuple with the given tid or a
> later one in the chain -- you want to fetch the _next_ tuple in the
> chain or a later one in the chain. Some wording of that description in a
> comment (either in the function's comment or near the use of "skip")
> would help a lot. Also, if skip is true, then the tid _must_ be visible
> according to the (non-MVCC) snapshot, correct? It might help if that was
> apparent from the code/comments.
>
> Other than that, it looks good.

OK, I've applied this with some additional comment changes.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company