Fix re-execution of a failed SQLFunctionCache entry.

If we error out during execution of a SQL-language function, we will
often leave behind non-null pointers in its SQLFunctionCache's cplan
and eslist fields.  This is problematic if the SQLFunctionCache is
re-used, because those pointers will point at resources that were
released during error cleanup.  This problem escaped detection so far
because ordinarily we won't re-use an FmgrInfo+SQLFunctionCache struct
after a query error.  However, in the rather improbable case that
someone implements an opclass support function in SQL language, there
will be long-lived FmgrInfos for it in the relcache, and then the
problem is reachable after the function throws an error.

To fix, add a flag to SQLFunctionCache that tracks whether execution
escapes out of fmgr_sql, and clear out the relevant fields during
init_sql_fcache if so.  (This is going to need more thought if we ever
try to share FMgrInfos across threads; but it's very far from being
the only problem such a project will encounter, since many functions
regard fn_extra as being query-local state.)

This broke at commit 0313c5dc6; before that we did not try to re-use
SQLFunctionCache state across calls.  Hence, back-patch to v18.

Bug: #19026
Reported-by: Alexander Lakhin <exclusion@gmail.com>
Author: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/19026-90aed5e71d0c8af3@postgresql.org
Backpatch-through: 18

1 files changed, 29 insertions, 0 deletions

diff --git a/src/backend/executor/functions.c b/src/backend/executor/functions.c
index 359aafea681..97455b1ed4a 100644
--- a/src/backend/executor/functions.c
+++ b/src/backend/executor/functions.c
@@ -143,6 +143,7 @@ typedef struct SQLFunctionCache
 {
 	SQLFunctionHashEntry *func; /* associated SQLFunctionHashEntry */
 
+	bool		active;			/* are we executing this cache entry? */
 	bool		lazyEvalOK;		/* true if lazyEval is safe */
 	bool		shutdown_reg;	/* true if registered shutdown callback */
 	bool		lazyEval;		/* true if using lazyEval for result query */
@@ -557,6 +558,28 @@ init_sql_fcache(FunctionCallInfo fcinfo, bool lazyEvalOK)
 	}
 
 	/*
+	 * If the SQLFunctionCache is marked as active, we must have errored out
+	 * of a prior execution.  Reset state.  (It might seem that we could also
+	 * reach this during recursive invocation of a SQL function, but we won't
+	 * because that case won't involve re-use of the same FmgrInfo.)
+	 */
+	if (fcache->active)
+	{
+		/*
+		 * In general, this stanza should clear all the same fields that
+		 * ShutdownSQLFunction would.  Note we must clear fcache->cplan
+		 * without doing ReleaseCachedPlan, because error cleanup from the
+		 * prior execution would have taken care of releasing that plan.
+		 * Likewise, if tstore is still set then it is pointing at garbage.
+		 */
+		fcache->cplan = NULL;
+		fcache->eslist = NULL;
+		fcache->tstore = NULL;
+		fcache->shutdown_reg = false;
+		fcache->active = false;
+	}
+
+	/*
 	 * If we are resuming execution of a set-returning function, just keep
 	 * using the same cache.  We do not ask funccache.c to re-validate the
 	 * SQLFunctionHashEntry: we want to run to completion using the function's
@@ -1597,6 +1620,9 @@ fmgr_sql(PG_FUNCTION_ARGS)
 	 */
 	fcache = init_sql_fcache(fcinfo, lazyEvalOK);
 
+	/* Mark fcache as active */
+	fcache->active = true;
+
 	/* Remember info that we might need later to construct tuplestore */
 	fcache->tscontext = tscontext;
 	fcache->randomAccess = randomAccess;
@@ -1853,6 +1879,9 @@ fmgr_sql(PG_FUNCTION_ARGS)
 	if (es == NULL)
 		fcache->eslist = NULL;
 
+	/* Mark fcache as inactive */
+	fcache->active = false;
+
 	error_context_stack = sqlerrcontext.previous;
 
 	return result;