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: Heikki Linnakangas <hlinnakangas(at)vmware(dot)com>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: Andres Freund <andres(at)2ndquadrant(dot)com>, 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 10:26:37
Message-ID: 50F13A5D.2030104@vmware.com (view raw or flat)
Thread:
Lists: pgsql-hackers
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));

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


In response to

Responses

pgsql-hackers by date

Next:From: Simon RiggsDate: 2013-01-12 11:06:05
Subject: Re: Performance Improvement by reducing WAL for Update Operation
Previous:From: Simon RiggsDate: 2013-01-12 10:15:43
Subject: Re: Performance Improvement by reducing WAL for Update Operation

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