Re: [PATCH] unified frontend support for pg_malloc et al and palloc/pfree mulation (was xlogreader-v4)
От | Heikki Linnakangas |
---|---|
Тема | Re: [PATCH] unified frontend support for pg_malloc et al and palloc/pfree mulation (was xlogreader-v4) |
Дата | |
Msg-id | 50F1C41D.8040304@vmware.com обсуждение исходный текст |
Ответ на | Re: [PATCH] unified frontend support for pg_malloc et al and palloc/pfree mulation (was xlogreader-v4) (Andres Freund <andres@2ndquadrant.com>) |
Ответы |
Re: [PATCH] unified frontend support for pg_malloc et al and palloc/pfree mulation (was xlogreader-v4)
(Tom Lane <tgl@sss.pgh.pa.us>)
|
Список | 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@vmware.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
Вложения
В списке pgsql-hackers по дате отправления:
Предыдущее
От: "Kevin Grittner"Дата:
Сообщение: Re: 9.2.1 & index-only scans : abnormal heap fetches after VACUUM FULL