Hi All,
PostgreSQL code uses Assert() as a way to
1. document the assumption or conditions which must be true at a given
place in code
2. make sure that some bug elsewhere does not break these assumptions or rules
3. conditions which can not be (easily) induced by user actions
E.g. following conditions in adjust_appendrel_attrs()
/* If there's nothing to adjust, don't call this function. */
Assert(nappinfos >= 1 && appinfos != NULL);
/* Should never be translating a Query tree. */
Assert(node == NULL || !IsA(node, Query));
These conditions do not make it to non-assert builds and thus do not
make it to the production binary. That saves some CPU cycles.
When an Assert fails, and it fails quite a lot for developers, the
Postgres backend that caused the Assert is Aborted, restarting the
server. So a developer testing code that caused the Assert has to
start a psql again, run any session setup before running the faulting
query, gdb needs to be reattached to the new backend process. That's
some waste of time and frustration, esp. when the Assert does not
damage the backend itself e.g. by corrupting memory.
Most of the Asserts are recoverable by rolling back the transaction
without crashing the backend. So an elog(ERROR, ) is enough. But just
by themselves elogs are compiled into non-debug binary and the
condition check can waste CPU cycles esp. conditions are mostly true
like the ones we use in Assert.
Attached patch combines Assert and elog(ERROR, ) so that when an
Assert is triggered in assert-enabled binary, it will throw an error
while keeping the backend intact. Thus it does not affect gdb session
or psql session. These elogs do not make their way to non-assert
binary so do not make it to production like Assert.
I have been using AssertLog for my work for some time. It is quite
convenient. With AssertLog I get
```
#explain (summary on) select * from a, b, c where a.c1 = b.c1 and b.c1
= c.c1 and a.c2 < b.c2 and a.c3 + b.c3 < c.c3;
ERROR: failed Assert("equal(child_rinfo, adjust_appendrel_attrs(root,
(Node *) rinfo, nappinfos, appinfos))"), File: "relnode.c", Line: 997,
PID: 568088
```
instead of
```
#explain (summary on) select * from a, b, c where a.c1 = b.c1 and b.c1
= c.c1 and a.c2 < b.c2 and a.c3 + b.c3 < c.c3;
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
The connection to the server was lost. Attempting reset: Failed.
The connection to the server was lost. Attempting reset: Failed.
@!#\q
```
If there is an interest in accepting the patch, I will add it to the
commitfest and work on it further.
--
Best Wishes,
Ashutosh Bapat
From 2464ac9de8a58846521d93db5c3c43a24d73f6b8 Mon Sep 17 00:00:00 2001
From: Ashutosh Bapat <ashutosh.ba...@enterprisedb.com>
Date: Tue, 8 Aug 2023 14:37:59 +0530
Subject: [PATCH 1/6] Add AssertLog to report an error instead of Abort
Assert is used in the code to document assumptions, check conditions which
ought to be correct at given places in the code and so on. When an Assert
fails, the process Aborts, restarting the whole server. This is too disruptive
for development. Many of the Assert check conditions which are quite local in
nature, many a times very specific to the encapsulating transaction. In such
cases the backend can be brought out of the bad state just by aborting the
transaction. elog(ERROR) serves that purpose well. But when just elog is used
it get compiled in non-assert and non-debug binary. The condition around it
eats some CPU cycles in production.
This commit introduces AssertLog which brings together the benefits of both
Assert() and elog(). AssertLog is compiled only in assert enabled binaries. It
throws an ERROR when the condition fails instead of Aborting the process. Thus
the backend recovers simply by aborting the transaction and developers can
continue to debug, investigate without restarting their psql and reattaching
gdb.
Ashutosh Bapat
---
src/include/c.h | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/src/include/c.h b/src/include/c.h
index f69d739be5..052b9441bc 100644
--- a/src/include/c.h
+++ b/src/include/c.h
@@ -66,6 +66,7 @@
#endif
#include <stdint.h>
#include <sys/types.h>
+#include <unistd.h>
#include <errno.h>
#if defined(WIN32) || defined(__CYGWIN__)
#include <fcntl.h> /* ensure O_BINARY is available */
@@ -840,12 +841,14 @@ typedef NameData *Name;
#ifndef USE_ASSERT_CHECKING
#define Assert(condition) ((void)true)
+#define AssertLog(condition) ((void)true)
#define AssertMacro(condition) ((void)true)
#elif defined(FRONTEND)
#include <assert.h>
#define Assert(p) assert(p)
+#define AssertLog(p) assert(p)
#define AssertMacro(p) ((void) assert(p))
#else /* USE_ASSERT_CHECKING && !FRONTEND */
@@ -860,6 +863,13 @@ typedef NameData *Name;
ExceptionalCondition(#condition, __FILE__, __LINE__); \
} while (0)
+#define AssertLog(condition) \
+ do { \
+ if (!(condition)) \
+ elog(ERROR, "failed Assert(\"%s\"), File: \"%s\", Line: %d, PID: %d", \
+ #condition , __FILE__, __LINE__, (int) getpid()); \
+ } while (0)
+
/*
* AssertMacro is the same as Assert but it's suitable for use in
* expression-like macros, for example:
--
2.25.1
