Re: [PATCH] unified frontend support for pg_malloc et al and palloc/pfree mulation (was xlogreader-v4)
От | Andres Freund |
---|---|
Тема | Re: [PATCH] unified frontend support for pg_malloc et al and palloc/pfree mulation (was xlogreader-v4) |
Дата | |
Msg-id | 20130111203345.GA26751@awork2.anarazel.de обсуждение исходный текст |
Ответ на | Re: Re: [PATCH] unified frontend support for pg_malloc et al and palloc/pfree mulation (was xlogreader-v4) (Tom Lane <tgl@sss.pgh.pa.us>) |
Ответы |
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 2013-01-11 15:05:54 -0500, Tom Lane wrote: > Andres Freund <andres@2ndquadrant.com> writes: > > [ patch to mark elog() as non-returning if elevel >= ERROR ] > > It strikes me that both in this patch, and in Peter's previous patch to > do this for ereport(), there is an opportunity for improvement. Namely, > that the added code only has any use if elevel is a constant, which in > some places it isn't. We don't really need a call to abort() to be > executed there, but the compiler knows no better and will have to > generate code to test the value of elevel and call abort(). > Which rather defeats the purpose of saving code; plus the compiler will still > not have any knowledge that code after the ereport isn't reached. Yea, I think that's why __builtin_unreachable() is a performance benefit in comparison with abort(). Both are a benefit over not doing either btw in my measurements. > And another thing: what if the elevel argument isn't safe for multiple > evaluation? No such hazard ever existed before these patches, so I'm > not very comfortable with adding one. (Even if all our own code is > safe, there's third-party code to worry about.) Hm. I am not really too scared about those dangers I have to admit. If at all I am caring more for ereport than for elog. Placing code with side effects as elevel arguments just seems a bit too absurd. > When we're using recent gcc, there is an easy fix, which is that the > macros could do this: Recent as in 3.1 or so ;) Its really too bad that there's no __builtin_pure() or __builtin_side_effect_free() or somesuch :(. > __builtin_constant_p(elevel) && (elevel) >= ERROR : pg_unreachable() : (void) 0 > > This gets rid of both useless code and the risk of undesirable multiple > evaluation, while not giving up any optimization possibility that > existed before. So I think we should add a configure test for > __builtin_constant_p() and do it like that when possible. I think there might be code somewhere that decides between FATAL and PANIC which would not hit that path then. Imo not a good enough reason not to do it, but I wanted to mention it anyway. > That still leaves the question of what to do when the compiler doesn't > have __builtin_constant_p(). Should we use the coding that we have, > or give up and not try to mark the macros as non-returning? I'm rather > inclined to do the latter, because of the multiple-evaluation-risk > argument. I dislike the latter somewhat as it would mean not to give that information at all to msvc and others which seems a bit sad. But I don't feel particularly strongly. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
В списке pgsql-hackers по дате отправления: