On Fri, 2011-06-24 at 15:32 -0400, Robert Haas wrote: > On Sun, Jun 19, 2011 at 2:16 PM, Robert Haas <robertmh...@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. 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. Regards, Jeff Davis -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers