8000 Avoid unnecessary plancache revalidation of utility statements. · postgres/postgres@9c59f38 · GitHub
[go: up one dir, main page]
Skip to content

Commit 9c59f38

Browse files
tglsfdctglsfdc
committed
Avoid unnecessary plancache revalidation of utility statements.
Revalidation of a plancache entry (after a cache invalidation event) requires acquiring a snapshot. Normally that is harmless, but not if the cached statement is one that needs to run without acquiring a snapshot. We were already aware of that for TransactionStmts, but for some reason hadn't extrapolated to the other statements that PlannedStmtRequiresSnapshot() knows mustn't set a snapshot. This can lead to unexpected failures of commands such as SET TRANSACTION ISOLATION LEVEL. We can fix it in the same way, by excluding those command types from revalidation. However, we can do even better than that: there is no need to revalidate for any statement type for which parse analysis, rewrite, and plan steps do nothing interesting, which is nearly all utility commands. To mechanize this, invent a parser function stmt_requires_parse_analysis() that tells whether parse analysis does anything beyond wrapping a CMD_UTILITY Query around the raw parse tree. If that's what it does, then rewrite and plan will just skip the Query, so that it is not possible for the same raw parse tree to produce a different plan tree after cache invalidation. stmt_requires_parse_analysis() is basically equivalent to the existing function analyze_requires_snapshot(), except that for obscure reasons that function omits ReturnStmt and CallStmt. It is unclear whether those were oversights or intentional. I have not been able to demonstrate a bug from not acquiring a snapshot while analyzing these commands, but at best it seems mighty fragile. It seems safer to acquire a snapshot for parse analysis of these commands too, which allows making stmt_requires_parse_analysis and analyze_requires_snapshot equivalent. In passing this fixes a second bug, which is that ResetPlanCache would exclude ReturnStmts and CallStmts from revalidation. That's surely *not* safe, since they contain parsable expressions. Per bug #18059 from Pavel Kulakov. Back-patch to all supported branches. Discussion: https://postgr.es/m/18059-79c692f036b25346@postgresql.org
1 parent 2d13dab commit 9c59f38

File tree

5 files changed

+107
-50
lines changed

5 files changed

+107
-50
lines changed

‎src/backend/parser/analyze.c

Lines changed: 44 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -274,6 +274,11 @@ transformStmt(ParseState *pstate, Node *parseTree)
274274
}
275275
#endif /* RAW_EXPRESSION_COVERAGE_TEST */
276276

277+
/*
278+
* Caution: when changing the set of statement types that have non-default
279+
* processing here, see also stmt_requires_parse_analysis() and
280+
* analyze_requires_snapshot().
281+
*/
277282
switch (nodeTag(parseTree))
278283
{
279284
/*
@@ -347,14 +352,22 @@ transformStmt(ParseState *pstate, Node *parseTree)
347352
}
348353

349354
/*
350-
* analyze_requires_snapshot
351-
* Returns true if a snapshot must be set before doing parse analysis
352-
* on the given raw parse tree.
355+
* stmt_requires_parse_analysis
356+
* Returns true if parse analysis will do anything non-trivial
357+
* with the given raw parse tree.
358+
*
359+
* Generally, this should return true for any statement type for which
360+
* transformStmt() does more than wrap a CMD_UTILITY Query around it.
361+
* When it returns false, the caller can assume that there is no situation
362+
* in which parse analysis of the raw statement could need to be re-done.
353363
*
354-
* Classification here should match transformStmt().
364+
* Currently, since the rewriter and planner do nothing for CMD_UTILITY
365+
* Queries, a false result means that the entire parse analysis/rewrite/plan
366+
* pipeline will never need to be re-done. If that ever changes, callers
367+
* will likely need adjustment.
355368
*/
356369
bool
357-
analyze_requires_snapshot(RawStmt *parseTree)
370+
stmt_requires_parse_analysis(RawStmt *parseTree)
358371
{
359372
bool result;
360373

@@ -376,19 +389,43 @@ analyze_requires_snapshot(RawStmt *parseTree)
376389
case T_DeclareCursorStmt:
377390
case T_ExplainStmt:
378391
case T_CreateTableAsStmt:
379-
/* yes, because we must analyze the contained statement */
392+
case T_CallStmt:
380393
result = true;
381394
break;
382395

383396
default:
384-
/* other utility statements don't have any real parse analysis */
397+
/* all other statements just get wrapped in a CMD_UTILITY Query */
385398
result = false;
386399
break;
387400
}
388401

389402
return result;
390403
}
391404

405+
/*
406+
* analyze_requires_snapshot
407+
* Returns true if a snapshot must be set before doing parse analysis
408+
* on the given raw parse tree.
409+
*/
410+
bool
411+
analyze_requires_snapshot(RawStmt *parseTree)
412+
{
413+
/*
414+
* Currently, this should return true in exactly the same cases that
415+
* stmt_requires_parse_analysis() does, so we just invoke that function
416+
* rather than duplicating it. We keep the two entry points separate for
417+
* clarity of callers, since from the callers' standpoint these are
418+
* different conditions.
419+
*
420+
* While there may someday be a statement type for which transformStmt()
421+
* does something nontrivial and yet no snapshot is needed for that
422+
* processing, it seems likely that making such a choice would be fragile.
423+
* If you want to install an exception, document the reasoning for it in a
424+
* comment.
425+
*/
426+
return stmt_requires_parse_analysis(parseTree);
427+
}
428+
392429
/*
393430
* transformDeleteStmt -
394431
* transforms a Delete Statement

‎src/backend/utils/cache/plancache.c

Lines changed: 27 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -73,13 +73,15 @@
7373

7474
/*
7575
* We must skip "overhead" operations that involve database access when the
76-
* cached plan's subject statement is a transaction control command.
77-
* For the convenience of postgres.c, treat empty statements as control
78-
* commands too.
76+
* cached plan's subject statement is a transaction control command or one
77+
* that requires a snapshot not to be set yet (such as SET or LOCK). More
78+
* generally, statements that do not require parse analysis/rewrite/plan
79+
* activity never need to be revalidated, so we can treat them all like that.
80+
* For the convenience of postgres.c, treat empty statements that way too.
7981
*/
80-
#define IsTransactionStmtPlan(plansource) \
81-
((plansource)->raw_parse_tree == NULL || \
82-
IsA((plansource)->raw_parse_tree->stmt, TransactionStmt))
82+
#define StmtPlanRequiresRevalidation(plansource) \
83+
((plansource)->raw_parse_tree != NULL && \
84+
stmt_requires_parse_analysis((plansource)->raw_parse_tree))
8385

8486
/*
8587
* This is the head of the backend's list of "saved" CachedPlanSources (i.e.,
@@ -371,13 +373,13 @@ CompleteCachedPlan(CachedPlanSource *plansource,
371373
plansource->query_context = querytree_context;
372374
plansource->query_list = querytree_list;
373375

374-
if (!plansource->is_oneshot && !IsTransactionStmtPlan(plansource))
376+
if (!plansource->is_oneshot && StmtPlanRequiresRevalidation(plansource))
375377
{
376378
/*
377379
* Use the planner machinery to extract dependencies. Data is saved
378380
* in query_context. (We assume that not a lot of extra cruft is
379381
* created by this call.) We can skip this for one-shot plans, and
380-
* transaction control commands have no such dependencies anyway.
382+
* plans not needing revalidation have no such dependencies anyway.
381383
*/
382384
extract_query_dependencies((Node *) querytree_list,
383385
&plansource->relationOids,
@@ -571,11 +573,11 @@ RevalidateCachedQuery(CachedPlanSource *plansource,
571573
/*
572574
* For one-shot plans, we do not support revalidation checking; it's
573575
* assumed the query is parsed, planned, and executed in one transaction,
574-
* so that no lock re-acquisition is necessary. Also, there is never any
575-
* need to revalidate plans for transaction control commands (and we
576-
* mustn't risk any catalog accesses when handling those).
576+
* so that no lock re-acquisition is necessary. Also, if the statement
577+
* type can't require revalidation, we needn't do anything (and we mustn't
578+
* risk catalog accesses when handling, eg, transaction control commands).
577579
*/
578-
if (plansource->is_oneshot || IsTransactionStmtPlan(plansource))
580+
if (plansource->is_oneshot || !StmtPlanRequiresRevalidation(plansource))
579581
{
580582
Assert(plansource->is_valid);
581583
return NIL;
@@ -1031,8 +1033,8 @@ choose_custom_plan(CachedPlanSource *plansource, ParamListInfo boundParams)
10311033
/* Otherwise, never any point in a custom plan if there's no parameters */
10321034
if (boundParams == NULL)
10331035
return false;
1034-
/* ... nor for transaction control statements */
1035-
if (IsTransactionStmtPlan(plansource))
1036+
/* ... nor when planning would be a no-op */
1037+
if (!StmtPlanRequiresRevalidation(plansource))
10361038
return false;
10371039

10381040
/* See if caller wants to force the decision */
@@ -1721,8 +1723,8 @@ PlanCacheRelCallback(Datum arg, Oid relid)
17211723
if (!plansource->is_valid)
17221724
continue;
17231725

1724-
/* Never invalidate transaction control commands */
1725-
if (IsTransactionStmtPlan(plansource))
1726+
/* Never invalidate if parse/plan would be a no-op anyway */
1727+
if (!StmtPlanRequiresRevalidation(plansource))
17261728
continue;
17271729

17281730
/*
@@ -1788,8 +1790,8 @@ PlanCacheFuncCallback(Datum arg, int cacheid, uint32 hashvalue)
17881790
if (!plansource->is_valid)
17891791
continue;
17901792

1791-
/* Never invalidate transaction control commands */
1792-
if (IsTransactionStmtPlan(plansource))
1793+
/* Never invalidate if parse/plan would be a no-op anyway */
1794+
if (!StmtPlanRequiresRevalidation(plansource))
17931795
continue;
17941796

17951797
/*
@@ -1868,8 +1870,6 @@ ResetPlanCache(void)
18681870

18691871
for (plansource = first_saved_plan; plansource; plansource = plansource->next_saved)
18701872
{
1871-
ListCell *lc;
1872-
18731873
Assert(plansource->magic == CACHEDPLANSOURCE_MAGIC);
18741874

18751875
/* No work if it's already invalidated */
@@ -1880,31 +1880,15 @@ ResetPlanCache(void)
18801880
* We *must not* mark transaction control statements as invalid,
18811881
* particularly not ROLLBACK, because they may need to be executed in
18821882
* aborted transactions when we can't revalidate them (cf bug #5269).
1883+
* In general there's no point in invalidating statements for which a
1884+
* new parse analysis/rewrite/plan cycle would certainly give the same
1885+
* results.
18831886
*/
1884-
if (IsTransactionStmtPlan(plansource))
1887+
if (!StmtPlanRequiresRevalidation(plansource))
18851888
continue;
18861889

1887-
/*
1888-
* In general there is no point in invalidating utility statements
1889-
* since they have no plans anyway. So invalidate it only if it
1890-
* contains at least one non-utility statement, or contains a utility
1891-
* statement that contains a pre-analyzed query (which could have
1892-
* dependencies.)
1893-
*/
1894-
foreach(lc, plansource->query_list)
1895-
{
1896-
Query *query = lfirst_node(Query, lc);
1897-
1898-
if (query->commandType != CMD_UTILITY ||
1899-
UtilityContainsQuery(query->utilityStmt))
1900-
{
1901-
/* non-utility statement, so invalidate */
1902-
plansource->is_valid = false;
1903-
if (plansource->gplan)
1904-
plansource->gplan->is_valid = false;
1905-
/* no need to look further */
1906-
break;
1907-
}
1908-
}
1890+
plansource->is_valid = false;
1891+
if (plansource->gplan)
1892+
plansource->gplan->is_valid = false;
19091893
}
19101894
}

‎src/include/parser/analyze.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@ extern Query *parse_sub_analyze(Node *parseTree, ParseState *parentParseState,
3535
extern Query *transformTopLevelStmt(ParseState *pstate, RawStmt *parseTree);
3636
extern Query *transformStmt(ParseState *pstate, Node *parseTree);
3737

38+
extern bool stmt_requires_parse_analysis(RawStmt *parseTree);
3839
extern bool analyze_requires_snapshot(RawStmt *parseTree);
3940

4041
extern const char *LCS_asString(LockClauseStrength strength);

‎src/pl/plpgsql/src/expected/plpgsql_call.out

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,23 @@ SELECT * FROM test1;
3535
55
3636
(1 row)
3737

38+
-- Check that plan revalidation doesn't prevent setting transaction properties
39+
-- (bug #18059). This test must include the first temp-object creation in
40+
-- this script, or it won't exercise the bug scenario. Hence we put it early.
41+
CREATE PROCEDURE test_proc3a()
42+
LANGUAGE plpgsql
43+
AS $$
44+
BEGIN
45+
COMMIT;
46+
SET TRANSACTION ISOLATION LEVEL REPEATABLE READ;
47+
RAISE NOTICE 'done';
48+
END;
49+
$$;
50+
CALL test_proc3a();
51+
NOTICE: done
52+
CREATE TEMP TABLE tt1(f1 int);
53+
CALL test_proc3a();
54+
NOTICE: done
3855
-- nested CALL
3956
TRUNCATE TABLE test1;
4057
CREATE PROCEDURE test_proc4(y int)

‎src/pl/plpgsql/src/sql/plpgsql_call.sql

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,24 @@ CALL test_proc3(55);
3838
SELECT * FROM test1;
3939

4040

41+
-- Check that plan revalidation doesn't prevent setting transaction properties
42+
-- (bug #18059). This test must include the first temp-object creation in
43+
-- this script, or it won't exercise the bug scenario. Hence we put it early.
44+
CREATE PROCEDURE test_proc3a()
45+
LANGUAGE plpgsql
46+
AS $$
47+
BEGIN
48+
COMMIT;
49+
SET TRANSACTION ISOLATION LEVEL REPEATABLE READ;
50+
RAISE NOTICE 'done';
51+
END;
52+
$$;
53+
54+
CALL test_proc3a();
55+
CREATE TEMP TABLE tt1(f1 int);
56+
CALL test_proc3a();
57+
58+
4159
-- nested CALL
4260
TRUNCATE TABLE test1;
4361

0 commit comments

Comments
 (0)
0