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