On Sat, Jun 25, 2011 at 6:24 AM, Jeff Davis <pg...@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 <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.
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
