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
diff --git a/src/backend/parser/analyze.c b/src/backend/parser/analyze.c
index 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.c
index 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.h
index 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.out
index 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.sql
index 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;