Skip site navigation (1) Skip section navigation (2)

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

From: Andres Freund <andres(at)2ndquadrant(dot)com>
To: Heikki Linnakangas <hlinnakangas(at)vmware(dot)com>
Cc: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Peter Eisentraut <peter_e(at)gmx(dot)net>,pgsql-hackers(at)postgresql(dot)org
Subject: Re: [PATCH] unified frontend support for pg_malloc et al and palloc/pfree mulation (was xlogreader-v4)
Date: 2013-01-12 17:20:34
Message-ID: 20130112172034.GA21523@awork2.anarazel.de (view raw or flat)
Thread:
Lists: 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(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


In response to

pgsql-hackers by date

Next:From: Tom LaneDate: 2013-01-12 18:16:56
Subject: Re: [PATCH] unified frontend support for pg_malloc et al and palloc/pfree mulation (was xlogreader-v4)
Previous:From: Simon RiggsDate: 2013-01-12 16:39:15
Subject: Hash twice

Privacy Policy | About PostgreSQL
Copyright © 1996-2014 The PostgreSQL Global Development Group