Re: BUG #19026: ResourceOwnerForget can't find owner for invalid plancache

I wrote:
> In practice, fcache->cplan will never be not-null after successful
> completion of a SQL function, so one idea is to simply clear it
> unconditionally as soon as we know we're starting a fresh execution,
> more or less as in alternative-1 attached.  However that leaves me
> a bit unsatisfied, because it doesn't protect against the case of
> erroring out of a set-returning function: if we come in and see
> eslist != NULL, we'll pick right back up attempting to execute
> plans that probably aren't there anymore.  I think that that case
> is unreachable today because we don't allow any opclass support
> functions to be SRFs, and AFAIK there are no other cases where an
> FmgrInfo would be re-used after a failed query.  Still, I'm inclined
> to go with something more like alternative-2, which feels a little
> more future-proof.

After closer inspection: Alexander's test case doesn't expose the full
scope of the problem.  If the function suffers an error at run-time
rather than early in setup, then we will fall out with fcache->eslist
being non-null as well as fcache->cplan.  Then on the next call,
init_sql_fcache will believe that it is resuming execution of a
set-returning function, and we'll merrily try to execute an executor
state tree that's not there anymore.

So we have to do something more like my alternative-2 than any of
the other proposals, and after reviewing the code I believe it
had better reset the tstore and shutdown_reg fields as well.
Hence, v3 attached, now with regression test case.

            regards, tom lane

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 */
@@ -556,6 +557,28 @@ init_sql_fcache(FunctionCallInfo fcinfo, bool lazyEvalOK)
         finfo->fn_extra = fcache;
     }

+    /*
+     * 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
@@ -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;
diff --git a/src/test/regress/expected/create_function_sql.out b/src/test/regress/expected/create_function_sql.out
index 963b6f863ff..da112608d66 100644
--- a/src/test/regress/expected/create_function_sql.out
+++ b/src/test/regress/expected/create_function_sql.out
@@ -733,6 +733,22 @@ SELECT double_append(array_append(ARRAY[q1], q2), q3)
  {4,5,6,4,5,6}
 (2 rows)

+-- Check that we can re-use a SQLFunctionCache after a run-time error.
+-- This function will fail with zero-divide at run time (not plan time).
+CREATE FUNCTION part_hashint4_error(value int4, seed int8) RETURNS int8
+LANGUAGE SQL STRICT IMMUTABLE PARALLEL SAFE AS
+$$ SELECT value + seed + random()::int/0 $$;
+-- Put it into an operator class so that FmgrInfo will be cached in relcache.
+CREATE OPERATOR CLASS part_test_int4_ops_bad FOR TYPE int4 USING hash AS
+  FUNCTION 2 part_hashint4_error(int4, int8);
+CREATE TABLE pt(i int) PARTITION BY hash (i part_test_int4_ops_bad);
+CREATE TABLE p1 PARTITION OF pt FOR VALUES WITH (modulus 4, remainder 0);
+INSERT INTO pt VALUES (1);
+ERROR:  division by zero
+CONTEXT:  SQL function "part_hashint4_error" statement 1
+INSERT INTO pt VALUES (1);
+ERROR:  division by zero
+CONTEXT:  SQL function "part_hashint4_error" statement 1
 -- Things that shouldn't work:
 CREATE FUNCTION test1 (int) RETURNS int LANGUAGE SQL
     AS 'SELECT ''not an integer'';';
@@ -773,7 +789,7 @@ CONTEXT:  SQL function "test1" during startup
 RESET check_function_bodies;
 -- Cleanup
 DROP SCHEMA temp_func_test CASCADE;
-NOTICE:  drop cascades to 35 other objects
+NOTICE:  drop cascades to 38 other objects
 DETAIL:  drop cascades to function functest_a_1(text,date)
 drop cascades to function functest_a_2(text[])
 drop cascades to function functest_a_3()
@@ -808,6 +824,9 @@ drop cascades to function create_and_insert()
 drop cascades to table ddl_test
 drop cascades to function alter_and_insert()
 drop cascades to function double_append(anyarray,anyelement)
+drop cascades to function part_hashint4_error(integer,bigint)
+drop cascades to operator family part_test_int4_ops_bad for access method hash
+drop cascades to table pt
 drop cascades to function test1(anyelement)
 DROP USER regress_unpriv_user;
 RESET search_path;
diff --git a/src/test/regress/sql/create_function_sql.sql b/src/test/regress/sql/create_function_sql.sql
index 6d1c102d780..3d5f2a92093 100644
--- a/src/test/regress/sql/create_function_sql.sql
+++ b/src/test/regress/sql/create_function_sql.sql
@@ -432,6 +432,23 @@ $$ SELECT array_append($1, $2) || array_append($1, $2) $$;
 SELECT double_append(array_append(ARRAY[q1], q2), q3)
   FROM (VALUES(1,2,3), (4,5,6)) v(q1,q2,q3);

+-- Check that we can re-use a SQLFunctionCache after a run-time error.
+
+-- This function will fail with zero-divide at run time (not plan time).
+CREATE FUNCTION part_hashint4_error(value int4, seed int8) RETURNS int8
+LANGUAGE SQL STRICT IMMUTABLE PARALLEL SAFE AS
+$$ SELECT value + seed + random()::int/0 $$;
+
+-- Put it into an operator class so that FmgrInfo will be cached in relcache.
+CREATE OPERATOR CLASS part_test_int4_ops_bad FOR TYPE int4 USING hash AS
+  FUNCTION 2 part_hashint4_error(int4, int8);
+
+CREATE TABLE pt(i int) PARTITION BY hash (i part_test_int4_ops_bad);
+CREATE TABLE p1 PARTITION OF pt FOR VALUES WITH (modulus 4, remainder 0);
+
+INSERT INTO pt VALUES (1);
+INSERT INTO pt VALUES (1);
+
 -- Things that shouldn't work:

 CREATE FUNCTION test1 (int) RETURNS int LANGUAGE SQL