Re: Missing errcode() in ereport - Mailing list pgsql-hackers

Hi,

On 2020-03-23 17:24:49 -0400, Tom Lane wrote:
> I wrote:
> > On balance I'm leaning towards keeping the parens as preferred style
> > for now, adjusting v12 so that the macro will allow paren omission
> > but we don't break ABI, and not touching the older branches.
> 
> Hearing no objections, I started to review Andres' patchset with
> that plan in mind.  I noted two things that I don't agree with:
> 
> 1. I think we should write the ereport macro as
> 
>         if (errstart(...)) \
>             __VA_ARGS__, errfinish(...); \
> 
> as I had it, not
> 
>         if (errstart(...)) \
>         { \
>             __VA_ARGS__; \
>             errfinish(...); \
>         } \
> 
> as per Andres.  The reason is that I don't have as much faith in the
> latter construct producing warnings for no-op expressions.

Hm. I don't think it'll be better, but I don't have a problem going with
yours either.


> 2. We cannot postpone the passing of the "domain" argument as Andres'
> 0003 patch proposes, because that has to be available to the auxiliary
> error functions so they do message translation properly.

Ah, good point.


I wondered before whether there's a way we could move the elevel check
in errstart to the macro. For it to be a win we'd presumably have to
have a "synthesized" log_level variable, basically
min(log_min_messages, client_min_messages, ERROR).

Probably not worth it.


> Maybe it'd be possible to finagle things to postpone translation to
> the very end, but the provisions for errcontext messages being
> translated in a different domain would make that pretty ticklish.
> Frankly I don't think it'd be worth the complication.  There is a
> clear benefit in delaying the passing of filename (since we can skip
> that strchr() call) but beyond that it seems pretty marginal.

Fair enough.


> Other than that it looks pretty good.  I wrote some documentation
> adjustments and re-split the patch into 0001, which is proposed for
> back-patch into v12, and 0002 which would have to be HEAD only.

Cool.


> One thing I'm not totally sure about is whether we can rely on
> this change:
> 
> -extern void errfinish(int dummy,...);
> +extern void errfinish(void);
> 
> being fully ABI-transparent.  One would think that as long as errfinish
> doesn't inspect its argument(s) it doesn't matter whether any are passed,
> but maybe somewhere there's an architecture where the possible presence
> of varargs arguments makes for a significant difference in the calling
> convention?  We could leave that change out of the v12 patch if we're
> worried about it.

I would vote for leaving that out of the v12 patch. I'm less worried
about odd architectures, and more about sanitizers and/or compilers
emitting "security enhanced" code checking signatures match etc
("control flow integrity").

Greetings,

Andres Freund