Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1uom7q-009y8z-UX for pgsql-bugs@arkaria.postgresql.org; Wed, 20 Aug 2025 16:57:28 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.94.2) (envelope-from ) id 1uom7q-009y9h-AM for pgsql-bugs@arkaria.postgresql.org; Wed, 20 Aug 2025 16:57:26 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1uom7q-009y9T-1D for pgsql-bugs@lists.postgresql.org; Wed, 20 Aug 2025 16:57:26 +0000 Received: from sss.pgh.pa.us ([68.162.161.243]) by magus.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.96) (envelope-from ) id 1uom7o-000wfq-0j for pgsql-bugs@lists.postgresql.org; Wed, 20 Aug 2025 16:57:26 +0000 Received: from sss1.sss.pgh.pa.us (localhost [127.0.0.1]) by sss.pgh.pa.us (8.15.2/8.15.2) with ESMTP id 57KGvLE31181153; Wed, 20 Aug 2025 12:57:21 -0400 From: Tom Lane To: Dilip Kumar cc: Kirill Reshke , exclusion@gmail.com, pgsql-bugs@lists.postgresql.org Subject: Re: BUG #19026: ResourceOwnerForget can't find owner for invalid plancache In-reply-to: <1117844.1755704498@sss.pgh.pa.us> References: <19026-90aed5e71d0c8af3@postgresql.org> <1117844.1755704498@sss.pgh.pa.us> Comments: In-reply-to Tom Lane message dated "Wed, 20 Aug 2025 11:41:38 -0400" MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0" Content-ID: <1181080.1755708987.0@sss.pgh.pa.us> Date: Wed, 20 Aug 2025 12:57:21 -0400 Message-ID: <1181152.1755709041@sss.pgh.pa.us> List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk ------- =_aaaaaaaaaa0 Content-Type: text/plain; charset="us-ascii" Content-ID: <1181080.1755708987.1@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 ------- =_aaaaaaaaaa0 Content-Type: text/x-diff; name="bug-19026-alternative-3.patch"; charset="us-ascii" Content-ID: <1181080.1755708987.2@sss.pgh.pa.us> Content-Description: bug-19026-alternative-3.patch Content-Transfer-Encoding: quoted-printable diff --git a/src/backend/executor/functions.c b/src/backend/executor/funct= ions.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 lazyEva= lOK) finfo->fn_extra =3D 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 als= o + * 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 =3D NULL; + fcache->eslist =3D NULL; + fcache->tstore =3D NULL; + fcache->shutdown_reg =3D false; + fcache->active =3D 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 =3D init_sql_fcache(fcinfo, lazyEvalOK); = + /* Mark fcache as active */ + fcache->active =3D true; + /* Remember info that we might need later to construct tuplestore */ fcache->tscontext =3D tscontext; fcache->randomAccess =3D randomAccess; @@ -1853,6 +1879,9 @@ fmgr_sql(PG_FUNCTION_ARGS) if (es =3D=3D NULL) fcache->eslist =3D NULL; = + /* Mark fcache as inactive */ + fcache->active =3D false; + error_context_stack =3D 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 relca= che. +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/regre= ss/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 relca= che. +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 ------- =_aaaaaaaaaa0--