Re: SSI patch version 14

From: "Kevin Grittner" <Kevin(dot)Grittner(at)wicourts(dot)gov>
To: "Jeff Davis" <pgsql(at)j-davis(dot)com>
Cc: <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: SSI patch version 14
Date: 2011-02-01 17:01:39
Message-ID: 4D47E813020000250003A0E9@gw.wicourts.gov
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Jeff Davis <pgsql(at)j-davis(dot)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

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Alex Hunsaker 2011-02-01 17:04:47 Re: arrays as pl/perl input arguments [PATCH]
Previous Message Alex Hunsaker 2011-02-01 16:51:55 Re: Optimize PL/Perl function argument passing [PATCH]