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));
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:
#undef fprintf
#define fprintf(file, fmt, msg) ereport(ERROR, (errmsg_internal("%s",
msg)))
and flex does this:
(void) fprintf( stderr, "%s\n", msg );
but that's easy to work around by creating a wrapper fprintf function in
those .l files.
When I compile with gcc -O0, I get one warning with this:
datetime.c: In function ‘DateTimeParseError’:
datetime.c:3575:1: warning: ‘noreturn’ function does return [enabled by
default]
That suggests that the compiler didn't correctly deduce that
ereport(ERROR, ...) doesn't return. With -O1, the warning goes away.
- Heikki
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
