|From:||Jeff Davis <pgsql(at)j-davis(dot)com>|
|To:||Robert Haas <robertmhaas(at)gmail(dot)com>|
|Subject:||Re: heap_hot_search_buffer refactoring|
|Views:||Raw Message | Whole Thread | Download mbox | Resend email|
On Fri, 2011-06-24 at 15:32 -0400, Robert Haas wrote:
> On Sun, Jun 19, 2011 at 2:16 PM, Robert Haas <robertmhaas(at)gmail(dot)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.
2. Comment above heap_hot_search_buffer should be updated to document
that heapTuple is an out-parameter and document the behavior of
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.
|Next Message||Kevin Grittner||2011-06-25 19:29:38||Re: Repeated PredicateLockRelation calls during seqscan|
|Previous Message||Martijn van Oosterhout||2011-06-25 09:05:30||Re: debugging tools inside postgres|