Re: Missing errcode() in ereport

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Andres Freund <andres(at)anarazel(dot)de>
Cc: Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>, Michael Paquier <michael(at)paquier(dot)xyz>, Julien Rouhaud <rjuju123(at)gmail(dot)com>, Masahiko Sawada <masahiko(dot)sawada(at)2ndquadrant(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>
Subject: Re: Missing errcode() in ereport
Date: 2020-03-23 21:24:49
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

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.

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

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.

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.

regards, tom lane

Attachment Content-Type Size
0001-allow-ereport-with-fewer-parens.patch text/x-diff 8.6 KB
0002-cleanups-that-break-ABI.patch text/x-diff 32.2 KB

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Andres Freund 2020-03-23 21:44:46 Re: Missing errcode() in ereport
Previous Message Andres Freund 2020-03-23 21:00:37 Re: Additional size of hash table is alway zero for hash aggregates