Re: Adding some error context for lock wait failures - Mailing list pgsql-hackers

From Tom Lane
Subject Re: Adding some error context for lock wait failures
Date
Msg-id 3017219.1760028653@sss.pgh.pa.us
Whole thread Raw
In response to Re: Adding some error context for lock wait failures  (Tom Lane <tgl@sss.pgh.pa.us>)
Responses Re: Adding some error context for lock wait failures
List pgsql-hackers
I wrote:
> Yeah.  I see that errfinish does FreeErrorDataContents in the
> non-ERROR code path, but of course that does nothing for random
> leakages during error processing.  I'm tempted to have it do
> MemoryContextReset(ErrorContext) if we are at stack depth zero.
> That'd be unsafe during nested error processing, but there
> should not be anything of interest leftover once we're out
> of the nest.

Concretely, like the attached.  This passes check-world, but
I can't test it under valgrind because I'm hitting the same
CREATE DATABASE failure skink is reporting.

I wonder if we should back-patch this.  In principle, a backend
that emits a long series of non-error log messages or client
notice messages could accumulate indefinitely much leakage in
ErrorContext.  The lack of field reports suggests that maybe
there weren't any such leaks up to now, but that seems unduly
optimistic.

            regards, tom lane

diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c
index b7b9692f8c8..648d2d2e70c 100644
--- a/src/backend/utils/error/elog.c
+++ b/src/backend/utils/error/elog.c
@@ -542,11 +542,20 @@ errfinish(const char *filename, int lineno, const char *funcname)
     /* Emit the message to the right places */
     EmitErrorReport();

-    /* Now free up subsidiary data attached to stack entry, and release it */
-    FreeErrorDataContents(edata);
-    errordata_stack_depth--;
+    /*
+     * If this is the outermost recursion level, we can clean up by resetting
+     * ErrorContext altogether (compare FlushErrorState), which is good
+     * because it cleans up any random leakages that might have occurred in
+     * places such as context callback functions.  If we're nested, we can
+     * only safely remove the subsidiary data of the current stack entry.
+     */
+    if (errordata_stack_depth == 0 && recursion_depth == 1)
+        MemoryContextReset(ErrorContext);
+    else
+        FreeErrorDataContents(edata);

-    /* Exit error-handling context */
+    /* Release stack entry and exit error-handling context */
+    errordata_stack_depth--;
     MemoryContextSwitchTo(oldcontext);
     recursion_depth--;


pgsql-hackers by date:

Previous
From: Álvaro Herrera
Date:
Subject: Re: memory leak in dbase_redo()
Next
From: Andres Freund
Date:
Subject: Re: Adding some error context for lock wait failures