From: Tom Lane <tgl@sss.pgh.pa.us>
To: Andres Freund <andres@anarazel.de>
Cc: Amit Kapila <amit.kapila16@gmail.com>
Cc: Michael Paquier <michael@paquier.xyz>
Cc: Julien Rouhaud <rjuju123@gmail.com>
Cc: Masahiko Sawada <masahiko.sawada@2ndquadrant.com>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: Missing errcode() in ereport
Date: Thu, 19 Mar 2020 21:03:17 -0400
Message-ID: <18567.1584666197@sss.pgh.pa.us> (raw)
In-Reply-To: <5995.1584660775@sss.pgh.pa.us>
References: <CA+fd4k6N8EjNvZpM8nme+y+05mz-SM8Z_BgkixzkA34R+ej0Kw@mail.gmail.com>
<CAA4eK1KKmr-Dt-8=C=uoeKk7b6WqOTjjpMt_MXs4eFN1=kAX5w@mail.gmail.com>
<CAOBaU_bb7FR5cF9RKN42kPHf6pk9CeQ=wTuieJv-t3MroU_q0g@mail.gmail.com>
<20200317095839.GG2206@paquier.xyz>
<CAA4eK1+powcFw2EAoVgvH4P3fspAJhxU-VTy0_cR2XpcBZGMAA@mail.gmail.com>
<32174.1584454158@sss.pgh.pa.us>
<20200319175902.vjib2tslosw65iyr@alap3.anarazel.de>
<5063.1584641224@sss.pgh.pa.us>
<20200319230446.lxjybufdcgwaqkhj@alap3.anarazel.de>
<5995.1584660775@sss.pgh.pa.us>
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
Attachments:better-ereport-wip.patch (text/x-diff, 2.3K) downloaddiff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.cindex 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.hindex 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);
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: tgl@sss.pgh.pa.us, andres@anarazel.de, amit.kapila16@gmail.com, michael@paquier.xyz, rjuju123@gmail.com, masahiko.sawada@2ndquadrant.com, pgsql-hackers@lists.postgresql.org
Subject: Re: Missing errcode() in ereport
In-Reply-To: <18567.1584666197@sss.pgh.pa.us>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox