From: Tom Lane <tgl@sss.pgh.pa.us>
To: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
Cc: Robert Haas <robertmhaas@gmail.com>
Cc: paul.kulakov@systematica.ru
Cc: pgsql-hackers@lists.postgresql.org
Subject: Re: BUG #18059: Unexpected error 25001 in stored procedure
Date: Wed, 23 Aug 2023 16:53:12 -0400
Message-ID: <3555188.1692823992@sss.pgh.pa.us> (raw)
In-Reply-To: <3211510.1692739765@sss.pgh.pa.us>
References: <18059-79c692f036b25346@postgresql.org>
<2341329.1692465562@sss.pgh.pa.us>
<CA+Tgmoa2dBm7=6H986FEynqv=tHdxvXt5HVYuO24TC97G4HkkA@mail.gmail.com>
<3211510.1692739765@sss.pgh.pa.us>
I wrote:
> I started to code this, and immediately noticed that transformStmt()
> already has a companion function analyze_requires_snapshot() that
> returns "true" in the cases of interest ... except that it does
> not return true for T_CallStmt. Perhaps that was intentional to
> begin with, but it is very hard to believe that it isn't a bug now,
> since transformCallStmt can invoke nearly arbitrary processing via
> transformExpr(). What semantic anomalies, if any, do we risk if CALL
> processing forces a transaction start? (I rather imagine it does
> already, somewhere later on...)
I poked around some more, and determined that there should not be any
new semantic anomalies if analyze_requires_snapshot starts returning
true for CallStmts, because ExecuteCallStmt already acquires and
releases a snapshot before invoking the procedure (at least in the
non-atomic case, which is the one of interest here). I spent some
time trying to devise a test case showing it's broken, and did not
succeed: the fact that we disallow sub-SELECTs in CALL arguments makes
it a lot harder than I'd expected to reach anyplace that would require
having a transaction snapshot set. Nonetheless, I have very little
faith that such a scenario doesn't exist today, and even less that
we won't add one in future. The only real reason I can see for not
setting a snapshot here is as a micro-optimization. While that's
not without value, it seems hard to argue that CALL deserves an
optimization that SELECT doesn't get.
I also realized that ReturnStmts are likewise missing from
analyze_requires_snapshot(). This is probably unreachable, because
ReturnStmt can only appear in a SQL-language function and I can't
think of a scenario where we'd be parsing one outside a transaction.
Nonetheless it seems hard to argue that this is an optimization
we need to keep.
Hence I propose the attached patch, which invents
stmt_requires_parse_analysis() and makes analyze_requires_snapshot()
into an alias for it, so that all these statement types are treated
alike. I made the adjacent comments a lot more opinionated, too,
in hopes that future additions won't overlook these concerns.
The actual bug fix is in plancache.c. I decided to invert the tests
in plancache.c, because the macro really needed renaming anyway and
it seemed to read better this way. I also noticed that
ResetPlanCache() already tries to optimize away invalidation of
utility statements, but that logic seems no longer necessary ---
what's more, it's outright wrong for CALL, because that does need
invalidation and won't get it. (I have not tried to build a test
case proving that that's broken, but surely it is.)
Barring objections, this needs to be back-patched as far as v11.
regards, tom lane
Attachments:
[text/x-diff] v1-fix-bug-18059.patch (11.1K, ../3555188.1692823992@sss.pgh.pa.us/2-v1-fix-bug-18059.patch)
download | inline diff:diff --git a/src/backend/parser/analyze.c b/src/backend/parser/analyze.cindex 4006632092..bc8176f545 100644--- a/src/backend/parser/analyze.c+++ b/src/backend/parser/analyze.c@@ -335,6 +335,11 @@ transformStmt(ParseState *pstate, Node *parseTree)
}
#endif /* RAW_EXPRESSION_COVERAGE_TEST */
+ /*+ * Caution: when changing the set of statement types that have non-default+ * processing here, see also stmt_requires_parse_analysis() and+ * analyze_requires_snapshot().+ */
switch (nodeTag(parseTree))
{
/*
@@ -421,14 +426,22 @@ transformStmt(ParseState *pstate, Node *parseTree)
}
/*
- * analyze_requires_snapshot- * Returns true if a snapshot must be set before doing parse analysis- * on the given raw parse tree.+ * stmt_requires_parse_analysis+ * Returns true if parse analysis will do anything non-trivial+ * with the given raw parse tree.+ *+ * Generally, this should return true for any statement type for which+ * transformStmt() does more than wrap a CMD_UTILITY Query around it.+ * When it returns false, the caller may assume that there is no situation+ * in which parse analysis of the raw statement could need to be re-done.
*
- * Classification here should match transformStmt().+ * Currently, since the rewriter and planner do nothing for CMD_UTILITY+ * Queries, a false result means that the entire parse analysis/rewrite/plan+ * pipeline will never need to be re-done. If that ever changes, callers+ * will likely need adjustment.
*/
bool
-analyze_requires_snapshot(RawStmt *parseTree)+stmt_requires_parse_analysis(RawStmt *parseTree)
{
bool result;
@@ -442,6 +455,7 @@ analyze_requires_snapshot(RawStmt *parseTree)
case T_UpdateStmt:
case T_MergeStmt:
case T_SelectStmt:
+ case T_ReturnStmt:
case T_PLAssignStmt:
result = true;
break;
@@ -452,12 +466,12 @@ analyze_requires_snapshot(RawStmt *parseTree)
case T_DeclareCursorStmt:
case T_ExplainStmt:
case T_CreateTableAsStmt:
- /* yes, because we must analyze the contained statement */+ case T_CallStmt:
result = true;
break;
default:
- /* other utility statements don't have any real parse analysis */+ /* all other statements just get wrapped in a CMD_UTILITY Query */
result = false;
break;
}
@@ -465,6 +479,30 @@ analyze_requires_snapshot(RawStmt *parseTree)
return result;
}
+/*+ * analyze_requires_snapshot+ * Returns true if a snapshot must be set before doing parse analysis+ * on the given raw parse tree.+ */+bool+analyze_requires_snapshot(RawStmt *parseTree)+{+ /*+ * Currently, this should return true in exactly the same cases that+ * stmt_requires_parse_analysis() does, so we just invoke that function+ * rather than duplicating it. We keep the two entry points separate for+ * clarity of callers, since from the callers' standpoint these are+ * different conditions.+ *+ * While there may someday be a statement type for which transformStmt()+ * does something nontrivial and yet no snapshot is needed for that+ * processing, it seems likely that making such a choice would be fragile.+ * If you want to install an exception, document the reasoning for it in a+ * comment.+ */+ return stmt_requires_parse_analysis(parseTree);+}+
/*
* transformDeleteStmt -
* transforms a Delete Statement
diff --git a/src/backend/utils/cache/plancache.c b/src/backend/utils/cache/plancache.cindex d67cd9a405..7d4168f82f 100644--- a/src/backend/utils/cache/plancache.c+++ b/src/backend/utils/cache/plancache.c@@ -77,13 +77,15 @@
/*
* We must skip "overhead" operations that involve database access when the
- * cached plan's subject statement is a transaction control command.- * For the convenience of postgres.c, treat empty statements as control- * commands too.+ * cached plan's subject statement is a transaction control command or one+ * that requires a snapshot not to be set yet (such as SET or LOCK). More+ * generally, statements that do not require parse analysis/rewrite/plan+ * activity never need to be revalidated, so we can treat them all like that.+ * For the convenience of postgres.c, treat empty statements that way too.
*/
-#define IsTransactionStmtPlan(plansource) \- ((plansource)->raw_parse_tree == NULL || \- IsA((plansource)->raw_parse_tree->stmt, TransactionStmt))+#define StmtPlanRequiresRevalidation(plansource) \+ ((plansource)->raw_parse_tree != NULL && \+ stmt_requires_parse_analysis((plansource)->raw_parse_tree))
/*
* This is the head of the backend's list of "saved" CachedPlanSources (i.e.,
@@ -383,13 +385,13 @@ CompleteCachedPlan(CachedPlanSource *plansource,
plansource->query_context = querytree_context;
plansource->query_list = querytree_list;
- if (!plansource->is_oneshot && !IsTransactionStmtPlan(plansource))+ if (!plansource->is_oneshot && StmtPlanRequiresRevalidation(plansource))
{
/*
* Use the planner machinery to extract dependencies. Data is saved
* in query_context. (We assume that not a lot of extra cruft is
* created by this call.) We can skip this for one-shot plans, and
- * transaction control commands have no such dependencies anyway.+ * plans not needing revalidation have no such dependencies anyway.
*/
extract_query_dependencies((Node *) querytree_list,
&plansource->relationOids,
@@ -568,11 +570,11 @@ RevalidateCachedQuery(CachedPlanSource *plansource,
/*
* For one-shot plans, we do not support revalidation checking; it's
* assumed the query is parsed, planned, and executed in one transaction,
- * so that no lock re-acquisition is necessary. Also, there is never any- * need to revalidate plans for transaction control commands (and we- * mustn't risk any catalog accesses when handling those).+ * so that no lock re-acquisition is necessary. Also, if the statement+ * type can't require revalidation, we needn't do anything (and we mustn't+ * risk catalog accesses when handling, eg, transaction control commands).
*/
- if (plansource->is_oneshot || IsTransactionStmtPlan(plansource))+ if (plansource->is_oneshot || !StmtPlanRequiresRevalidation(plansource))
{
Assert(plansource->is_valid);
return NIL;
@@ -1029,8 +1031,8 @@ choose_custom_plan(CachedPlanSource *plansource, ParamListInfo boundParams)
/* Otherwise, never any point in a custom plan if there's no parameters */
if (boundParams == NULL)
return false;
- /* ... nor for transaction control statements */- if (IsTransactionStmtPlan(plansource))+ /* ... nor when planning would be a no-op */+ if (!StmtPlanRequiresRevalidation(plansource))
return false;
/* Let settings force the decision */
@@ -1972,8 +1974,8 @@ PlanCacheRelCallback(Datum arg, Oid relid)
if (!plansource->is_valid)
continue;
- /* Never invalidate transaction control commands */- if (IsTransactionStmtPlan(plansource))+ /* Never invalidate if parse/plan would be a no-op anyway */+ if (!StmtPlanRequiresRevalidation(plansource))
continue;
/*
@@ -2057,8 +2059,8 @@ PlanCacheObjectCallback(Datum arg, int cacheid, uint32 hashvalue)
if (!plansource->is_valid)
continue;
- /* Never invalidate transaction control commands */- if (IsTransactionStmtPlan(plansource))+ /* Never invalidate if parse/plan would be a no-op anyway */+ if (!StmtPlanRequiresRevalidation(plansource))
continue;
/*
@@ -2167,7 +2169,6 @@ ResetPlanCache(void)
{
CachedPlanSource *plansource = dlist_container(CachedPlanSource,
node, iter.cur);
- ListCell *lc;
Assert(plansource->magic == CACHEDPLANSOURCE_MAGIC);
@@ -2179,32 +2180,16 @@ ResetPlanCache(void)
* We *must not* mark transaction control statements as invalid,
* particularly not ROLLBACK, because they may need to be executed in
* aborted transactions when we can't revalidate them (cf bug #5269).
+ * In general there's no point in invalidating statements for which a+ * new parse analysis/rewrite/plan cycle would certainly give the same+ * results.
*/
- if (IsTransactionStmtPlan(plansource))+ if (!StmtPlanRequiresRevalidation(plansource))
continue;
- /*- * In general there is no point in invalidating utility statements- * since they have no plans anyway. So invalidate it only if it- * contains at least one non-utility statement, or contains a utility- * statement that contains a pre-analyzed query (which could have- * dependencies.)- */- foreach(lc, plansource->query_list)- {- Query *query = lfirst_node(Query, lc);-- if (query->commandType != CMD_UTILITY ||- UtilityContainsQuery(query->utilityStmt))- {- /* non-utility statement, so invalidate */- plansource->is_valid = false;- if (plansource->gplan)- plansource->gplan->is_valid = false;- /* no need to look further */- break;- }- }+ plansource->is_valid = false;+ if (plansource->gplan)+ plansource->gplan->is_valid = false;
}
/* Likewise invalidate cached expressions */
diff --git a/src/include/parser/analyze.h b/src/include/parser/analyze.hindex 1cef1833a6..c96483ae78 100644--- a/src/include/parser/analyze.h+++ b/src/include/parser/analyze.h@@ -47,6 +47,7 @@ extern List *transformUpdateTargetList(ParseState *pstate,
extern Query *transformTopLevelStmt(ParseState *pstate, RawStmt *parseTree);
extern Query *transformStmt(ParseState *pstate, Node *parseTree);
+extern bool stmt_requires_parse_analysis(RawStmt *parseTree);
extern bool analyze_requires_snapshot(RawStmt *parseTree);
extern const char *LCS_asString(LockClauseStrength strength);
diff --git a/src/pl/plpgsql/src/expected/plpgsql_call.out b/src/pl/plpgsql/src/expected/plpgsql_call.outindex 1ec6182a8d..7ab23c6a21 100644--- a/src/pl/plpgsql/src/expected/plpgsql_call.out+++ b/src/pl/plpgsql/src/expected/plpgsql_call.out@@ -35,6 +35,23 @@ SELECT * FROM test1;
55
(1 row)
+-- Check that plan revalidation doesn't prevent setting transaction properties+-- (bug #18059). This test must include the first temp-object creation in+-- this script, or it won't exercise the bug scenario. Hence we put it early.+CREATE PROCEDURE test_proc3a()+LANGUAGE plpgsql+AS $$+BEGIN+ COMMIT;+ SET TRANSACTION ISOLATION LEVEL REPEATABLE READ;+ RAISE NOTICE 'done';+END;+$$;+CALL test_proc3a();+NOTICE: done+CREATE TEMP TABLE tt1(f1 int);+CALL test_proc3a();+NOTICE: done
-- nested CALL
TRUNCATE TABLE test1;
CREATE PROCEDURE test_proc4(y int)
diff --git a/src/pl/plpgsql/src/sql/plpgsql_call.sql b/src/pl/plpgsql/src/sql/plpgsql_call.sqlindex 5028398348..14bbffa0b2 100644--- a/src/pl/plpgsql/src/sql/plpgsql_call.sql+++ b/src/pl/plpgsql/src/sql/plpgsql_call.sql@@ -38,6 +38,24 @@ CALL test_proc3(55);
SELECT * FROM test1;
+-- Check that plan revalidation doesn't prevent setting transaction properties+-- (bug #18059). This test must include the first temp-object creation in+-- this script, or it won't exercise the bug scenario. Hence we put it early.+CREATE PROCEDURE test_proc3a()+LANGUAGE plpgsql+AS $$+BEGIN+ COMMIT;+ SET TRANSACTION ISOLATION LEVEL REPEATABLE READ;+ RAISE NOTICE 'done';+END;+$$;++CALL test_proc3a();+CREATE TEMP TABLE tt1(f1 int);+CALL test_proc3a();++
-- nested CALL
TRUNCATE TABLE test1;
=
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, peter.eisentraut@enterprisedb.com, robertmhaas@gmail.com, paul.kulakov@systematica.ru, pgsql-hackers@lists.postgresql.org
Subject: Re: BUG #18059: Unexpected error 25001 in stored procedure
In-Reply-To: <3555188.1692823992@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