Jeff Davis <pgsql@j-davis.com> wrote:
> 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?)
Right.
> Since you have the "Assert(TransactionIdIsValid(xid))" there
> anyway, why not just initialize xid to InvalidTransactionId and
> get rid of the default case?
I feel a little better keeping even that trivial work out of the
code path if possible, and it seems less confusing to me on the
default case than up front. I'll improve the comment.
> 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
My compiler doesn't. Would it make sense to elog here, rather than
Assert? I'm not clear on the rules for that. I'll push something
that way for review and comment. If it's wrong, I'll change it.
> This is all really minor stuff, obviously.
In a million line code base, I hate to call anything which affects
readability minor. ;-)
> Also, from a code standpoint, it might be possible to early return
> in the HEAPTUPLE_RECENTLY_DEAD case where visible=false.
Yeah, good point. It seems worth testing a bool there.
A small push dealing with all the above issues and adding a little
to comments:
http://git.postgresql.org/gitweb?p=users/kgrittn/postgres.git;a=commitdiff;h=538ff57691256de0341e22513f59e9dc4dfd998f
Let me know if any of that still needs work to avoid confusion and
comply with PostgreSQL coding conventions. Like I said, I'm not
totally clear whether elog is right here, but it seems to me a
conceptually similar case to some I found elsewhere that elog was
used.
-Kevin