Re: [PATCH] unified frontend support for pg_malloc et al and palloc/pfree mulation (was xlogreader-v4)

Heikki Linnakangas <hlinnakangas@vmware.com> writes:
> On 11.01.2013 23:49, Tom Lane wrote:
>> Hm ... well, that's a point.  I may be putting too much weight on the
>> fact that any such bug for elevel would be a new one that never existed
>> before.  What do others think?

> It sure would be nice to avoid multiple evaluation. At least in xlog.c 
> we use emode_for_corrupt_record() to pass the elevel, and it does have a 
> side-effect.

Um.  I had forgotten about that example, but its existence is sufficient
to reinforce my opinion that we must not risk double evaluation of the
elevel argument.

> Here's one more construct to consider:

> #define ereport_domain(elevel, domain, rest) \
>     do { \
>         const int elevel_ = elevel;    \
>         if (errstart(elevel_,__FILE__, __LINE__, PG_FUNCNAME_MACRO, domain) || 
> elevel_>= ERROR) \
>         { \
>             (void) rest; \
>             if (elevel_>= ERROR) \
>                 errfinish_noreturn(1); \
>             else \
>                 errfinish(1); \
>         } \
>     } while(0)

I don't care for that too much in detail -- if errstart were to return
false (it shouldn't, but if it did) this would be utterly broken,
because we'd be making error reporting calls that elog.c wouldn't be
prepared to accept.  We should stick to the technique of doing the
ereport as today and then adding something that tells the compiler we
don't expect to get here; preferably something that emits no added code.

However, using a do-block with a local variable is definitely something
worth considering.  I'm getting less enamored of the __builtin_constant_p
idea after finding out that the gcc boys seem to have curious ideas
about what its semantics ought to be:
https://bugzilla.redhat.com/show_bug.cgi?id=894515
        regards, tom lane