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

On 2013-01-12 12:26:37 +0200, Heikki Linnakangas wrote:
> On 11.01.2013 23:49, Tom Lane wrote:
> >Andres Freund<andres(at)2ndquadrant(dot)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