Re: Missing errcode() in ereport

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