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
