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

Commitd8b2fcc

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 parent3da13a6 commitd8b2fcc

File tree

5 files changed

+108
-49
lines changed

5 files changed

+108
-49
lines changed

‎src/backend/parser/analyze.c

Lines changed: 45 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -335,6 +335,11 @@ transformStmt(ParseState *pstate, Node *parseTree)
335335
}
336336
#endif/* RAW_EXPRESSION_COVERAGE_TEST */
337337

338+
/*
339+
* Caution: when changing the set of statement types that have non-default
340+
* processing here, see also stmt_requires_parse_analysis() and
341+
* analyze_requires_snapshot().
342+
*/
338343
switch (nodeTag(parseTree))
339344
{
340345
/*
@@ -421,14 +426,22 @@ transformStmt(ParseState *pstate, Node *parseTree)
421426
}
422427

423428
/*
424-
* analyze_requires_snapshot
425-
*Returns true if a snapshot must be set before doing parse analysis
426-
*on the given raw parse tree.
429+
* stmt_requires_parse_analysis
430+
*Returns true if parse analysis will do anything non-trivial
431+
*with the given raw parse tree.
432+
*
433+
* Generally, this should return true for any statement type for which
434+
* transformStmt() does more than wrap a CMD_UTILITY Query around it.
435+
* When it returns false, the caller can assume that there is no situation
436+
* in which parse analysis of the raw statement could need to be re-done.
427437
*
428-
* Classification here should match transformStmt().
438+
* Currently, since the rewriter and planner do nothing for CMD_UTILITY
439+
* Queries, a false result means that the entire parse analysis/rewrite/plan
440+
* pipeline will never need to be re-done. If that ever changes, callers
441+
* will likely need adjustment.
429442
*/
430443
bool
431-
analyze_requires_snapshot(RawStmt*parseTree)
444+
stmt_requires_parse_analysis(RawStmt*parseTree)
432445
{
433446
boolresult;
434447

@@ -442,6 +455,7 @@ analyze_requires_snapshot(RawStmt *parseTree)
442455
caseT_UpdateStmt:
443456
caseT_MergeStmt:
444457
caseT_SelectStmt:
458+
caseT_ReturnStmt:
445459
caseT_PLAssignStmt:
446460
result= true;
447461
break;
@@ -452,19 +466,43 @@ analyze_requires_snapshot(RawStmt *parseTree)
452466
caseT_DeclareCursorStmt:
453467
caseT_ExplainStmt:
454468
caseT_CreateTableAsStmt:
455-
/* yes, because we must analyze the contained statement */
469+
caseT_CallStmt:
456470
result= true;
457471
break;
458472

459473
default:
460-
/* otherutilitystatementsdon't have any real parse analysis */
474+
/*allother statementsjust get wrapped in a CMD_UTILITY Query */
461475
result= false;
462476
break;
463477
}
464478

465479
returnresult;
466480
}
467481

482+
/*
483+
* analyze_requires_snapshot
484+
*Returns true if a snapshot must be set before doing parse analysis
485+
*on the given raw parse tree.
486+
*/
487+
bool
488+
analyze_requires_snapshot(RawStmt*parseTree)
489+
{
490+
/*
491+
* Currently, this should return true in exactly the same cases that
492+
* stmt_requires_parse_analysis() does, so we just invoke that function
493+
* rather than duplicating it. We keep the two entry points separate for
494+
* clarity of callers, since from the callers' standpoint these are
495+
* different conditions.
496+
*
497+
* While there may someday be a statement type for which transformStmt()
498+
* does something nontrivial and yet no snapshot is needed for that
499+
* processing, it seems likely that making such a choice would be fragile.
500+
* If you want to install an exception, document the reasoning for it in a
501+
* comment.
502+
*/
503+
returnstmt_requires_parse_analysis(parseTree);
504+
}
505+
468506
/*
469507
* transformDeleteStmt -
470508
* 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.,
@@ -383,13 +385,13 @@ CompleteCachedPlan(CachedPlanSource *plansource,
383385
plansource->query_context=querytree_context;
384386
plansource->query_list=querytree_list;
385387

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

10361038
/* Let settings force the decision */
@@ -1972,8 +1974,8 @@ PlanCacheRelCallback(Datum arg, Oid relid)
19721974
if (!plansource->is_valid)
19731975
continue;
19741976

1975-
/* Never invalidatetransaction control commands */
1976-
if (IsTransactionStmtPlan(plansource))
1977+
/* Never invalidateif parse/plan would be a no-op anyway */
1978+
if (!StmtPlanRequiresRevalidation(plansource))
19771979
continue;
19781980

19791981
/*
@@ -2057,8 +2059,8 @@ PlanCacheObjectCallback(Datum arg, int cacheid, uint32 hashvalue)
20572059
if (!plansource->is_valid)
20582060
continue;
20592061

2060-
/* Never invalidatetransaction control commands */
2061-
if (IsTransactionStmtPlan(plansource))
2062+
/* Never invalidateif parse/plan would be a no-op anyway */
2063+
if (!StmtPlanRequiresRevalidation(plansource))
20622064
continue;
20632065

20642066
/*
@@ -2167,7 +2169,6 @@ ResetPlanCache(void)
21672169
{
21682170
CachedPlanSource*plansource=dlist_container(CachedPlanSource,
21692171
node,iter.cur);
2170-
ListCell*lc;
21712172

21722173
Assert(plansource->magic==CACHEDPLANSOURCE_MAGIC);
21732174

@@ -2179,32 +2180,16 @@ ResetPlanCache(void)
21792180
* We *must not* mark transaction control statements as invalid,
21802181
* particularly not ROLLBACK, because they may need to be executed in
21812182
* aborted transactions when we can't revalidate them (cf bug #5269).
2183+
* In general there's no point in invalidating statements for which a
2184+
* new parse analysis/rewrite/plan cycle would certainly give the same
2185+
* results.
21822186
*/
2183-
if (IsTransactionStmtPlan(plansource))
2187+
if (!StmtPlanRequiresRevalidation(plansource))
21842188
continue;
21852189

2186-
/*
2187-
* In general there is no point in invalidating utility statements
2188-
* since they have no plans anyway. So invalidate it only if it
2189-
* contains at least one non-utility statement, or contains a utility
2190-
* statement that contains a pre-analyzed query (which could have
2191-
* dependencies.)
2192-
*/
2193-
foreach(lc,plansource->query_list)
2194-
{
2195-
Query*query=lfirst_node(Query,lc);
2196-
2197-
if (query->commandType!=CMD_UTILITY||
2198-
UtilityContainsQuery(query->utilityStmt))
2199-
{
2200-
/* non-utility statement, so invalidate */
2201-
plansource->is_valid= false;
2202-
if (plansource->gplan)
2203-
plansource->gplan->is_valid= false;
2204-
/* no need to look further */
2205-
break;
2206-
}
2207-
}
2190+
plansource->is_valid= false;
2191+
if (plansource->gplan)
2192+
plansource->gplan->is_valid= false;
22082193
}
22092194

22102195
/* Likewise invalidate cached expressions */

‎src/include/parser/analyze.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,7 @@ extern List *transformUpdateTargetList(ParseState *pstate,
4747
externQuery*transformTopLevelStmt(ParseState*pstate,RawStmt*parseTree);
4848
externQuery*transformStmt(ParseState*pstate,Node*parseTree);
4949

50+
externboolstmt_requires_parse_analysis(RawStmt*parseTree);
5051
externboolanalyze_requires_snapshot(RawStmt*parseTree);
5152

5253
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