Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Commit27566bc

Browse files
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 notif the cached statement is one that needs to run without acquiring asnapshot. We were already aware of that for TransactionStmts,but for some reason hadn't extrapolated to the other statements thatPlannedStmtRequiresSnapshot() knows mustn't set a snapshot. This canlead to unexpected failures of commands such as SET TRANSACTIONISOLATION LEVEL. We can fix it in the same way, by excluding thosecommand types from revalidation.However, we can do even better than that: there is no need torevalidate for any statement type for which parse analysis, rewrite,and plan steps do nothing interesting, which is nearly all utilitycommands. To mechanize this, invent a parser functionstmt_requires_parse_analysis() that tells whether parse analysis doesanything beyond wrapping a CMD_UTILITY Query around the raw parsetree. If that's what it does, then rewrite and plan will justskip the Query, so that it is not possible for the same raw parsetree to produce a different plan tree after cache invalidation.stmt_requires_parse_analysis() is basically equivalent to theexisting function analyze_requires_snapshot(), except that forobscure 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 asnapshot while analyzing these commands, but at best it seems mightyfragile. It seems safer to acquire a snapshot for parse analysis ofthese commands too, which allows making stmt_requires_parse_analysisand analyze_requires_snapshot equivalent.In passing this fixes a second bug, which is that ResetPlanCachewould 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 supportedbranches.Discussion:https://postgr.es/m/18059-79c692f036b25346@postgresql.org
1 parent07e7014 commit27566bc

File tree

5 files changed

+107
-49
lines changed

5 files changed

+107
-49
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
boolresult;
360373

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

383396
default:
384-
/* otherutilitystatementsdon't have any real parse analysis */
397+
/*allother statementsjust get wrapped in a CMD_UTILITY Query */
385398
result= false;
386399
break;
387400
}
388401

389402
returnresult;
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+
returnstmt_requires_parse_analysis(parseTree);
427+
}
428+
392429
/*
393430
* transformDeleteStmt -
394431
* transforms a Delete Statement

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

Lines changed: 27 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -77,13 +77,15 @@
7777

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

8890
/*
8991
* This is the head of the backend's list of "saved" CachedPlanSources (i.e.,
@@ -381,13 +383,13 @@ CompleteCachedPlan(CachedPlanSource *plansource,
381383
plansource->query_context=querytree_context;
382384
plansource->query_list=querytree_list;
383385

384-
if (!plansource->is_oneshot&&!IsTransactionStmtPlan(plansource))
386+
if (!plansource->is_oneshot&&StmtPlanRequiresRevalidation(plansource))
385387
{
386388
/*
387389
* Use the planner machinery to extract dependencies. Data is saved
388390
* in query_context. (We assume that not a lot of extra cruft is
389391
* created by this call.) We can skip this for one-shot plans, and
390-
*transaction control commands have no such dependencies anyway.
392+
*plans not needing revalidation have no such dependencies anyway.
391393
*/
392394
extract_query_dependencies((Node*)querytree_list,
393395
&plansource->relationOids,
@@ -566,11 +568,11 @@ RevalidateCachedQuery(CachedPlanSource *plansource,
566568
/*
567569
* For one-shot plans, we do not support revalidation checking; it's
568570
* assumed the query is parsed, planned, and executed in one transaction,
569-
* so that no lock re-acquisition is necessary. Also,there is never any
570-
*need to revalidate plans for transaction control commands (and we
571-
*mustn'triskanycatalog accesses when handling those).
571+
* so that no lock re-acquisition is necessary. Also,if the statement
572+
*type can't require revalidation, we needn't do anything (and we mustn't
573+
* risk catalog accesses when handling, eg, transaction control commands).
572574
*/
573-
if (plansource->is_oneshot||IsTransactionStmtPlan(plansource))
575+
if (plansource->is_oneshot||!StmtPlanRequiresRevalidation(plansource))
574576
{
575577
Assert(plansource->is_valid);
576578
returnNIL;
@@ -1027,8 +1029,8 @@ choose_custom_plan(CachedPlanSource *plansource, ParamListInfo boundParams)
10271029
/* Otherwise, never any point in a custom plan if there's no parameters */
10281030
if (boundParams==NULL)
10291031
return false;
1030-
/* ... norfor transaction control statements */
1031-
if (IsTransactionStmtPlan(plansource))
1032+
/* ... norwhen planning would be a no-op */
1033+
if (!StmtPlanRequiresRevalidation(plansource))
10321034
return false;
10331035

10341036
/* Let settings force the decision */
@@ -1958,8 +1960,8 @@ PlanCacheRelCallback(Datum arg, Oid relid)
19581960
if (!plansource->is_valid)
19591961
continue;
19601962

1961-
/* Never invalidatetransaction control commands */
1962-
if (IsTransactionStmtPlan(plansource))
1963+
/* Never invalidateif parse/plan would be a no-op anyway */
1964+
if (!StmtPlanRequiresRevalidation(plansource))
19631965
continue;
19641966

19651967
/*
@@ -2043,8 +2045,8 @@ PlanCacheObjectCallback(Datum arg, int cacheid, uint32 hashvalue)
20432045
if (!plansource->is_valid)
20442046
continue;
20452047

2046-
/* Never invalidatetransaction control commands */
2047-
if (IsTransactionStmtPlan(plansource))
2048+
/* Never invalidateif parse/plan would be a no-op anyway */
2049+
if (!StmtPlanRequiresRevalidation(plansource))
20482050
continue;
20492051

20502052
/*
@@ -2153,7 +2155,6 @@ ResetPlanCache(void)
21532155
{
21542156
CachedPlanSource*plansource=dlist_container(CachedPlanSource,
21552157
node,iter.cur);
2156-
ListCell*lc;
21572158

21582159
Assert(plansource->magic==CACHEDPLANSOURCE_MAGIC);
21592160

@@ -2165,32 +2166,16 @@ ResetPlanCache(void)
21652166
* We *must not* mark transaction control statements as invalid,
21662167
* particularly not ROLLBACK, because they may need to be executed in
21672168
* aborted transactions when we can't revalidate them (cf bug #5269).
2169+
* In general there's no point in invalidating statements for which a
2170+
* new parse analysis/rewrite/plan cycle would certainly give the same
2171+
* results.
21682172
*/
2169-
if (IsTransactionStmtPlan(plansource))
2173+
if (!StmtPlanRequiresRevalidation(plansource))
21702174
continue;
21712175

2172-
/*
2173-
* In general there is no point in invalidating utility statements
2174-
* since they have no plans anyway. So invalidate it only if it
2175-
* contains at least one non-utility statement, or contains a utility
2176-
* statement that contains a pre-analyzed query (which could have
2177-
* dependencies.)
2178-
*/
2179-
foreach(lc,plansource->query_list)
2180-
{
2181-
Query*query=lfirst_node(Query,lc);
2182-
2183-
if (query->commandType!=CMD_UTILITY||
2184-
UtilityContainsQuery(query->utilityStmt))
2185-
{
2186-
/* non-utility statement, so invalidate */
2187-
plansource->is_valid= false;
2188-
if (plansource->gplan)
2189-
plansource->gplan->is_valid= false;
2190-
/* no need to look further */
2191-
break;
2192-
}
2193-
}
2176+
plansource->is_valid= false;
2177+
if (plansource->gplan)
2178+
plansource->gplan->is_valid= false;
21942179
}
21952180

21962181
/* Likewise invalidate cached expressions */

‎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
externQuery*transformTopLevelStmt(ParseState*pstate,RawStmt*parseTree);
3636
externQuery*transformStmt(ParseState*pstate,Node*parseTree);
3737

38+
externboolstmt_requires_parse_analysis(RawStmt*parseTree);
3839
externboolanalyze_requires_snapshot(RawStmt*parseTree);
3940

4041
externconstchar*LCS_asString(LockClauseStrengthstrength);

‎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(f1int);
56+
CALL test_proc3a();
57+
58+
4159
-- nested CALL
4260
TRUNCATE TABLE test1;
4361

0 commit comments

Comments
 (0)
[8]ページ先頭
©2009-2025 Movatter.jp