On Mon, 2011-01-31 at 17:55 -0600, Kevin Grittner wrote:
>
http://git.postgresql.org/gitweb?p=users/kgrittn/postgres.git;a=commitdiff;h=6360b0d4ca88c09cf590a75409cd29831afff58b
>
> With confidence that it works, I looked it over some more and now
> like this a lot. It is definitely more readable and should be less
> fragile in the face of changes to MVCC bit-twiddling techniques. Of
> course, any changes to the HTSV_Result enum will require changes to
> this code, but that seems easier to spot and fix than the
> alternative. Thanks for the suggestion!
One thing that confused me a little about the code is the default case
at the end. The enum is exhaustive, so the default doesn't really make
sense. The compiler warning you are silencing is the uninitialized
variable xid (right?), which is clearly a spurious warning. Since you
have the "Assert(TransactionIdIsValid(xid))" there anyway, why not just
initialize xid to InvalidTransactionId and get rid of the default case?
I assume the "Assert(false)" is there to detect if someone adds a new
enum value, but the compiler should issue a warning in that case anyway
(and the comment next to Assert(false) is worded in a confusing way).
This is all really minor stuff, obviously.
Also, from a code standpoint, it might be possible to early return in
the HEAPTUPLE_RECENTLY_DEAD case where visible=false. It looks like it
will be handled quickly afterward (at TransactionIdPrecedes), so you
don't have to change anything, but I thought I would mention it.
> Having gotten my head around it, I embellished here:
>
>
http://git.postgresql.org/gitweb?p=users/kgrittn/postgres.git;a=commitdiff;h=f9307a41c198a9aa4203eb529f9c6d1b55c5c6e1
>
> Do those changes look reasonable? None of that is really
> *necessary*, but it seemed cleaner and clearer that way once I
> looked at the code with the changes you suggested.
Yes, I like those changes.
Regards,Jeff Davis