Thread: Missing errcode() in ereport
Hi,
In PageIsVerified() we report a WARNING as follows:
ereport(WARNING,
(ERRCODE_DATA_CORRUPTED,
errmsg("page verification failed, calculated checksum
%u but expected %u",
checksum, p->pd_checksum)));
However the error message won't have sql error code due to missing
errcode(). As far as I can see there are four places:
$ git grep "(ERRCODE" | grep -v errcode
contrib/adminpack/adminpack.c:
(ERRCODE_DUPLICATE_FILE,
contrib/adminpack/adminpack.c: (ERRCODE_DUPLICATE_FILE,
contrib/adminpack/adminpack.c:
(ERRCODE_UNDEFINED_FILE,
src/backend/storage/page/bufpage.c:
(ERRCODE_DATA_CORRUPTED,
src/pl/plpgsql/src/pl_exec.c: else if
(ERRCODE_IS_CATEGORY(sqlerrstate) &&
Attached patch add errcode() to these places.
Regards,
--
Masahiko Sawada http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachment
On Tue, Mar 17, 2020 at 2:08 PM Masahiko Sawada
<masahiko.sawada@2ndquadrant.com> wrote:
>
> Hi,
>
> In PageIsVerified() we report a WARNING as follows:
>
> ereport(WARNING,
> (ERRCODE_DATA_CORRUPTED,
> errmsg("page verification failed, calculated checksum
> %u but expected %u",
> checksum, p->pd_checksum)));
>
> However the error message won't have sql error code due to missing
> errcode(). As far as I can see there are four places:
>
> $ git grep "(ERRCODE" | grep -v errcode
> contrib/adminpack/adminpack.c:
> (ERRCODE_DUPLICATE_FILE,
> contrib/adminpack/adminpack.c: (ERRCODE_DUPLICATE_FILE,
> contrib/adminpack/adminpack.c:
> (ERRCODE_UNDEFINED_FILE,
> src/backend/storage/page/bufpage.c:
> (ERRCODE_DATA_CORRUPTED,
> src/pl/plpgsql/src/pl_exec.c: else if
> (ERRCODE_IS_CATEGORY(sqlerrstate) &&
>
> Attached patch add errcode() to these places.
>
+1. This looks like an oversight to me.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
On Tue, Mar 17, 2020 at 10:00 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Tue, Mar 17, 2020 at 2:08 PM Masahiko Sawada
> <masahiko.sawada@2ndquadrant.com> wrote:
> >
> > Hi,
> >
> > In PageIsVerified() we report a WARNING as follows:
> >
> > ereport(WARNING,
> > (ERRCODE_DATA_CORRUPTED,
> > errmsg("page verification failed, calculated checksum
> > %u but expected %u",
> > checksum, p->pd_checksum)));
> >
> > However the error message won't have sql error code due to missing
> > errcode(). As far as I can see there are four places:
> >
> > $ git grep "(ERRCODE" | grep -v errcode
> > contrib/adminpack/adminpack.c:
> > (ERRCODE_DUPLICATE_FILE,
> > contrib/adminpack/adminpack.c: (ERRCODE_DUPLICATE_FILE,
> > contrib/adminpack/adminpack.c:
> > (ERRCODE_UNDEFINED_FILE,
> > src/backend/storage/page/bufpage.c:
> > (ERRCODE_DATA_CORRUPTED,
> > src/pl/plpgsql/src/pl_exec.c: else if
> > (ERRCODE_IS_CATEGORY(sqlerrstate) &&
> >
> > Attached patch add errcode() to these places.
> >
>
> +1. This looks like an oversight to me.
good catch! And patch LGTM.
On Tue, Mar 17, 2020 at 10:26:57AM +0100, Julien Rouhaud wrote: > On Tue, Mar 17, 2020 at 10:00 AM Amit Kapila <amit.kapila16@gmail.com> wrote: >> +1. This looks like an oversight to me. > > good catch! And patch LGTM. Definitely an oversight. All stable branches down to 9.5 have mistakes in the same area, with nothing extra by grepping around. Amit, I guess that you will take care of it? -- Michael
Attachment
On Tue, Mar 17, 2020 at 3:28 PM Michael Paquier <michael@paquier.xyz> wrote: > > On Tue, Mar 17, 2020 at 10:26:57AM +0100, Julien Rouhaud wrote: > > On Tue, Mar 17, 2020 at 10:00 AM Amit Kapila <amit.kapila16@gmail.com> wrote: > >> +1. This looks like an oversight to me. > > > > good catch! And patch LGTM. > > Definitely an oversight. All stable branches down to 9.5 have > mistakes in the same area, with nothing extra by grepping around. > Amit, I guess that you will take care of it? > Yes, I will unless I see any objections in a day or so. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
Amit Kapila <amit.kapila16@gmail.com> writes:
> On Tue, Mar 17, 2020 at 3:28 PM Michael Paquier <michael@paquier.xyz> wrote:
>> Definitely an oversight. All stable branches down to 9.5 have
>> mistakes in the same area, with nothing extra by grepping around.
>> Amit, I guess that you will take care of it?
> Yes, I will unless I see any objections in a day or so.
No need to wait, it's a pretty obvious thinko.
We might want to spend some effort thinking how to find or prevent
additional bugs of the same ilk ... but correcting the ones already
found needn't wait on that.
regards, tom lane
On Tue, Mar 17, 2020 at 7:39 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Amit Kapila <amit.kapila16@gmail.com> writes: > > On Tue, Mar 17, 2020 at 3:28 PM Michael Paquier <michael@paquier.xyz> wrote: > >> Definitely an oversight. All stable branches down to 9.5 have > >> mistakes in the same area, with nothing extra by grepping around. > >> Amit, I guess that you will take care of it? > > > Yes, I will unless I see any objections in a day or so. > > No need to wait, it's a pretty obvious thinko. > Okay, I will push in some time. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
On Wed, Mar 18, 2020 at 9:01 AM Amit Kapila <amit.kapila16@gmail.com> wrote: > > On Tue, Mar 17, 2020 at 7:39 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > > > Amit Kapila <amit.kapila16@gmail.com> writes: > > > On Tue, Mar 17, 2020 at 3:28 PM Michael Paquier <michael@paquier.xyz> wrote: > > >> Definitely an oversight. All stable branches down to 9.5 have > > >> mistakes in the same area, with nothing extra by grepping around. > > >> Amit, I guess that you will take care of it? > > > > > Yes, I will unless I see any objections in a day or so. > > > > No need to wait, it's a pretty obvious thinko. > > > > Okay, I will push in some time. > Pushed. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
On Wed, 18 Mar 2020 at 13:57, Amit Kapila <amit.kapila16@gmail.com> wrote: > > On Wed, Mar 18, 2020 at 9:01 AM Amit Kapila <amit.kapila16@gmail.com> wrote: > > > > On Tue, Mar 17, 2020 at 7:39 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > > > > > Amit Kapila <amit.kapila16@gmail.com> writes: > > > > On Tue, Mar 17, 2020 at 3:28 PM Michael Paquier <michael@paquier.xyz> wrote: > > > >> Definitely an oversight. All stable branches down to 9.5 have > > > >> mistakes in the same area, with nothing extra by grepping around. > > > >> Amit, I guess that you will take care of it? > > > > > > > Yes, I will unless I see any objections in a day or so. > > > > > > No need to wait, it's a pretty obvious thinko. > > > > > > > Okay, I will push in some time. > > > > Pushed. Thank you! Regards, -- Masahiko Sawada http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Hi, On 2020-03-17 10:09:18 -0400, Tom Lane wrote: > We might want to spend some effort thinking how to find or prevent > additional bugs of the same ilk ... Yea, that'd be good. Trying to help people new to postgres write their first patches I found that ereport is very confusing to them - largely because the syntax doesn't make much sense. Made worse by the compiler error messages being terrible in many cases. Not sure there's much we can do without changing ereport's "signature" though :( Regards, Andres
Andres Freund <andres@anarazel.de> writes:
> On 2020-03-17 10:09:18 -0400, Tom Lane wrote:
>> We might want to spend some effort thinking how to find or prevent
>> additional bugs of the same ilk ...
> Yea, that'd be good. Trying to help people new to postgres write their
> first patches I found that ereport is very confusing to them - largely
> because the syntax doesn't make much sense. Made worse by the compiler
> error messages being terrible in many cases.
> Not sure there's much we can do without changing ereport's "signature"
> though :(
Now that we can rely on having varargs macros, I think we could
stop requiring the extra level of parentheses, ie instead of
ereport(ERROR,
(errcode(ERRCODE_DIVISION_BY_ZERO),
errmsg("division by zero")));
it could be just
ereport(ERROR,
errcode(ERRCODE_DIVISION_BY_ZERO),
errmsg("division by zero"));
(The old syntax had better still work, of course. I'm not advocating
running around and changing existing calls.)
I'm not sure that this'd really move the goalposts much in terms of making
it any less confusing, but possibly it would improve the compiler errors?
Do you have any concrete examples of crummy error messages?
regards, tom lane
Hi,
On 2020-03-19 14:07:04 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2020-03-17 10:09:18 -0400, Tom Lane wrote:
> >> We might want to spend some effort thinking how to find or prevent
> >> additional bugs of the same ilk ...
>
> > Yea, that'd be good. Trying to help people new to postgres write their
> > first patches I found that ereport is very confusing to them - largely
> > because the syntax doesn't make much sense. Made worse by the compiler
> > error messages being terrible in many cases.
>
> > Not sure there's much we can do without changing ereport's "signature"
> > though :(
>
> Now that we can rely on having varargs macros, I think we could
> stop requiring the extra level of parentheses, ie instead of
>
> ereport(ERROR,
> (errcode(ERRCODE_DIVISION_BY_ZERO),
> errmsg("division by zero")));
>
> it could be just
>
> ereport(ERROR,
> errcode(ERRCODE_DIVISION_BY_ZERO),
> errmsg("division by zero"));
>
> (The old syntax had better still work, of course. I'm not advocating
> running around and changing existing calls.)
I think that'd be an improvement, because:
> I'm not sure that this'd really move the goalposts much in terms of making
> it any less confusing, but possibly it would improve the compiler errors?
> Do you have any concrete examples of crummy error messages?
ane of the ones I saw confuse people is just:
/home/andres/src/postgresql/src/backend/tcop/postgres.c:3727:4: error: ‘ereport’ undeclared (first use in this
function);did you mean ‘rresvport’?
3727 | ereport(FATAL,
| ^~~~~~~
| rresvport
/home/andres/src/postgresql/src/backend/tcop/postgres.c:3727:4: note: each undeclared identifier is reported only once
foreach function it appears in
because the extra parens haven't been added.
I personally actually hit a variant of that on a semi-regular basis:
Closing the parens for "rest" early, as there's just so many parens in
ereports, especially if an errmsg argument is a function call
itself. Which leads to a variation of the above error message. I know
how to address it, obviously, but I do find it somewhat annoying to deal
with.
Another one I've both seen and committed byself is converting an elog to
an ereport, and not adding an errcode() around the error code - which
silently looks like it works.
Greetings,
Andres Freund
Andres Freund <andres@anarazel.de> writes:
> On 2020-03-19 14:07:04 -0400, Tom Lane wrote:
>> Now that we can rely on having varargs macros, I think we could
>> stop requiring the extra level of parentheses,
> I think that'd be an improvement, because:
> ane of the ones I saw confuse people is just:
> /home/andres/src/postgresql/src/backend/tcop/postgres.c:3727:4: error: ‘ereport’ undeclared (first use in this
function);did you mean ‘rresvport’?
> 3727 | ereport(FATAL,
> | ^~~~~~~
> | rresvport
> /home/andres/src/postgresql/src/backend/tcop/postgres.c:3727:4: note: each undeclared identifier is reported only
oncefor each function it appears in
> because the extra parens haven't been added.
Ah, so the macro isn't expanded at all if it appears to have more than
two parameters? That seems odd, but I suppose some compilers might
work that way. Switching to varargs would improve that for sure.
> Another one I've both seen and committed byself is converting an elog to
> an ereport, and not adding an errcode() around the error code - which
> silently looks like it works.
You mean not adding errmsg() around the error string? Yeah, I can
believe that one easily. It's sort of the same problem as forgetting
to wrap errcode() around the ERRCODE_ constant.
I think that at least some compilers will complain about side-effect-free
subexpressions of a comma expression. Could we restructure things so
that the errcode/errmsg/etc calls form a standalone comma expression
rather than appearing to be arguments of a varargs function? The
compiler's got no hope of realizing there's anything wrong when it
thinks it's passing an integer or string constant to a function it knows
nothing about. But if it could see that nothing at all is being done with
the constant, maybe we'd get somewhere.
regards, tom lane
I wrote:
> I think that at least some compilers will complain about side-effect-free
> subexpressions of a comma expression. Could we restructure things so
> that the errcode/errmsg/etc calls form a standalone comma expression
> rather than appearing to be arguments of a varargs function?
Yeah, the attached quick-hack patch seems to work really well for this.
I find that this now works:
ereport(ERROR,
errcode(ERRCODE_DIVISION_BY_ZERO),
errmsg("foo"));
where before it gave the weird error you quoted. Also, these both
draw warnings about side-effect-free expressions:
ereport(ERROR,
ERRCODE_DIVISION_BY_ZERO,
errmsg("foo"));
ereport(ERROR,
"%d", 42);
With gcc you just need -Wall to get such warnings, and clang
seems to give them by default.
As a nice side benefit, the backend gets a couple KB smaller from
removal of errfinish's useless dummy argument.
I think that we could now also change all the helper functions
(errmsg et al) to return void instead of a dummy value, but that
would make the patch a lot longer and probably not move the
goalposts much in terms of error detection.
It also looks like we could use the same trick to get plain elog()
to have the behavior of not evaluating its arguments when it's
not going to print anything. I've not poked at that yet either.
regards, tom lane
diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c
index 62eef7b..a29ccd9 100644
--- a/src/backend/utils/error/elog.c
+++ b/src/backend/utils/error/elog.c
@@ -438,7 +438,7 @@ matches_backtrace_functions(const char *funcname)
* See elog.h for the error level definitions.
*/
void
-errfinish(int dummy,...)
+errfinish(void)
{
ErrorData *edata = &errordata[errordata_stack_depth];
int elevel;
@@ -1463,7 +1463,7 @@ elog_finish(int elevel, const char *fmt,...)
/*
* And let errfinish() finish up.
*/
- errfinish(0);
+ errfinish();
}
@@ -1749,7 +1749,7 @@ ThrowErrorData(ErrorData *edata)
recursion_depth--;
/* Process the error. */
- errfinish(0);
+ errfinish();
}
/*
@@ -1863,7 +1863,7 @@ pg_re_throw(void)
*/
error_context_stack = NULL;
- errfinish(0);
+ errfinish();
}
/* Doesn't return ... */
diff --git a/src/include/utils/elog.h b/src/include/utils/elog.h
index 0a4ef02..e6fa85f 100644
--- a/src/include/utils/elog.h
+++ b/src/include/utils/elog.h
@@ -118,34 +118,34 @@
*----------
*/
#ifdef HAVE__BUILTIN_CONSTANT_P
-#define ereport_domain(elevel, domain, rest) \
+#define ereport_domain(elevel, domain, ...) \
do { \
pg_prevent_errno_in_scope(); \
if (errstart(elevel, __FILE__, __LINE__, PG_FUNCNAME_MACRO, domain)) \
- errfinish rest; \
+ __VA_ARGS__, errfinish(); \
if (__builtin_constant_p(elevel) && (elevel) >= ERROR) \
pg_unreachable(); \
} while(0)
#else /* !HAVE__BUILTIN_CONSTANT_P */
-#define ereport_domain(elevel, domain, rest) \
+#define ereport_domain(elevel, domain, ...) \
do { \
const int elevel_ = (elevel); \
pg_prevent_errno_in_scope(); \
if (errstart(elevel_, __FILE__, __LINE__, PG_FUNCNAME_MACRO, domain)) \
- errfinish rest; \
+ __VA_ARGS__, errfinish(); \
if (elevel_ >= ERROR) \
pg_unreachable(); \
} while(0)
#endif /* HAVE__BUILTIN_CONSTANT_P */
-#define ereport(elevel, rest) \
- ereport_domain(elevel, TEXTDOMAIN, rest)
+#define ereport(elevel, ...) \
+ ereport_domain(elevel, TEXTDOMAIN, __VA_ARGS__)
#define TEXTDOMAIN NULL
extern bool errstart(int elevel, const char *filename, int lineno,
const char *funcname, const char *domain);
-extern void errfinish(int dummy,...);
+extern void errfinish(void);
extern int errcode(int sqlerrcode);
Hi,
On 2020-03-19 19:32:55 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2020-03-19 14:07:04 -0400, Tom Lane wrote:
> >> Now that we can rely on having varargs macros, I think we could
> >> stop requiring the extra level of parentheses,
>
> > I think that'd be an improvement, because:
> > ane of the ones I saw confuse people is just:
> > /home/andres/src/postgresql/src/backend/tcop/postgres.c:3727:4: error: ‘ereport’ undeclared (first use in this
function);did you mean ‘rresvport’?
> > 3727 | ereport(FATAL,
> > | ^~~~~~~
> > | rresvport
> > /home/andres/src/postgresql/src/backend/tcop/postgres.c:3727:4: note: each undeclared identifier is reported only
oncefor each function it appears in
> > because the extra parens haven't been added.
>
> Ah, so the macro isn't expanded at all if it appears to have more than
> two parameters? That seems odd, but I suppose some compilers might
> work that way. Switching to varargs would improve that for sure.
Yea. Newer gcc versions do warn about a parameter mismatch count, which
helps. I'm not sure what the preprocessor / compiler should do intead?
FWIW, clang also doesn't expand the macro.
> > Another one I've both seen and committed byself is converting an elog to
> > an ereport, and not adding an errcode() around the error code - which
> > silently looks like it works.
>
> You mean not adding errmsg() around the error string? Yeah, I can
> believe that one easily. It's sort of the same problem as forgetting
> to wrap errcode() around the ERRCODE_ constant.
Both of these, I think.
> Could we restructure things so that the errcode/errmsg/etc calls form
> a standalone comma expression rather than appearing to be arguments of
> a varargs function? The compiler's got no hope of realizing there's
> anything wrong when it thinks it's passing an integer or string
> constant to a function it knows nothing about. But if it could see
> that nothing at all is being done with the constant, maybe we'd get
> somewhere.
Worth a try. Not doing a pointless varargs call could also end up
reducing elog overhead a bit (right now we push a lot of 0 as vararg
arguments for no reason).
A quick hack suggests that it works:
/home/andres/src/postgresql/src/backend/tcop/postgres.c: In function ‘process_postgres_switches’:
/home/andres/src/postgresql/src/backend/tcop/postgres.c:3728:27: warning: left-hand operand of comma expression has no
effect[-Wunused-value]
3728 | (ERRCODE_SYNTAX_ERROR,
| ^
/home/andres/src/postgresql/src/include/utils/elog.h:126:4: note: in definition of macro ‘ereport_domain’
126 | rest; \
| ^~~~
/home/andres/src/postgresql/src/backend/tcop/postgres.c:3727:4: note: in expansion of macro ‘ereport’
3727 | ereport(FATAL,
| ^~~~~~~
and in an optimized build the resulting code indeed looks a bit better:
before:
text data bss dec hex filename
8462795 176128 204464 8843387 86f07b src/backend/postgres
Looking at FloatExceptionHandler as an example:
2860 /* We're not returning, so no need to save errno */
2861 ereport(ERROR,
0x0000000000458bc0 <+0>: push %rbp
0x0000000000458bc1 <+1>: mov $0xb2d,%edx
0x0000000000458bc6 <+6>: lea 0x214c9b(%rip),%rsi # 0x66d868
0x0000000000458bcd <+13>: mov %rsp,%rbp
0x0000000000458bd0 <+16>: push %r13
0x0000000000458bd2 <+18>: xor %r8d,%r8d
0x0000000000458bd5 <+21>: lea 0x2d9dc4(%rip),%rcx # 0x7329a0 <__func__.41598>
0x0000000000458bdc <+28>: push %r12
0x0000000000458bde <+30>: mov $0x14,%edi
0x0000000000458be3 <+35>: callq 0x5a8c00 <errstart>
0x0000000000458be8 <+40>: lea 0x214e59(%rip),%rdi # 0x66da48
0x0000000000458bef <+47>: xor %eax,%eax
0x0000000000458bf1 <+49>: callq 0x5acb00 <errdetail>
0x0000000000458bf6 <+54>: mov %eax,%r13d
0x0000000000458bf9 <+57>: lea 0x1bf7fb(%rip),%rdi # 0x6183fb
0x0000000000458c00 <+64>: xor %eax,%eax
0x0000000000458c02 <+66>: callq 0x5ac710 <errmsg>
0x0000000000458c07 <+71>: mov $0x1020082,%edi
0x0000000000458c0c <+76>: mov %eax,%r12d
0x0000000000458c0f <+79>: callq 0x5ac5b0 <errcode>
0x0000000000458c14 <+84>: mov %eax,%edi
0x0000000000458c16 <+86>: mov %r13d,%edx
0x0000000000458c19 <+89>: mov %r12d,%esi
0x0000000000458c1c <+92>: xor %eax,%eax
0x0000000000458c1e <+94>: callq 0x5ac020 <errfinish>
vs after:
text data bss dec hex filename
8395731 176128 204464 8776323 85ea83 src/backend/postgres
2861 ereport(ERROR,
0x0000000000449dd0 <+0>: push %rbp
0x0000000000449dd1 <+1>: xor %r8d,%r8d
0x0000000000449dd4 <+4>: lea 0x2d8a65(%rip),%rcx # 0x722840 <__func__.41598>
0x0000000000449ddb <+11>: mov %rsp,%rbp
0x0000000000449dde <+14>: mov $0xb2d,%edx
0x0000000000449de3 <+19>: lea 0x2137ee(%rip),%rsi # 0x65d5d8
0x0000000000449dea <+26>: mov $0x14,%edi
0x0000000000449def <+31>: callq 0x5992e0 <errstart>
0x0000000000449df4 <+36>: mov $0x1020082,%edi
0x0000000000449df9 <+41>: callq 0x59cc80 <errcode>
0x0000000000449dfe <+46>: lea 0x1be496(%rip),%rdi # 0x60829b
0x0000000000449e05 <+53>: xor %eax,%eax
0x0000000000449e07 <+55>: callq 0x59cde0 <errmsg>
0x0000000000449e0c <+60>: lea 0x2139a5(%rip),%rdi # 0x65d7b8
0x0000000000449e13 <+67>: xor %eax,%eax
0x0000000000449e15 <+69>: callq 0x59d1d0 <errdetail>
0x0000000000449e1a <+74>: callq 0x59c700 <errfinish>
I wonder if it'd become a relevant backpatch pain if we started to have
some ereports() without the additional parens in 13+. Would it perhaps
make sense to backpatch just the part that removes the need for the
parents, but not the return type changes?
This is patch 0001.
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.
I think it might also be a good idea to move __FILE__, __LINE__,
PG_FUNCNAME_MACRO, domain from being parameters to errstart to
errfinish. For elevel < ERROR its sad that we currently pass them even
if we don't emit the message. This is patch 0003.
I wonder if its worth additionally ensuring that errcode, errmsg,
... are only called within errstart/errfinish? We could have errstart()
return the error being "prepared" and store it in a local variable, and
make errmsg() etc macros that internally pass that variable to the
"real" errmsg() etc. Like
#define errmsg(...) errmsg_impl(current_error, __VA_ARGS__)
and set up current_error in ereport_domain() roughly like
do {
ErrorData *current_error_;
if ((current_error_ = errstart(elevel, the, rest)) != NULL)
{
...
errfinish()
}
...
probably not worth it?
Greetings,
Andres Freund
Attachment
Hi,
On 2020-03-19 21:03:17 -0400, Tom Lane wrote:
> I wrote:
> > I think that at least some compilers will complain about side-effect-free
> > subexpressions of a comma expression. Could we restructure things so
> > that the errcode/errmsg/etc calls form a standalone comma expression
> > rather than appearing to be arguments of a varargs function?
>
> Yeah, the attached quick-hack patch seems to work really well for
> this.
Heh, you're too fast. I just sent something similar, and a few followup
patches.
What is your thinking about pain around backpatching it might introduce?
We can't easily backpatch support for ereport-without-extra-parens, I
think, because it needs __VA_ARGS__?
> As a nice side benefit, the backend gets a couple KB smaller from
> removal of errfinish's useless dummy argument.
I don't think it's the removal of the dummy parameter itself that
constitutes most of the savings, but instead it's not having to push the
return value of errmsg(), errcode(), et al as vararg arguments.
> I think that we could now also change all the helper functions
> (errmsg et al) to return void instead of a dummy value, but that
> would make the patch a lot longer and probably not move the
> goalposts much in terms of error detection.
I did include that in my prototype patch. Agree that it's not necessary
for the error detection capability, but it seems misleading to leave the
return values around if they're not actually needed.
> It also looks like we could use the same trick to get plain elog()
> to have the behavior of not evaluating its arguments when it's
> not going to print anything. I've not poked at that yet either.
I've included a patch for that. I think it's now sufficient to
#define elog(elevel, ...) \
ereport(elevel, errmsg(__VA_ARGS__))
which imo is quite nice, as it allows us to get rid of a lot of
duplication.
Greetings,
Andres Freund
Andres Freund <andres@anarazel.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.
(I suspect the last buildfarm animal that would've complained about
this was pademelon, which I retired more than a year ago IIRC.)
> 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(); also, does the default
for errcode come out the same?
> I think it might also be a good idea to move __FILE__, __LINE__,
> PG_FUNCNAME_MACRO, domain from being parameters to errstart to
> errfinish. For elevel < ERROR its sad that we currently pass them even
> if we don't emit the message. This is patch 0003.
Oh, that's a good idea that hadn't occurred to me.
> 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.
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. 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.
regards, tom lane
Hi, On 2020-03-19 22:32:30 -0400, Tom Lane wrote: > Andres Freund <andres@anarazel.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
Andres Freund <andres@anarazel.de> writes:
> On 2020-03-19 22:32:30 -0400, Tom Lane wrote:
>> 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.
Yeah, I also grepped the v11 branch for that.
>> (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?
I did a more wide-ranging scan, looking at the 9.4 branch as far back
as 2015. Indeed, pademelon is the only animal that ever reported
not having __VA_ARGS__ in that timeframe.
So I've got mixed emotions about this. On the one hand, it seems
quite unlikely that anyone would ever object if we started requiring
__VA_ARGS__ in the back branches. On the other hand, it's definitely
not project policy to change requirements like that in minor releases.
Also the actual benefit might not be much. If anyone did mistakenly
back-patch a fix that included a paren-free ereport, the buildfarm
would point out the error soon enough.
I thought for a little bit about making the back branches define ereport
with "..." if HAVE__VA_ARGS and otherwise not, but if we did that then
the buildfarm would *not* complain about paren-free ereports in the back
branches. I think that would inevitably lead to there soon being some,
so that we'd effectively be requiring __VA_ARGS__ anyway. (I suppose
I could resurrect pademelon ... but who knows whether that old war
horse will keep working for another four years till v11 is retired.)
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. But
if there's a consensus to require __VA_ARGS__ in the back branches,
I won't stand in the way.
regards, tom lane
On 2020-Mar-19, Tom Lane wrote: > 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. 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. It seems fine to accept new code in pg14 without the extra parens. All existing committers are (or should be) used to the style with the parens, so it's unlikely that we'll mess up when backpatching bugfixes; and even if we do, the buildfarm would alert us to that soon enough. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
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
diff --git a/doc/src/sgml/sources.sgml b/doc/src/sgml/sources.sgml
index 32ca220..283c3e0 100644
--- a/doc/src/sgml/sources.sgml
+++ b/doc/src/sgml/sources.sgml
@@ -103,9 +103,9 @@ less -x4
message text. In addition there are optional elements, the most
common of which is an error identifier code that follows the SQL spec's
SQLSTATE conventions.
- <function>ereport</function> itself is just a shell function, that exists
+ <function>ereport</function> itself is just a shell macro, that exists
mainly for the syntactic convenience of making message generation
- look like a function call in the C source code. The only parameter
+ look like a single function call in the C source code. The only parameter
accepted directly by <function>ereport</function> is the severity level.
The primary message text and any optional message elements are
generated by calling auxiliary functions, such as <function>errmsg</function>,
@@ -116,36 +116,50 @@ less -x4
A typical call to <function>ereport</function> might look like this:
<programlisting>
ereport(ERROR,
- (errcode(ERRCODE_DIVISION_BY_ZERO),
- errmsg("division by zero")));
+ errcode(ERRCODE_DIVISION_BY_ZERO),
+ errmsg("division by zero"));
</programlisting>
This specifies error severity level <literal>ERROR</literal> (a run-of-the-mill
error). The <function>errcode</function> call specifies the SQLSTATE error code
using a macro defined in <filename>src/include/utils/errcodes.h</filename>. The
- <function>errmsg</function> call provides the primary message text. Notice the
- extra set of parentheses surrounding the auxiliary function calls —
- these are annoying but syntactically necessary.
+ <function>errmsg</function> call provides the primary message text.
+ </para>
+
+ <para>
+ You will also frequently see this older style, with an extra set of
+ parentheses surrounding the auxiliary function calls:
+<programlisting>
+ereport(ERROR,
+ (errcode(ERRCODE_DIVISION_BY_ZERO),
+ errmsg("division by zero")));
+</programlisting>
+ The extra parentheses were required
+ before <productname>PostgreSQL</productname> version 12, but are now
+ optional.
</para>
<para>
Here is a more complex example:
<programlisting>
ereport(ERROR,
- (errcode(ERRCODE_AMBIGUOUS_FUNCTION),
- errmsg("function %s is not unique",
- func_signature_string(funcname, nargs,
- NIL, actual_arg_types)),
- errhint("Unable to choose a best candidate function. "
- "You might need to add explicit typecasts.")));
+ errcode(ERRCODE_AMBIGUOUS_FUNCTION),
+ errmsg("function %s is not unique",
+ func_signature_string(funcname, nargs,
+ NIL, actual_arg_types)),
+ errhint("Unable to choose a best candidate function. "
+ "You might need to add explicit typecasts."));
</programlisting>
This illustrates the use of format codes to embed run-time values into
a message text. Also, an optional <quote>hint</quote> message is provided.
+ The auxiliary function calls can be written in any order, but
+ conventionally <function>errcode</function>
+ and <function>errmsg</function> appear first.
</para>
<para>
If the severity level is <literal>ERROR</literal> or higher,
- <function>ereport</function> aborts the execution of the user-defined
- function and does not return to the caller. If the severity level is
+ <function>ereport</function> aborts execution of the current query
+ and does not return to the caller. If the severity level is
lower than <literal>ERROR</literal>, <function>ereport</function> returns normally.
</para>
@@ -390,7 +404,7 @@ elog(level, "format string", ...);
</programlisting>
is exactly equivalent to:
<programlisting>
-ereport(level, (errmsg_internal("format string", ...)));
+ereport(level, errmsg_internal("format string", ...));
</programlisting>
Notice that the SQLSTATE error code is always defaulted, and the message
string is not subject to translation.
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 00c77b6..cb8c23e 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -3720,15 +3720,15 @@ process_postgres_switches(int argc, char *argv[], GucContext ctx,
/* spell the error message a bit differently depending on context */
if (IsUnderPostmaster)
ereport(FATAL,
- (errcode(ERRCODE_SYNTAX_ERROR),
- errmsg("invalid command-line argument for server process: %s", argv[optind]),
- errhint("Try \"%s --help\" for more information.", progname)));
+ errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("invalid command-line argument for server process: %s", argv[optind]),
+ errhint("Try \"%s --help\" for more information.", progname));
else
ereport(FATAL,
- (errcode(ERRCODE_SYNTAX_ERROR),
- errmsg("%s: invalid command-line argument: %s",
- progname, argv[optind]),
- errhint("Try \"%s --help\" for more information.", progname)));
+ errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("%s: invalid command-line argument: %s",
+ progname, argv[optind]),
+ errhint("Try \"%s --help\" for more information.", progname));
}
/*
diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c
index 62eef7b..a29ccd9 100644
--- a/src/backend/utils/error/elog.c
+++ b/src/backend/utils/error/elog.c
@@ -438,7 +438,7 @@ matches_backtrace_functions(const char *funcname)
* See elog.h for the error level definitions.
*/
void
-errfinish(int dummy,...)
+errfinish(void)
{
ErrorData *edata = &errordata[errordata_stack_depth];
int elevel;
@@ -1463,7 +1463,7 @@ elog_finish(int elevel, const char *fmt,...)
/*
* And let errfinish() finish up.
*/
- errfinish(0);
+ errfinish();
}
@@ -1749,7 +1749,7 @@ ThrowErrorData(ErrorData *edata)
recursion_depth--;
/* Process the error. */
- errfinish(0);
+ errfinish();
}
/*
@@ -1863,7 +1863,7 @@ pg_re_throw(void)
*/
error_context_stack = NULL;
- errfinish(0);
+ errfinish();
}
/* Doesn't return ... */
diff --git a/src/include/utils/elog.h b/src/include/utils/elog.h
index 0a4ef02..73d84d1 100644
--- a/src/include/utils/elog.h
+++ b/src/include/utils/elog.h
@@ -91,9 +91,9 @@
/*----------
* New-style error reporting API: to be used in this way:
* ereport(ERROR,
- * (errcode(ERRCODE_UNDEFINED_CURSOR),
- * errmsg("portal \"%s\" not found", stmt->portalname),
- * ... other errxxx() fields as needed ...));
+ * errcode(ERRCODE_UNDEFINED_CURSOR),
+ * errmsg("portal \"%s\" not found", stmt->portalname),
+ * ... other errxxx() fields as needed ...);
*
* The error level is required, and so is a primary error message (errmsg
* or errmsg_internal). All else is optional. errcode() defaults to
@@ -101,6 +101,9 @@
* if elevel is WARNING, or ERRCODE_SUCCESSFUL_COMPLETION if elevel is
* NOTICE or below.
*
+ * Before v12, extra parentheses were required around the support routine
+ * calls; that's now optional.
+ *
* ereport_domain() allows a message domain to be specified, for modules that
* wish to use a different message catalog from the backend's. To avoid having
* one copy of the default text domain per .o file, we define it as NULL here
@@ -118,34 +121,34 @@
*----------
*/
#ifdef HAVE__BUILTIN_CONSTANT_P
-#define ereport_domain(elevel, domain, rest) \
+#define ereport_domain(elevel, domain, ...) \
do { \
pg_prevent_errno_in_scope(); \
if (errstart(elevel, __FILE__, __LINE__, PG_FUNCNAME_MACRO, domain)) \
- errfinish rest; \
+ __VA_ARGS__, errfinish(); \
if (__builtin_constant_p(elevel) && (elevel) >= ERROR) \
pg_unreachable(); \
} while(0)
#else /* !HAVE__BUILTIN_CONSTANT_P */
-#define ereport_domain(elevel, domain, rest) \
+#define ereport_domain(elevel, domain, ...) \
do { \
const int elevel_ = (elevel); \
pg_prevent_errno_in_scope(); \
if (errstart(elevel_, __FILE__, __LINE__, PG_FUNCNAME_MACRO, domain)) \
- errfinish rest; \
+ __VA_ARGS__, errfinish(); \
if (elevel_ >= ERROR) \
pg_unreachable(); \
} while(0)
#endif /* HAVE__BUILTIN_CONSTANT_P */
-#define ereport(elevel, rest) \
- ereport_domain(elevel, TEXTDOMAIN, rest)
+#define ereport(elevel, ...) \
+ ereport_domain(elevel, TEXTDOMAIN, __VA_ARGS__)
#define TEXTDOMAIN NULL
extern bool errstart(int elevel, const char *filename, int lineno,
const char *funcname, const char *domain);
-extern void errfinish(int dummy,...);
+extern void errfinish(void);
extern int errcode(int sqlerrcode);
diff --git a/src/backend/executor/execUtils.c b/src/backend/executor/execUtils.c
index cc5177c..6deef1c 100644
--- a/src/backend/executor/execUtils.c
+++ b/src/backend/executor/execUtils.c
@@ -832,21 +832,21 @@ UpdateChangedParamSet(PlanState *node, Bitmapset *newchg)
* normal non-error case: computing character indexes would be much more
* expensive than storing token offsets.)
*/
-int
+void
executor_errposition(EState *estate, int location)
{
int pos;
/* No-op if location was not provided */
if (location < 0)
- return 0;
+ return;
/* Can't do anything if source text is not available */
if (estate == NULL || estate->es_sourceText == NULL)
- return 0;
+ return;
/* Convert offset to character number */
pos = pg_mbstrlen_with_len(estate->es_sourceText, location) + 1;
/* And pass it to the ereport mechanism */
- return errposition(pos);
+ errposition(pos);
}
/*
diff --git a/src/backend/parser/parse_coerce.c b/src/backend/parser/parse_coerce.c
index 645e4aa..91d4e99 100644
--- a/src/backend/parser/parse_coerce.c
+++ b/src/backend/parser/parse_coerce.c
@@ -1246,15 +1246,15 @@ coerce_to_specific_type(ParseState *pstate, Node *node,
* XXX possibly this is more generally useful than coercion errors;
* if so, should rename and place with parser_errposition.
*/
-int
+void
parser_coercion_errposition(ParseState *pstate,
int coerce_location,
Node *input_expr)
{
if (coerce_location >= 0)
- return parser_errposition(pstate, coerce_location);
+ parser_errposition(pstate, coerce_location);
else
- return parser_errposition(pstate, exprLocation(input_expr));
+ parser_errposition(pstate, exprLocation(input_expr));
}
diff --git a/src/backend/parser/parse_node.c b/src/backend/parser/parse_node.c
index 6e98fe5..9a2fd92 100644
--- a/src/backend/parser/parse_node.c
+++ b/src/backend/parser/parse_node.c
@@ -106,21 +106,21 @@ free_parsestate(ParseState *pstate)
* normal non-error case: computing character indexes would be much more
* expensive than storing token offsets.)
*/
-int
+void
parser_errposition(ParseState *pstate, int location)
{
int pos;
/* No-op if location was not provided */
if (location < 0)
- return 0;
+ return;
/* Can't do anything if source text is not available */
if (pstate == NULL || pstate->p_sourcetext == NULL)
- return 0;
+ return;
/* Convert offset to character number */
pos = pg_mbstrlen_with_len(pstate->p_sourcetext, location) + 1;
/* And pass it to the ereport mechanism */
- return errposition(pos);
+ errposition(pos);
}
diff --git a/src/backend/parser/scan.l b/src/backend/parser/scan.l
index b1ea0cb..50ba68a 100644
--- a/src/backend/parser/scan.l
+++ b/src/backend/parser/scan.l
@@ -1076,18 +1076,18 @@ other .
* (essentially, scan.l, parser.c, and gram.y), since it requires the
* yyscanner struct to still be available.
*/
-int
+void
scanner_errposition(int location, core_yyscan_t yyscanner)
{
int pos;
if (location < 0)
- return 0; /* no-op if location is unknown */
+ return; /* no-op if location is unknown */
/* Convert byte offset to character number */
pos = pg_mbstrlen_with_len(yyextra->scanbuf, location) + 1;
/* And pass it to the ereport mechanism */
- return errposition(pos);
+ errposition(pos);
}
/*
diff --git a/src/backend/storage/ipc/dsm_impl.c b/src/backend/storage/ipc/dsm_impl.c
index 1972aec..8dc9c0b 100644
--- a/src/backend/storage/ipc/dsm_impl.c
+++ b/src/backend/storage/ipc/dsm_impl.c
@@ -92,7 +92,7 @@ static bool dsm_impl_mmap(dsm_op op, dsm_handle handle, Size request_size,
void **impl_private, void **mapped_address,
Size *mapped_size, int elevel);
#endif
-static int errcode_for_dynamic_shared_memory(void);
+static void errcode_for_dynamic_shared_memory(void);
const struct config_enum_entry dynamic_shared_memory_options[] = {
#ifdef USE_DSM_POSIX
@@ -1030,11 +1030,11 @@ dsm_impl_unpin_segment(dsm_handle handle, void **impl_private)
}
}
-static int
+static void
errcode_for_dynamic_shared_memory(void)
{
if (errno == EFBIG || errno == ENOMEM)
- return errcode(ERRCODE_OUT_OF_MEMORY);
+ errcode(ERRCODE_OUT_OF_MEMORY);
else
- return errcode_for_file_access();
+ errcode_for_file_access();
}
diff --git a/src/backend/utils/adt/jsonfuncs.c b/src/backend/utils/adt/jsonfuncs.c
index 4b5007e..321ab9a 100644
--- a/src/backend/utils/adt/jsonfuncs.c
+++ b/src/backend/utils/adt/jsonfuncs.c
@@ -329,7 +329,7 @@ typedef struct JsObject
hash_destroy((jso)->val.json_hash); \
} while (0)
-static int report_json_context(JsonLexContext *lex);
+static void report_json_context(JsonLexContext *lex);
/* semantic action functions for json_object_keys */
static void okeys_object_field_start(void *state, char *fname, bool isnull);
@@ -631,7 +631,7 @@ json_ereport_error(JsonParseErrorType error, JsonLexContext *lex)
* The return value isn't meaningful, but we make it non-void so that this
* can be invoked inside ereport().
*/
-static int
+static void
report_json_context(JsonLexContext *lex)
{
const char *context_start;
@@ -689,8 +689,8 @@ report_json_context(JsonLexContext *lex)
prefix = (context_start > line_start) ? "..." : "";
suffix = (lex->token_type != JSON_TOKEN_END && context_end - lex->input < lex->input_length && *context_end !=
'\n'&& *context_end != '\r') ? "..." : "";
- return errcontext("JSON data, line %d: %s%s%s",
- line_number, prefix, ctxt, suffix);
+ errcontext("JSON data, line %d: %s%s%s",
+ line_number, prefix, ctxt, suffix);
}
diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c
index a29ccd9..6ac2152 100644
--- a/src/backend/utils/error/elog.c
+++ b/src/backend/utils/error/elog.c
@@ -223,17 +223,15 @@ err_gettext(const char *str)
/*
* errstart --- begin an error-reporting cycle
*
- * Create a stack entry and store the given parameters in it. Subsequently,
- * errmsg() and perhaps other routines will be called to further populate
- * the stack entry. Finally, errfinish() will be called to actually process
- * the error report.
+ * Create and initialize error stack entry. Subsequently, errmsg() and
+ * perhaps other routines will be called to further populate the stack entry.
+ * Finally, errfinish() will be called to actually process the error report.
*
* Returns true in normal case. Returns false to short-circuit the error
* report (if it's a warning or lower and not to be reported anywhere).
*/
bool
-errstart(int elevel, const char *filename, int lineno,
- const char *funcname, const char *domain)
+errstart(int elevel, const char *domain)
{
ErrorData *edata;
bool output_to_server;
@@ -321,8 +319,7 @@ errstart(int elevel, const char *filename, int lineno,
if (ErrorContext == NULL)
{
/* Oops, hard crash time; very little we can do safely here */
- write_stderr("error occurred at %s:%d before error message processing is available\n",
- filename ? filename : "(unknown file)", lineno);
+ write_stderr("error occurred before error message processing is available\n");
exit(2);
}
@@ -368,18 +365,6 @@ errstart(int elevel, const char *filename, int lineno,
edata->elevel = elevel;
edata->output_to_server = output_to_server;
edata->output_to_client = output_to_client;
- if (filename)
- {
- const char *slash;
-
- /* keep only base name, useful especially for vpath builds */
- slash = strrchr(filename, '/');
- if (slash)
- filename = slash + 1;
- }
- edata->filename = filename;
- edata->lineno = lineno;
- edata->funcname = funcname;
/* the default text domain is the backend's */
edata->domain = domain ? domain : PG_TEXTDOMAIN("postgres");
/* initialize context_domain the same way (see set_errcontext_domain()) */
@@ -434,11 +419,11 @@ matches_backtrace_functions(const char *funcname)
*
* Produce the appropriate error report(s) and pop the error stack.
*
- * If elevel is ERROR or worse, control does not return to the caller.
- * See elog.h for the error level definitions.
+ * If elevel, as passed to errstart(), is ERROR or worse, control does not
+ * return to the caller. See elog.h for the error level definitions.
*/
void
-errfinish(void)
+errfinish(const char *filename, int lineno, const char *funcname)
{
ErrorData *edata = &errordata[errordata_stack_depth];
int elevel;
@@ -447,6 +432,22 @@ errfinish(void)
recursion_depth++;
CHECK_STACK_DEPTH();
+
+ /* Save the last few bits of error state into the stack entry */
+ if (filename)
+ {
+ const char *slash;
+
+ /* keep only base name, useful especially for vpath builds */
+ slash = strrchr(filename, '/');
+ if (slash)
+ filename = slash + 1;
+ }
+
+ edata->filename = filename;
+ edata->lineno = lineno;
+ edata->funcname = funcname;
+
elevel = edata->elevel;
/*
@@ -605,7 +606,7 @@ errfinish(void)
*
* The code is expected to be represented as per MAKE_SQLSTATE().
*/
-int
+void
errcode(int sqlerrcode)
{
ErrorData *edata = &errordata[errordata_stack_depth];
@@ -614,8 +615,6 @@ errcode(int sqlerrcode)
CHECK_STACK_DEPTH();
edata->sqlerrcode = sqlerrcode;
-
- return 0; /* return value does not matter */
}
@@ -628,7 +627,7 @@ errcode(int sqlerrcode)
* NOTE: the primary error message string should generally include %m
* when this is used.
*/
-int
+void
errcode_for_file_access(void)
{
ErrorData *edata = &errordata[errordata_stack_depth];
@@ -686,8 +685,6 @@ errcode_for_file_access(void)
edata->sqlerrcode = ERRCODE_INTERNAL_ERROR;
break;
}
-
- return 0; /* return value does not matter */
}
/*
@@ -699,7 +696,7 @@ errcode_for_file_access(void)
* NOTE: the primary error message string should generally include %m
* when this is used.
*/
-int
+void
errcode_for_socket_access(void)
{
ErrorData *edata = &errordata[errordata_stack_depth];
@@ -722,8 +719,6 @@ errcode_for_socket_access(void)
edata->sqlerrcode = ERRCODE_INTERNAL_ERROR;
break;
}
-
- return 0; /* return value does not matter */
}
@@ -819,7 +814,7 @@ errcode_for_socket_access(void)
* Note: no newline is needed at the end of the fmt string, since
* ereport will provide one for the output methods that need it.
*/
-int
+void
errmsg(const char *fmt,...)
{
ErrorData *edata = &errordata[errordata_stack_depth];
@@ -834,14 +829,13 @@ errmsg(const char *fmt,...)
MemoryContextSwitchTo(oldcontext);
recursion_depth--;
- return 0; /* return value does not matter */
}
/*
* Add a backtrace to the containing ereport() call. This is intended to be
* added temporarily during debugging.
*/
-int
+void
errbacktrace(void)
{
ErrorData *edata = &errordata[errordata_stack_depth];
@@ -855,8 +849,6 @@ errbacktrace(void)
MemoryContextSwitchTo(oldcontext);
recursion_depth--;
-
- return 0;
}
/*
@@ -906,7 +898,7 @@ set_backtrace(ErrorData *edata, int num_skip)
* the message because the translation would fail and result in infinite
* error recursion.
*/
-int
+void
errmsg_internal(const char *fmt,...)
{
ErrorData *edata = &errordata[errordata_stack_depth];
@@ -921,7 +913,6 @@ errmsg_internal(const char *fmt,...)
MemoryContextSwitchTo(oldcontext);
recursion_depth--;
- return 0; /* return value does not matter */
}
@@ -929,7 +920,7 @@ errmsg_internal(const char *fmt,...)
* errmsg_plural --- add a primary error message text to the current error,
* with support for pluralization of the message text
*/
-int
+void
errmsg_plural(const char *fmt_singular, const char *fmt_plural,
unsigned long n,...)
{
@@ -945,14 +936,13 @@ errmsg_plural(const char *fmt_singular, const char *fmt_plural,
MemoryContextSwitchTo(oldcontext);
recursion_depth--;
- return 0; /* return value does not matter */
}
/*
* errdetail --- add a detail error message text to the current error
*/
-int
+void
errdetail(const char *fmt,...)
{
ErrorData *edata = &errordata[errordata_stack_depth];
@@ -966,7 +956,6 @@ errdetail(const char *fmt,...)
MemoryContextSwitchTo(oldcontext);
recursion_depth--;
- return 0; /* return value does not matter */
}
@@ -979,7 +968,7 @@ errdetail(const char *fmt,...)
* messages that seem not worth translating for one reason or another
* (typically, that they don't seem to be useful to average users).
*/
-int
+void
errdetail_internal(const char *fmt,...)
{
ErrorData *edata = &errordata[errordata_stack_depth];
@@ -993,14 +982,13 @@ errdetail_internal(const char *fmt,...)
MemoryContextSwitchTo(oldcontext);
recursion_depth--;
- return 0; /* return value does not matter */
}
/*
* errdetail_log --- add a detail_log error message text to the current error
*/
-int
+void
errdetail_log(const char *fmt,...)
{
ErrorData *edata = &errordata[errordata_stack_depth];
@@ -1014,14 +1002,13 @@ errdetail_log(const char *fmt,...)
MemoryContextSwitchTo(oldcontext);
recursion_depth--;
- return 0; /* return value does not matter */
}
/*
* errdetail_log_plural --- add a detail_log error message text to the current error
* with support for pluralization of the message text
*/
-int
+void
errdetail_log_plural(const char *fmt_singular, const char *fmt_plural,
unsigned long n,...)
{
@@ -1036,7 +1023,6 @@ errdetail_log_plural(const char *fmt_singular, const char *fmt_plural,
MemoryContextSwitchTo(oldcontext);
recursion_depth--;
- return 0; /* return value does not matter */
}
@@ -1044,7 +1030,7 @@ errdetail_log_plural(const char *fmt_singular, const char *fmt_plural,
* errdetail_plural --- add a detail error message text to the current error,
* with support for pluralization of the message text
*/
-int
+void
errdetail_plural(const char *fmt_singular, const char *fmt_plural,
unsigned long n,...)
{
@@ -1059,14 +1045,13 @@ errdetail_plural(const char *fmt_singular, const char *fmt_plural,
MemoryContextSwitchTo(oldcontext);
recursion_depth--;
- return 0; /* return value does not matter */
}
/*
* errhint --- add a hint error message text to the current error
*/
-int
+void
errhint(const char *fmt,...)
{
ErrorData *edata = &errordata[errordata_stack_depth];
@@ -1080,7 +1065,6 @@ errhint(const char *fmt,...)
MemoryContextSwitchTo(oldcontext);
recursion_depth--;
- return 0; /* return value does not matter */
}
@@ -1091,7 +1075,7 @@ errhint(const char *fmt,...)
* context information. We assume earlier calls represent more-closely-nested
* states.
*/
-int
+void
errcontext_msg(const char *fmt,...)
{
ErrorData *edata = &errordata[errordata_stack_depth];
@@ -1105,7 +1089,6 @@ errcontext_msg(const char *fmt,...)
MemoryContextSwitchTo(oldcontext);
recursion_depth--;
- return 0; /* return value does not matter */
}
/*
@@ -1116,18 +1099,8 @@ errcontext_msg(const char *fmt,...)
* translate it. Instead, each errcontext_msg() call should be preceded by
* a set_errcontext_domain() call to specify the domain. This is usually
* done transparently by the errcontext() macro.
- *
- * Although errcontext is primarily meant for use at call sites distant from
- * the original ereport call, there are a few places that invoke errcontext
- * within ereport. The expansion of errcontext as a comma expression calling
- * set_errcontext_domain then errcontext_msg is problematic in this case,
- * because the intended comma expression becomes two arguments to errfinish,
- * which the compiler is at liberty to evaluate in either order. But in
- * such a case, the set_errcontext_domain calls must be selecting the same
- * TEXTDOMAIN value that the errstart call did, so order does not matter
- * so long as errstart initializes context_domain along with domain.
*/
-int
+void
set_errcontext_domain(const char *domain)
{
ErrorData *edata = &errordata[errordata_stack_depth];
@@ -1137,8 +1110,6 @@ set_errcontext_domain(const char *domain)
/* the default text domain is the backend's */
edata->context_domain = domain ? domain : PG_TEXTDOMAIN("postgres");
-
- return 0; /* return value does not matter */
}
@@ -1147,7 +1118,7 @@ set_errcontext_domain(const char *domain)
*
* This should be called if the message text already includes the statement.
*/
-int
+void
errhidestmt(bool hide_stmt)
{
ErrorData *edata = &errordata[errordata_stack_depth];
@@ -1156,8 +1127,6 @@ errhidestmt(bool hide_stmt)
CHECK_STACK_DEPTH();
edata->hide_stmt = hide_stmt;
-
- return 0; /* return value does not matter */
}
/*
@@ -1166,7 +1135,7 @@ errhidestmt(bool hide_stmt)
* This should only be used for verbose debugging messages where the repeated
* inclusion of context would bloat the log volume too much.
*/
-int
+void
errhidecontext(bool hide_ctx)
{
ErrorData *edata = &errordata[errordata_stack_depth];
@@ -1175,8 +1144,6 @@ errhidecontext(bool hide_ctx)
CHECK_STACK_DEPTH();
edata->hide_ctx = hide_ctx;
-
- return 0; /* return value does not matter */
}
@@ -1187,7 +1154,7 @@ errhidecontext(bool hide_ctx)
* name appear in messages sent to old-protocol clients. Note that the
* passed string is expected to be a non-freeable constant string.
*/
-int
+void
errfunction(const char *funcname)
{
ErrorData *edata = &errordata[errordata_stack_depth];
@@ -1197,14 +1164,12 @@ errfunction(const char *funcname)
edata->funcname = funcname;
edata->show_funcname = true;
-
- return 0; /* return value does not matter */
}
/*
* errposition --- add cursor position to the current error
*/
-int
+void
errposition(int cursorpos)
{
ErrorData *edata = &errordata[errordata_stack_depth];
@@ -1213,14 +1178,12 @@ errposition(int cursorpos)
CHECK_STACK_DEPTH();
edata->cursorpos = cursorpos;
-
- return 0; /* return value does not matter */
}
/*
* internalerrposition --- add internal cursor position to the current error
*/
-int
+void
internalerrposition(int cursorpos)
{
ErrorData *edata = &errordata[errordata_stack_depth];
@@ -1229,8 +1192,6 @@ internalerrposition(int cursorpos)
CHECK_STACK_DEPTH();
edata->internalpos = cursorpos;
-
- return 0; /* return value does not matter */
}
/*
@@ -1240,7 +1201,7 @@ internalerrposition(int cursorpos)
* is intended for use in error callback subroutines that are editorializing
* on the layout of the error report.
*/
-int
+void
internalerrquery(const char *query)
{
ErrorData *edata = &errordata[errordata_stack_depth];
@@ -1256,8 +1217,6 @@ internalerrquery(const char *query)
if (query)
edata->internalquery = MemoryContextStrdup(edata->assoc_context, query);
-
- return 0; /* return value does not matter */
}
/*
@@ -1270,7 +1229,7 @@ internalerrquery(const char *query)
* Most potential callers should not use this directly, but instead prefer
* higher-level abstractions, such as errtablecol() (see relcache.c).
*/
-int
+void
err_generic_string(int field, const char *str)
{
ErrorData *edata = &errordata[errordata_stack_depth];
@@ -1299,8 +1258,6 @@ err_generic_string(int field, const char *str)
elog(ERROR, "unsupported ErrorData field id: %d", field);
break;
}
-
- return 0; /* return value does not matter */
}
/*
@@ -1366,108 +1323,6 @@ getinternalerrposition(void)
/*
- * elog_start --- startup for old-style API
- *
- * All that we do here is stash the hidden filename/lineno/funcname
- * arguments into a stack entry, along with the current value of errno.
- *
- * We need this to be separate from elog_finish because there's no other
- * C89-compliant way to deal with inserting extra arguments into the elog
- * call. (When using C99's __VA_ARGS__, we could possibly merge this with
- * elog_finish, but there doesn't seem to be a good way to save errno before
- * evaluating the format arguments if we do that.)
- */
-void
-elog_start(const char *filename, int lineno, const char *funcname)
-{
- ErrorData *edata;
-
- /* Make sure that memory context initialization has finished */
- if (ErrorContext == NULL)
- {
- /* Oops, hard crash time; very little we can do safely here */
- write_stderr("error occurred at %s:%d before error message processing is available\n",
- filename ? filename : "(unknown file)", lineno);
- exit(2);
- }
-
- if (++errordata_stack_depth >= ERRORDATA_STACK_SIZE)
- {
- /*
- * Wups, stack not big enough. We treat this as a PANIC condition
- * because it suggests an infinite loop of errors during error
- * recovery. Note that the message is intentionally not localized,
- * else failure to convert it to client encoding could cause further
- * recursion.
- */
- errordata_stack_depth = -1; /* make room on stack */
- ereport(PANIC, (errmsg_internal("ERRORDATA_STACK_SIZE exceeded")));
- }
-
- edata = &errordata[errordata_stack_depth];
- if (filename)
- {
- const char *slash;
-
- /* keep only base name, useful especially for vpath builds */
- slash = strrchr(filename, '/');
- if (slash)
- filename = slash + 1;
- }
- edata->filename = filename;
- edata->lineno = lineno;
- edata->funcname = funcname;
- /* errno is saved now so that error parameter eval can't change it */
- edata->saved_errno = errno;
-
- /* Use ErrorContext for any allocations done at this level. */
- edata->assoc_context = ErrorContext;
-}
-
-/*
- * elog_finish --- finish up for old-style API
- */
-void
-elog_finish(int elevel, const char *fmt,...)
-{
- ErrorData *edata = &errordata[errordata_stack_depth];
- MemoryContext oldcontext;
-
- CHECK_STACK_DEPTH();
-
- /*
- * Do errstart() to see if we actually want to report the message.
- */
- errordata_stack_depth--;
- errno = edata->saved_errno;
- if (!errstart(elevel, edata->filename, edata->lineno, edata->funcname, NULL))
- return; /* nothing to do */
-
- /*
- * Format error message just like errmsg_internal().
- */
- recursion_depth++;
- oldcontext = MemoryContextSwitchTo(edata->assoc_context);
-
- if (!edata->backtrace &&
- edata->funcname &&
- matches_backtrace_functions(edata->funcname))
- set_backtrace(edata, 2);
-
- edata->message_id = fmt;
- EVALUATE_MESSAGE(edata->domain, message, false, false);
-
- MemoryContextSwitchTo(oldcontext);
- recursion_depth--;
-
- /*
- * And let errfinish() finish up.
- */
- errfinish();
-}
-
-
-/*
* Functions to allow construction of error message strings separately from
* the ereport() call itself.
*
@@ -1706,8 +1561,7 @@ ThrowErrorData(ErrorData *edata)
ErrorData *newedata;
MemoryContext oldcontext;
- if (!errstart(edata->elevel, edata->filename, edata->lineno,
- edata->funcname, NULL))
+ if (!errstart(edata->elevel, edata->domain))
return; /* error is not to be reported at all */
newedata = &errordata[errordata_stack_depth];
@@ -1749,7 +1603,7 @@ ThrowErrorData(ErrorData *edata)
recursion_depth--;
/* Process the error. */
- errfinish();
+ errfinish(edata->filename, edata->lineno, edata->funcname);
}
/*
@@ -1863,7 +1717,7 @@ pg_re_throw(void)
*/
error_context_stack = NULL;
- errfinish();
+ errfinish(edata->filename, edata->lineno, edata->funcname);
}
/* Doesn't return ... */
diff --git a/src/include/executor/executor.h b/src/include/executor/executor.h
index 9489051..cd0e643 100644
--- a/src/include/executor/executor.h
+++ b/src/include/executor/executor.h
@@ -546,7 +546,7 @@ exec_rt_fetch(Index rti, EState *estate)
extern Relation ExecGetRangeTableRelation(EState *estate, Index rti);
-extern int executor_errposition(EState *estate, int location);
+extern void executor_errposition(EState *estate, int location);
extern void RegisterExprContextCallback(ExprContext *econtext,
ExprContextCallbackFunction function,
diff --git a/src/include/parser/parse_coerce.h b/src/include/parser/parse_coerce.h
index 8686eaa..a3295b3 100644
--- a/src/include/parser/parse_coerce.h
+++ b/src/include/parser/parse_coerce.h
@@ -61,7 +61,7 @@ extern Node *coerce_to_specific_type_typmod(ParseState *pstate, Node *node,
Oid targetTypeId, int32 targetTypmod,
const char *constructName);
-extern int parser_coercion_errposition(ParseState *pstate,
+extern void parser_coercion_errposition(ParseState *pstate,
int coerce_location,
Node *input_expr);
diff --git a/src/include/parser/parse_node.h b/src/include/parser/parse_node.h
index d25819a..b223b59 100644
--- a/src/include/parser/parse_node.h
+++ b/src/include/parser/parse_node.h
@@ -307,7 +307,7 @@ typedef struct ParseCallbackState
extern ParseState *make_parsestate(ParseState *parentParseState);
extern void free_parsestate(ParseState *pstate);
-extern int parser_errposition(ParseState *pstate, int location);
+extern void parser_errposition(ParseState *pstate, int location);
extern void setup_parser_errposition_callback(ParseCallbackState *pcbstate,
ParseState *pstate, int location);
diff --git a/src/include/parser/scanner.h b/src/include/parser/scanner.h
index a27352a..02ae10a 100644
--- a/src/include/parser/scanner.h
+++ b/src/include/parser/scanner.h
@@ -140,7 +140,7 @@ extern core_yyscan_t scanner_init(const char *str,
extern void scanner_finish(core_yyscan_t yyscanner);
extern int core_yylex(core_YYSTYPE *lvalp, YYLTYPE *llocp,
core_yyscan_t yyscanner);
-extern int scanner_errposition(int location, core_yyscan_t yyscanner);
+extern void scanner_errposition(int location, core_yyscan_t yyscanner);
extern void setup_scanner_errposition_callback(ScannerCallbackState *scbstate,
core_yyscan_t yyscanner,
int location);
diff --git a/src/include/utils/elog.h b/src/include/utils/elog.h
index 73d84d1..8e5fcf7 100644
--- a/src/include/utils/elog.h
+++ b/src/include/utils/elog.h
@@ -124,8 +124,8 @@
#define ereport_domain(elevel, domain, ...) \
do { \
pg_prevent_errno_in_scope(); \
- if (errstart(elevel, __FILE__, __LINE__, PG_FUNCNAME_MACRO, domain)) \
- __VA_ARGS__, errfinish(); \
+ if (errstart(elevel, domain)) \
+ __VA_ARGS__, errfinish(__FILE__, __LINE__, PG_FUNCNAME_MACRO); \
if (__builtin_constant_p(elevel) && (elevel) >= ERROR) \
pg_unreachable(); \
} while(0)
@@ -134,8 +134,8 @@
do { \
const int elevel_ = (elevel); \
pg_prevent_errno_in_scope(); \
- if (errstart(elevel_, __FILE__, __LINE__, PG_FUNCNAME_MACRO, domain)) \
- __VA_ARGS__, errfinish(); \
+ if (errstart(elevel_, domain)) \
+ __VA_ARGS__, errfinish(__FILE__, __LINE__, PG_FUNCNAME_MACRO); \
if (elevel_ >= ERROR) \
pg_unreachable(); \
} while(0)
@@ -146,34 +146,33 @@
#define TEXTDOMAIN NULL
-extern bool errstart(int elevel, const char *filename, int lineno,
- const char *funcname, const char *domain);
-extern void errfinish(void);
+extern bool errstart(int elevel, const char *domain);
+extern void errfinish(const char *filename, int lineno, const char *funcname);
-extern int errcode(int sqlerrcode);
+extern void errcode(int sqlerrcode);
-extern int errcode_for_file_access(void);
-extern int errcode_for_socket_access(void);
+extern void errcode_for_file_access(void);
+extern void errcode_for_socket_access(void);
-extern int errmsg(const char *fmt,...) pg_attribute_printf(1, 2);
-extern int errmsg_internal(const char *fmt,...) pg_attribute_printf(1, 2);
+extern void errmsg(const char *fmt,...) pg_attribute_printf(1, 2);
+extern void errmsg_internal(const char *fmt,...) pg_attribute_printf(1, 2);
-extern int errmsg_plural(const char *fmt_singular, const char *fmt_plural,
+extern void errmsg_plural(const char *fmt_singular, const char *fmt_plural,
unsigned long n,...) pg_attribute_printf(1, 4) pg_attribute_printf(2, 4);
-extern int errdetail(const char *fmt,...) pg_attribute_printf(1, 2);
-extern int errdetail_internal(const char *fmt,...) pg_attribute_printf(1, 2);
+extern void errdetail(const char *fmt,...) pg_attribute_printf(1, 2);
+extern void errdetail_internal(const char *fmt,...) pg_attribute_printf(1, 2);
-extern int errdetail_log(const char *fmt,...) pg_attribute_printf(1, 2);
+extern void errdetail_log(const char *fmt,...) pg_attribute_printf(1, 2);
-extern int errdetail_log_plural(const char *fmt_singular,
+extern void errdetail_log_plural(const char *fmt_singular,
const char *fmt_plural,
unsigned long n,...) pg_attribute_printf(1, 4) pg_attribute_printf(2, 4);
-extern int errdetail_plural(const char *fmt_singular, const char *fmt_plural,
+extern void errdetail_plural(const char *fmt_singular, const char *fmt_plural,
unsigned long n,...) pg_attribute_printf(1, 4) pg_attribute_printf(2, 4);
-extern int errhint(const char *fmt,...) pg_attribute_printf(1, 2);
+extern void errhint(const char *fmt,...) pg_attribute_printf(1, 2);
/*
* errcontext() is typically called in error context callback functions, not
@@ -185,22 +184,22 @@ extern int errhint(const char *fmt,...) pg_attribute_printf(1, 2);
*/
#define errcontext set_errcontext_domain(TEXTDOMAIN), errcontext_msg
-extern int set_errcontext_domain(const char *domain);
+extern void set_errcontext_domain(const char *domain);
-extern int errcontext_msg(const char *fmt,...) pg_attribute_printf(1, 2);
+extern void errcontext_msg(const char *fmt,...) pg_attribute_printf(1, 2);
-extern int errhidestmt(bool hide_stmt);
-extern int errhidecontext(bool hide_ctx);
+extern void errhidestmt(bool hide_stmt);
+extern void errhidecontext(bool hide_ctx);
-extern int errbacktrace(void);
+extern void errbacktrace(void);
-extern int errfunction(const char *funcname);
-extern int errposition(int cursorpos);
+extern void errfunction(const char *funcname);
+extern void errposition(int cursorpos);
-extern int internalerrposition(int cursorpos);
-extern int internalerrquery(const char *query);
+extern void internalerrposition(int cursorpos);
+extern void internalerrquery(const char *query);
-extern int err_generic_string(int field, const char *str);
+extern void err_generic_string(int field, const char *str);
extern int geterrcode(void);
extern int geterrposition(void);
@@ -212,37 +211,8 @@ extern int getinternalerrposition(void);
* elog(ERROR, "portal \"%s\" not found", stmt->portalname);
*----------
*/
-/*
- * Using variadic macros, we can give the compiler a hint about the
- * call not returning when elevel >= ERROR. See comments for ereport().
- * Note that historically elog() has called elog_start (which saves errno)
- * before evaluating "elevel", so we preserve that behavior here.
- */
-#ifdef HAVE__BUILTIN_CONSTANT_P
#define elog(elevel, ...) \
- do { \
- pg_prevent_errno_in_scope(); \
- elog_start(__FILE__, __LINE__, PG_FUNCNAME_MACRO); \
- elog_finish(elevel, __VA_ARGS__); \
- if (__builtin_constant_p(elevel) && (elevel) >= ERROR) \
- pg_unreachable(); \
- } while(0)
-#else /* !HAVE__BUILTIN_CONSTANT_P */
-#define elog(elevel, ...) \
- do { \
- pg_prevent_errno_in_scope(); \
- elog_start(__FILE__, __LINE__, PG_FUNCNAME_MACRO); \
- { \
- const int elevel_ = (elevel); \
- elog_finish(elevel_, __VA_ARGS__); \
- if (elevel_ >= ERROR) \
- pg_unreachable(); \
- } \
- } while(0)
-#endif /* HAVE__BUILTIN_CONSTANT_P */
-
-extern void elog_start(const char *filename, int lineno, const char *funcname);
-extern void elog_finish(int elevel, const char *fmt,...) pg_attribute_printf(2, 3);
+ ereport(elevel, errmsg_internal(__VA_ARGS__))
/* Support for constructing error strings separately from ereport() calls */
diff --git a/src/pl/plpgsql/src/pl_scanner.c b/src/pl/plpgsql/src/pl_scanner.c
index 9cea2e4..9d81679 100644
--- a/src/pl/plpgsql/src/pl_scanner.c
+++ b/src/pl/plpgsql/src/pl_scanner.c
@@ -468,20 +468,20 @@ plpgsql_peek2(int *tok1_p, int *tok2_p, int *tok1_loc, int *tok2_loc)
* parsing of a plpgsql function, since it requires the scanorig string
* to still be available.
*/
-int
+void
plpgsql_scanner_errposition(int location)
{
int pos;
if (location < 0 || scanorig == NULL)
- return 0; /* no-op if location is unknown */
+ return; /* no-op if location is unknown */
/* Convert byte offset to character number */
pos = pg_mbstrlen_with_len(scanorig, location) + 1;
/* And pass it to the ereport mechanism */
(void) internalerrposition(pos);
/* Also pass the function body string */
- return internalerrquery(scanorig);
+ internalerrquery(scanorig);
}
/*
diff --git a/src/pl/plpgsql/src/plpgsql.h b/src/pl/plpgsql/src/plpgsql.h
index 69df330..325f4f7 100644
--- a/src/pl/plpgsql/src/plpgsql.h
+++ b/src/pl/plpgsql/src/plpgsql.h
@@ -1319,7 +1319,7 @@ extern void plpgsql_append_source_text(StringInfo buf,
extern int plpgsql_peek(void);
extern void plpgsql_peek2(int *tok1_p, int *tok2_p, int *tok1_loc,
int *tok2_loc);
-extern int plpgsql_scanner_errposition(int location);
+extern void plpgsql_scanner_errposition(int location);
extern void plpgsql_yyerror(const char *message) pg_attribute_noreturn();
extern int plpgsql_location_to_lineno(int location);
extern int plpgsql_latest_lineno(void);
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
Andres Freund <andres@anarazel.de> writes:
> 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.
Yeah, I don't really agree with that idea. The whole business of which
elevels will trigger output is fundamentally policy, not mechanism,
and you do not want policy decisions embedded into ABI so that there
are hundreds of copies of them. It's a loss for code size and a worse
loss if you ever want to change the behavior.
> On 2020-03-23 17:24:49 -0400, Tom Lane wrote:
>> 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").
Yeah, good point. Let's skinny the v12 patch down to just macro
and docs changes, then, not touching elog.c at all.
I made a commitfest entry for this just to see what the cfbot
thinks of it, but if there are not objections we should go ahead
and push in a day or so, IMO.
regards, tom lane
BTW, the CF app is showing me as the first author, even though
I definitely put you in first. Guess it doesn't care about
author ordering ... is that a bug to be fixed?
On 2020-03-23 17:24:49 -0400, Tom Lane wrote: > Hearing no objections, I started to review Andres' patchset with > that plan in mind. Thanks for pushing the first part!
Andres Freund <andres@anarazel.de> writes:
> On 2020-03-23 17:24:49 -0400, Tom Lane wrote:
>> Hearing no objections, I started to review Andres' patchset with
>> that plan in mind.
> Thanks for pushing the first part!
I pushed all of it, actually.
regards, tom lane
On Wed, Mar 25, 2020 at 9:30 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2020-03-23 17:24:49 -0400, Tom Lane wrote:
> >> Hearing no objections, I started to review Andres' patchset with
> >> that plan in mind.
>
> > Thanks for pushing the first part!
>
> I pushed all of it, actually.
I think this caused anole to say:
"reloptions.c", line 1362: error #2042: operand types are incompatible
("void" and "int")
errdetail_internal("%s", _(optenum->detailmsg)) : 0));
Thomas Munro <thomas.munro@gmail.com> writes:
> I think this caused anole to say:
> "reloptions.c", line 1362: error #2042: operand types are incompatible
> ("void" and "int")
> errdetail_internal("%s", _(optenum->detailmsg)) : 0));
Yeah, I was just looking at that :-(
We could revert the change to have these functions return void,
or we could run around and change the places with this usage
pattern to use "(void) 0" instead of just "0". The latter would
be somewhat painful if only minority compilers warn, though.
Also, I don't think that having to change ereport usage was part
of the agreed-to plan here ... so I'm leaning to the former.
regards, tom lane
I wrote:
> Thomas Munro <thomas.munro@gmail.com> writes:
>> I think this caused anole to say:
>> "reloptions.c", line 1362: error #2042: operand types are incompatible
>> ("void" and "int")
>> errdetail_internal("%s", _(optenum->detailmsg)) : 0));
> Yeah, I was just looking at that :-(
> We could revert the change to have these functions return void,
> or we could run around and change the places with this usage
> pattern to use "(void) 0" instead of just "0". The latter would
> be somewhat painful if only minority compilers warn, though.
> Also, I don't think that having to change ereport usage was part
> of the agreed-to plan here ... so I'm leaning to the former.
Done that way.
regards, tom lane
On Fri, 20 Mar 2020, 01:59 Andres Freund, <andres@anarazel.de> wrote:
Hi,
On 2020-03-17 10:09:18 -0400, Tom Lane wrote:
> We might want to spend some effort thinking how to find or prevent
> additional bugs of the same ilk ...
Yea, that'd be good. Trying to help people new to postgres write their
first patches I found that ereport is very confusing to them - largely
because the syntax doesn't make much sense. Made worse by the compiler
error messages being terrible in many cases.
Very much agreed.
I'd have found it helpful to just have the docs explain clearly how it works by chaining the comma operator using functions with ignored return values.
That would also help people understand how they can make parts of an ereport conditional, e.g. only set errdetail() if there additional info is currently available W/O duplicating the rest of the ereport .
Not sure there's much we can do without changing ereport's "signature"
though :(
Regards,
Andres
Craig Ringer <craig@2ndquadrant.com> writes:
> I'd have found it helpful to just have the docs explain clearly how it
> works by chaining the comma operator using functions with ignored return
> values.
Want to write some text?
> That would also help people understand how they can make parts of an
> ereport conditional, e.g. only set errdetail() if there additional info is
> currently available W/O duplicating the rest of the ereport .
There are examples of that in the tree, of course, but maybe an
example in the docs wouldn't be bad either.
regards, tom lane
Hi, On 2020-03-30 11:54:03 +0800, Craig Ringer wrote: > On Fri, 20 Mar 2020, 01:59 Andres Freund, <andres@anarazel.de> wrote: > > On 2020-03-17 10:09:18 -0400, Tom Lane wrote: > > > We might want to spend some effort thinking how to find or prevent > > > additional bugs of the same ilk ... > > > > Yea, that'd be good. Trying to help people new to postgres write their > > first patches I found that ereport is very confusing to them - largely > > because the syntax doesn't make much sense. Made worse by the compiler > > error messages being terrible in many cases. > > > Very much agreed. > > I'd have found it helpful to just have the docs explain clearly how it > works by chaining the comma operator using functions with ignored return > values. IDK, that seems like it'll be a bit too implementation specific. > That would also help people understand how they can make parts of an > ereport conditional, e.g. only set errdetail() if there additional info is > currently available W/O duplicating the rest of the ereport . Well, they can do whatever they can in any function argument list. I don't see why the ereport docs are a good place to explain ... ? ... : ... ? Or am I misunderstanding what you suggest here? > Not sure there's much we can do without changing ereport's "signature" I think the changes we just did already improved the situation a good bit. Both by obviating the need for the additional parentheses, as well as making parameters outside of err*() trigger warnings, and also generally better error/warning messages. I still think we might want to do a bigger change of the logging APIs. See the messages leading up to https://postgr.es/m/20190805214952.dmnn2oetdquixpp4%40alap3.anarazel.de and the message linked from there. Greetings, Andres Freund