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

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.
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.)

When we're using recent gcc, there is an easy fix, which is that the
macros could do this:

__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.

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.

Comments?

regards, tom lane