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: Andres Freund <andres(at)2ndquadrant(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 20:14:21
Message-ID: 50F1C41D.8040304@vmware.com (view raw or flat)
Thread:
Lists: pgsql-hackers
On 12.01.2013 20:42, Andres Freund wrote:
> On 2013-01-12 13:16:56 -0500, Tom Lane wrote:
>> Heikki Linnakangas<hlinnakangas(at)vmware(dot)com>  writes:
>>> 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)
>>
>> I don't care for that too much in detail -- if errstart were to return
>> false (it shouldn't, but if it did) this would be utterly broken,
>> because we'd be making error reporting calls that elog.c wouldn't be
>> prepared to accept.  We should stick to the technique of doing the
>> ereport as today and then adding something that tells the compiler we
>> don't expect to get here; preferably something that emits no added code.

With the current ereport(), we'll call abort() if errstart returns false 
and elevel >= ERROR. That's even worse than making an error reporting 
calls that elog.c isn't prepared for. If we take that risk seriously, 
with bogus error reporting calls we at least have chance of catching it 
and reporting an error.

> Yea, I didn't really like it all that much either - but anyway, I have
> yet to find *any* version with a local variable that doesn't lead to
> 200kb size increase :(.

Hmm, that's strange. Assuming the optimizer optimizes away the paths in 
the macro that are never taken when elevel is a constant, I would expect 
the resulting binary to be smaller than what we have now. All those 
useless abort() calls must take up some space.

Here's what I got with and without my patch on my laptop:

-rwxr-xr-x 1 heikki heikki 24767237 tammi 12 21:27 postgres-do-while-mypatch
-rwxr-xr-x 1 heikki heikki 24623081 tammi 12 21:28 postgres-unpatched

But when I build without --enable-debug, the situation reverses:

-rwxr-xr-x 1 heikki heikki 5961309 tammi 12 21:48 postgres-do-while-mypatch
-rwxr-xr-x 1 heikki heikki 5986961 tammi 12 21:49 postgres-unpatched

I suspect you're also just seeing bloat caused by more debugging 
symbols, which won't have any effect at runtime. It would be nice to 
have smaller debug-enabled builds, too, of course, but it's not that 
important.

>> However, using a do-block with a local variable is definitely something
>> worth considering.  I'm getting less enamored of the __builtin_constant_p
>> idea after finding out that the gcc boys seem to have curious ideas
>> about what its semantics ought to be:
>> https://bugzilla.redhat.com/show_bug.cgi?id=894515
>
> I wonder whether __builtin_choose_expr is any better?

I forgot to mention (and it wasn't visible in the snippet I posted) the 
reason I used __attribute__ ((noreturn)). We already use that attribute 
elsewhere, so this optimization wouldn't require anything more from the 
compiler than what already take advantage of. I think that noreturn is 
more widely supported than these other constructs. Also, if I'm reading 
the MSDN docs correctly, while MSVC doesn't understand __attribute__ 
((noreturn)) directly, it has an equivalent syntax _declspec(noreturn). 
So we could easily support MSVC with this approach.

Attached is the complete patch I used for the above tests.

- Heikki

Attachment: ereport-with-do-while-1.patch
Description: text/x-diff (4.0 KB)

In response to

Responses

pgsql-hackers by date

Next:From: Aaron W. SwensonDate: 2013-01-12 20:30:12
Subject: Re: "pg_ctl promote" exit status
Previous:From: Kevin GrittnerDate: 2013-01-12 19:14:03
Subject: Re: 9.2.1 & index-only scans : abnormal heap fetches after VACUUM FULL

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