On 2013-01-12 12:26:37 +0200, Heikki Linnakangas wrote:
> On 11.01.2013 23:49, Tom Lane wrote:
> >Andres Freund<and...@2ndquadrant.com>  writes:
> >>On 2013-01-11 16:28:13 -0500, Tom Lane wrote:
> >>>I'm not very satisfied with that answer.  Right now, Peter's
> >>>patch has added a class of bugs that never existed before 9.3, and yours
> >>>would add more.  It might well be that those classes are empty ... but
> >>>*we can't know that*.  I don't think that keeping a new optimization for
> >>>non-gcc compilers is worth that risk.  Postgres is already full of
> >>>gcc-only optimizations, anyway.
> >
> >>ISTM that ereport() already has so strange behaviour wrt evaluation of
> >>arguments (i.e doing it only when the message is really logged) that its
> >>doesn't seem to add a real additional risk.
> >
> >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. It's quite harmless in practice, you'll get some extra DEBUG1
> messages in the log, but still.
> 
> 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)
> 
> extern void errfinish(int dummy,...);
> extern void errfinish_noreturn(int dummy,...) __attribute__((noreturn));

I am afraid that that would bloat the code again as it would have to put
that if() into each callsite whereas a variant that ends up using
__builtin_unreachable() doesn't. So I think we should use
__builtin_unreachable() while falling back to abort() as before. But
that's really more a detail than anything fundamental about the approach.

I'll play a bit.

> With do-while, ereport() would no longer be an expression. There only place
> where that's a problem in our codebase is in scan.l, bootscanner.l and
> repl_scanner.l, which do this:

I aggree that would easy to fix, so no problem there.

Greetings,

Andres Freund

-- 
 Andres Freund                     http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Reply via email to