From: Tom Lane <tgl@sss.pgh.pa.us>
To: Dilip Kumar <dilipbalaut@gmail.com>
Cc: Kirill Reshke <reshkekirill@gmail.com>
Cc: exclusion@gmail.com
Cc: pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #19026: ResourceOwnerForget can't find owner for invalid plancache
Date: Wed, 20 Aug 2025 12:57:21 -0400
Message-ID: <1181152.1755709041@sss.pgh.pa.us> (raw)
In-Reply-To: <1117844.1755704498@sss.pgh.pa.us>
References: <19026-90aed5e71d0c8af3@postgresql.org>
<CAFiTN-st+8+OQExTgLB1FUi_i+NSBz9aUeuzAcRzx6nQZDHUwg@mail.gmail.com>
<CALdSSPijvR-X693FPJfT2==7Otb=Ft6NrPQ02r4i1c8MnW_8tQ@mail.gmail.com>
<CAFiTN-v+N6Po698Tuew0GPnBNTYodV17vedENohC-GSDEAciUA@mail.gmail.com>
<CALdSSPjsYgY18vCveFzPNJQ_HM6+9i_2CEbt_Lgj5JBx9a==zg@mail.gmail.com>
<CAFiTN-uauvLrXXJ6uVO0nawT-w6DjSz=wRSRXk43ML50sCSVMg@mail.gmail.com>
<1117844.1755704498@sss.pgh.pa.us>
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
Attachments:
[text/x-diff] bug-19026-alternative-3.patch (5.5K, ../1181152.1755709041@sss.pgh.pa.us/2-bug-19026-alternative-3.patch)
download | inline diff:diff --git a/src/backend/executor/functions.c b/src/backend/executor/functions.cindex 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.outindex 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.sqlindex 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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-bugs@postgresql.org
Cc: tgl@sss.pgh.pa.us, dilipbalaut@gmail.com, reshkekirill@gmail.com, exclusion@gmail.com, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #19026: ResourceOwnerForget can't find owner for invalid plancache
In-Reply-To: <1181152.1755709041@sss.pgh.pa.us>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox