Thread: Simplifying our Trap/Assert infrastructure
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
On Sun, Oct 09, 2022 at 03:51:57PM -0400, Tom Lane wrote: > 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. +1, I noticed this recently, too. > Hence, I propose the attached. The patch LGTM. It might be worth removing usages of AssertArg and AssertState, too, but that can always be done separately. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Nathan Bossart <nathandbossart@gmail.com> writes:
> On Sun, Oct 09, 2022 at 03:51:57PM -0400, Tom Lane wrote:
>> Hence, I propose the attached.
> The patch LGTM. It might be worth removing usages of AssertArg and
> AssertState, too, but that can always be done separately.
Something I thought about but forgot to mention in the initial email:
is it worth sprinkling these macros with "unlikely()"? I think that
compilers might assume the right thing automatically based on noticing
that ExceptionalCondition is noreturn ... but then again they might
not. Of course we're not that fussed about micro-optimizations in
assert-enabled builds; but with so many Asserts in the system, it
might still add up to something noticeable if there is an effect.
regards, tom lane
On Sun, Oct 09, 2022 at 05:08:39PM -0400, Tom Lane wrote: > Something I thought about but forgot to mention in the initial email: > is it worth sprinkling these macros with "unlikely()"? I think that > compilers might assume the right thing automatically based on noticing > that ExceptionalCondition is noreturn ... but then again they might > not. Of course we're not that fussed about micro-optimizations in > assert-enabled builds; but with so many Asserts in the system, it > might still add up to something noticeable if there is an effect. I don't see why not. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Nathan Bossart <nathandbossart@gmail.com> writes:
> On Sun, Oct 09, 2022 at 05:08:39PM -0400, Tom Lane wrote:
>> Something I thought about but forgot to mention in the initial email:
>> is it worth sprinkling these macros with "unlikely()"?
> I don't see why not.
I experimented with that, and found something that surprised me:
there's a noticeable code-bloat effect. With the patch as given,
$ size src/backend/postgres
text data bss dec hex filename
9001199 86280 204496 9291975 8dc8c7 src/backend/postgres
but with unlikely(),
$ size src/backend/postgres
text data bss dec hex filename
9035423 86280 204496 9326199 8e4e77 src/backend/postgres
I don't quite understand why that's happening, but it seems to
show that this requires some investigation of its own. So for
now I just pushed the patch as-is.
regards, tom lane
On Sun, Oct 09, 2022 at 02:01:48PM -0700, Nathan Bossart wrote: > The patch LGTM. It might be worth removing usages of AssertArg and > AssertState, too, but that can always be done separately. If you are so inclined... -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Attachment
On 12.10.22 20:36, Nathan Bossart wrote: > On Sun, Oct 09, 2022 at 02:01:48PM -0700, Nathan Bossart wrote: >> The patch LGTM. It might be worth removing usages of AssertArg and >> AssertState, too, but that can always be done separately. > > If you are so inclined... I'm in favor of this. These variants are a distraction.
On Wed, Oct 12, 2022 at 09:19:17PM +0200, Peter Eisentraut wrote: > I'm in favor of this. These variants are a distraction. Agreed, even if extensions could use these, it looks like any out-of-core code using what's removed here would also gain in clarity. This is logically fine (except for an indentation blip in miscadmin.h?), so I have marked this entry as ready for committer. Side note, rather unrelated to what's proposed here: would it be worth extending AssertPointerAlignment() for the frontend code? -- Michael
Attachment
On 27.10.22 09:23, Michael Paquier wrote: > Agreed, even if extensions could use these, it looks like any > out-of-core code using what's removed here would also gain in clarity. > This is logically fine (except for an indentation blip in > miscadmin.h?), so I have marked this entry as ready for committer. committed > Side note, rather unrelated to what's proposed here: would it be worth > extending AssertPointerAlignment() for the frontend code? Would there be a use for that? It's currently only used in the atomics code.
On Fri, Oct 28, 2022 at 09:36:23AM +0200, Peter Eisentraut wrote: > Would there be a use for that? It's currently only used in the atomics > code. Yep, but they would not trigger when using atomics in the frontend code. We don't have any use for that in core on HEAD, still that could be useful for some external frontend code? Please see the attached. -- Michael
Attachment
On 31.10.22 01:04, Michael Paquier wrote: > On Fri, Oct 28, 2022 at 09:36:23AM +0200, Peter Eisentraut wrote: >> Would there be a use for that? It's currently only used in the atomics >> code. > > Yep, but they would not trigger when using atomics in the frontend > code. We don't have any use for that in core on HEAD, still that > could be useful for some external frontend code? Please see the > attached. I don't think we need separate definitions for frontend and backend, since the contained Assert() will take care of the difference. So the attached would be simpler.
Attachment
Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:
> I don't think we need separate definitions for frontend and backend,
> since the contained Assert() will take care of the difference. So the
> attached would be simpler.
WFM.
regards, tom lane
On Mon, Oct 31, 2022 at 09:14:10AM -0400, Tom Lane wrote: > Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes: >> I don't think we need separate definitions for frontend and backend, >> since the contained Assert() will take care of the difference. So the >> attached would be simpler. > > WFM. Thanks, fine by me. -- Michael