Re: [PATCH] unified frontend support for pg_malloc et al and palloc/pfree mulation (was xlogreader-v4)

On 2013-01-11 15:05:54 -0500, Tom Lane wrote:
> Andres Freund <andres(at)2ndquadrant(dot)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