Simplifying our Trap/Assert infrastructure - Mailing list pgsql-hackers

I happened to notice that the Trap and TrapMacro macros defined in c.h
have a grand total of one usage apiece across our entire code base.
It seems a little pointless and confusing to have them at all, since
they're essentially Assert/AssertMacro but with the inverse condition
polarity.  I'm also annoyed that they are documented while the macros
we actually use are not.

I'm also thinking that the "errorType" argument of ExceptionalCondition
is not nearly pulling its weight given the actual usage.  Removing it
reduces the size of an assert-enabled build of HEAD from

$ size src/backend/postgres 
   text    data     bss     dec     hex filename
9065335   86280  204496 9356111  8ec34f src/backend/postgres

to

$ size src/backend/postgres 
   text    data     bss     dec     hex filename
9001199   86280  204496 9291975  8dc8c7 src/backend/postgres

(on RHEL8 x86_64), which admittedly is only about 1%, but it's 1%
for just about no detectable return.

Hence, I propose the attached.

            regards, tom lane

diff --git a/contrib/amcheck/verify_heapam.c b/contrib/amcheck/verify_heapam.c
index d33f33f170..83dc728011 100644
--- a/contrib/amcheck/verify_heapam.c
+++ b/contrib/amcheck/verify_heapam.c
@@ -1319,7 +1319,7 @@ check_tuple_attribute(HeapCheckContext *ctx)
      */

     /*
-     * Check that VARTAG_SIZE won't hit a TrapMacro on a corrupt va_tag before
+     * Check that VARTAG_SIZE won't hit an Assert on a corrupt va_tag before
      * risking a call into att_addlength_pointer
      */
     if (VARATT_IS_EXTERNAL(tp + ctx->offset))
diff --git a/src/backend/utils/error/assert.c b/src/backend/utils/error/assert.c
index 2da512a2f1..ac6173e660 100644
--- a/src/backend/utils/error/assert.c
+++ b/src/backend/utils/error/assert.c
@@ -28,20 +28,17 @@
  */
 void
 ExceptionalCondition(const char *conditionName,
-                     const char *errorType,
                      const char *fileName,
                      int lineNumber)
 {
     /* Report the failure on stderr (or local equivalent) */
     if (!PointerIsValid(conditionName)
-        || !PointerIsValid(fileName)
-        || !PointerIsValid(errorType))
+        || !PointerIsValid(fileName))
         write_stderr("TRAP: ExceptionalCondition: bad arguments in PID %d\n",
                      (int) getpid());
     else
-        write_stderr("TRAP: %s(\"%s\", File: \"%s\", Line: %d, PID: %d)\n",
-                     errorType, conditionName,
-                     fileName, lineNumber, (int) getpid());
+        write_stderr("TRAP: failed Assert(\"%s\"), File: \"%s\", Line: %d, PID: %d\n",
+                     conditionName, fileName, lineNumber, (int) getpid());

     /* Usually this shouldn't be needed, but make sure the msg went out */
     fflush(stderr);
diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c
index eb724a9d7f..6e0a66c29e 100644
--- a/src/backend/utils/error/elog.c
+++ b/src/backend/utils/error/elog.c
@@ -1832,8 +1832,7 @@ pg_re_throw(void)
     }

     /* Doesn't return ... */
-    ExceptionalCondition("pg_re_throw tried to return", "FailedAssertion",
-                         __FILE__, __LINE__);
+    ExceptionalCondition("pg_re_throw tried to return", __FILE__, __LINE__);
 }


diff --git a/src/include/c.h b/src/include/c.h
index c8f72e44d8..0ba58f1f1b 100644
--- a/src/include/c.h
+++ b/src/include/c.h
@@ -796,8 +796,6 @@ typedef NameData *Name;
 #define AssertArg(condition)    ((void)true)
 #define AssertState(condition)    ((void)true)
 #define AssertPointerAlignment(ptr, bndr)    ((void)true)
-#define Trap(condition, errorType)    ((void)true)
-#define TrapMacro(condition, errorType) (true)

 #elif defined(FRONTEND)

@@ -811,60 +809,38 @@ typedef NameData *Name;
 #else                            /* USE_ASSERT_CHECKING && !FRONTEND */

 /*
- * Trap
- *        Generates an exception if the given condition is true.
+ * Assert
+ *        Generates a fatal exception if the given condition is false.
  */
-#define Trap(condition, errorType) \
-    do { \
-        if (condition) \
-            ExceptionalCondition(#condition, (errorType), \
-                                 __FILE__, __LINE__); \
-    } while (0)
-
-/*
- *    TrapMacro is the same as Trap but it's intended for use in macros:
- *
- *        #define foo(x) (AssertMacro(x != 0), bar(x))
- *
- *    Isn't CPP fun?
- */
-#define TrapMacro(condition, errorType) \
-    ((bool) (! (condition) || \
-             (ExceptionalCondition(#condition, (errorType), \
-                                   __FILE__, __LINE__), 0)))
-
 #define Assert(condition) \
     do { \
         if (!(condition)) \
-            ExceptionalCondition(#condition, "FailedAssertion", \
-                                 __FILE__, __LINE__); \
+            ExceptionalCondition(#condition, __FILE__, __LINE__); \
     } while (0)

+/*
+ * AssertMacro is the same as Assert but it's suitable for use in
+ * expression-like macros, for example:
+ *
+ *        #define foo(x) (AssertMacro(x != 0), bar(x))
+ */
 #define AssertMacro(condition) \
     ((void) ((condition) || \
-             (ExceptionalCondition(#condition, "FailedAssertion", \
-                                   __FILE__, __LINE__), 0)))
+             (ExceptionalCondition(#condition, __FILE__, __LINE__), 0)))

-#define AssertArg(condition) \
-    do { \
-        if (!(condition)) \
-            ExceptionalCondition(#condition, "BadArgument", \
-                                 __FILE__, __LINE__); \
-    } while (0)
-
-#define AssertState(condition) \
-    do { \
-        if (!(condition)) \
-            ExceptionalCondition(#condition, "BadState", \
-                                 __FILE__, __LINE__); \
-    } while (0)
+/*
+ * AssertArg and AssertState are identical to Assert.  Some places use them
+ * to indicate that the complaint is specifically about a bad argument or
+ * unexpected state, but this usage is largely obsolescent.
+ */
+#define AssertArg(condition) Assert(condition)
+#define AssertState(condition) Assert(condition)

 /*
  * Check that `ptr' is `bndr' aligned.
  */
 #define AssertPointerAlignment(ptr, bndr) \
-    Trap(TYPEALIGN(bndr, (uintptr_t)(ptr)) != (uintptr_t)(ptr), \
-         "UnalignedPointer")
+    Assert(TYPEALIGN(bndr, (uintptr_t)(ptr)) == (uintptr_t)(ptr))

 #endif                            /* USE_ASSERT_CHECKING && !FRONTEND */

@@ -876,7 +852,6 @@ typedef NameData *Name;
  */
 #ifndef FRONTEND
 extern void ExceptionalCondition(const char *conditionName,
-                                 const char *errorType,
                                  const char *fileName, int lineNumber) pg_attribute_noreturn();
 #endif

diff --git a/src/include/postgres.h b/src/include/postgres.h
index 5f6a1e3d5a..54730dfb38 100644
--- a/src/include/postgres.h
+++ b/src/include/postgres.h
@@ -135,7 +135,7 @@ typedef enum vartag_external
     ((tag) == VARTAG_INDIRECT ? sizeof(varatt_indirect) : \
      VARTAG_IS_EXPANDED(tag) ? sizeof(varatt_expanded) : \
      (tag) == VARTAG_ONDISK ? sizeof(varatt_external) : \
-     TrapMacro(true, "unrecognized TOAST vartag"))
+     (AssertMacro(false), 0))

 /*
  * These structs describe the header of a varlena object that may have been