Re: Missing errcode() in ereport

Hi,

On 2020-03-19 22:32:30 -0400, Tom Lane wrote:
> Andres Freund <andres(at)anarazel(dot)de> writes:
> > I wonder if it'd become a relevant backpatch pain if we started to have
> > some ereports() without the additional parens in 13+.
>
> Yeah, it would be a nasty backpatch hazard.
>
> > Would it perhaps
> > make sense to backpatch just the part that removes the need for the
> > parents, but not the return type changes?
>
> I was just looking into that. It would be pretty painless to do it
> in v12, but before that we weren't requiring C99. Having said that,
> trolling the buildfarm database shows exactly zero active members
> that don't report having __VA_ARGS__, in the branches where we test
> that. (And pg_config.h.win32 was assuming that MSVC had that, too.)
>
> Could we get away with moving the compiler goalposts for the back
> branches? I dunno, but it's a fact that we aren't testing anymore
> with any compilers that would complain about unconditional use of
> __VA_ARGS__. So it might be broken already and we wouldn't know it.

FWIW, I did grep for unprotected uses, and didn't find anything.

> (I suspect the last buildfarm animal that would've complained about
> this was pademelon, which I retired more than a year ago IIRC.)

I guess a query that searches the logs backwards for animals without
__VA_ARGS__ would be a good idea?

> > We can also remove elog() support code now, because with __VA_ARGS__
> > support it's really just a wrapper for ereport(elevel,
> > errmsg(__VA_ARGS_)). This is patch 0002.
>
> Yeah, something similar had occurred to me but I didn't write the patch
> yet. Note it should be errmsg_internal();

Oh, right.

> also, does the default for errcode come out the same?

I think so - there's no distinct code setting sqlerrcode in
elog_start/finish. That already relied on errstart().

> > I wonder if its worth additionally ensuring that errcode, errmsg,
> > ... are only called within errstart/errfinish?
>
> Meh. That's wrong at least for errcontext(), and I'm not sure it's
> really worth anything to enforce it for the others.

Yea, I'm not convinced either. Especially after changing the err* return
types to void, as that presumably will cause errors with a lot of
incorrect parens, e.g. about a function with void return type as an
argument to errmsg().

> I think the key decision we'd have to make to move forward on this
> is to decide whether it's still project style to prefer the extra
> parens, or whether we want new code to do without them going
> forward. If we don't want to risk requiring __VA_ARGS__ for the
> old branches then I'd vote in favor of keeping the parens as
> preferred style, at least till v11 is out of support.

Agreed.

> If we do change that in the back branches then there'd be reason to
> prefer to go without parens. New coders might still be confused about
> why there are all these calls with the useless parens, though.

That seems to be an acceptable pain, from my POV.

Greetings,

Andres Freund