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

From Andres Freund
Subject Re: [PATCH] unified frontend support for pg_malloc et al and palloc/pfree mulation (was xlogreader-v4)
Date
Msg-id 20130112172034.GA21523@awork2.anarazel.de
Whole thread Raw
In response to Re: [PATCH] unified frontend support for pg_malloc et al and palloc/pfree mulation (was xlogreader-v4)  (Heikki Linnakangas <hlinnakangas@vmware.com>)
List pgsql-hackers
On 2013-01-12 12:26:37 +0200, Heikki Linnakangas wrote:
> On 11.01.2013 23:49, Tom Lane wrote:
> >Andres Freund<andres@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



pgsql-hackers by date:

Previous
From: Simon Riggs
Date:
Subject: Hash twice
Next
From: Tom Lane
Date:
Subject: Re: [PATCH] unified frontend support for pg_malloc et al and palloc/pfree mulation (was xlogreader-v4)