Re-implement the ereport() macro using __VA_ARGS__.

Now that we require C99, we can depend on __VA_ARGS__ to work, and
revising ereport() to use it has several significant benefits:

* The extra parentheses around the auxiliary function calls are now
optional.  Aside from being a bit less ugly, this removes a common
gotcha for new contributors, because in some cases the compiler errors
you got from forgetting them were unintelligible.

* The auxiliary function calls are now evaluated as a comma expression
list rather than as extra arguments to errfinish().  This means that
compilers can be expected to warn about no-op expressions in the list,
allowing detection of several other common mistakes such as forgetting
to add errmsg(...) when converting an elog() call to ereport().

* Unlike the situation with extra function arguments, comma expressions
are guaranteed to be evaluated left-to-right, so this removes platform
dependency in the order of the auxiliary function calls.  While that
dependency hasn't caused us big problems in the past, this change does
allow dropping some rather shaky assumptions around errcontext() domain
handling.

There's no intention to make wholesale changes of existing ereport
calls, but as proof-of-concept this patch removes the extra parens
from a couple of calls in postgres.c.

While new code can be written either way, code intended to be
back-patched will need to use extra parens for awhile yet.  It seems
worth back-patching this change into v12, so as to reduce the window
where we have to be careful about that by one year.  Hence, this patch
is careful to preserve ABI compatibility; a followup HEAD-only patch
will make some additional simplifications.

Andres Freund and Tom Lane

Discussion: https://postgr.es/m/CA+fd4k6N8EjNvZpM8nme+y+05mz-SM8Z_BgkixzkA34R+ej0Kw@mail.gmail.com

4 files changed, 49 insertions, 42 deletions

diff --git a/doc/src/sgml/sources.sgml b/doc/src/sgml/sources.sgml
index 25e7763c7c7..730430c8641 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 &mdash;
-    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 6c2db935736..ee1b4f68570 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -3670,15 +3670,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 8b4720ef3ab..e164f5bfee4 100644
--- a/src/backend/utils/error/elog.c
+++ b/src/backend/utils/error/elog.c
@@ -1020,16 +1020,6 @@ 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
 set_errcontext_domain(const char *domain)
diff --git a/src/include/utils/elog.h b/src/include/utils/elog.h
index dbfd8efd269..cb8d5e91901 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 Postgres v12, extra parentheses were required around the
+ * list of auxiliary function 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,28 +121,28 @@
  *----------
  */
 #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(0); \
 		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(0); \
 		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