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

Commit8700851

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 parent908f711 commit8700851

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
@@ -334,6 +334,11 @@ transformStmt(ParseState *pstate, Node *parseTree)
334334
}
335335
#endif/* RAW_EXPRESSION_COVERAGE_TEST */
336336

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

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

@@ -441,6 +454,7 @@ analyze_requires_snapshot(RawStmt *parseTree)
441454
caseT_UpdateStmt:
442455
caseT_MergeStmt:
443456
caseT_SelectStmt:
457+
caseT_ReturnStmt:
444458
caseT_PLAssignStmt:
445459
result= true;
446460
break;
@@ -451,19 +465,43 @@ analyze_requires_snapshot(RawStmt *parseTree)
451465
caseT_DeclareCursorStmt:
452466
caseT_ExplainStmt:
453467
caseT_CreateTableAsStmt:
454-
/* yes, because we must analyze the contained statement */
468+
caseT_CallStmt:
455469
result= true;
456470
break;
457471

458472
default:
459-
/* otherutilitystatementsdon't have any real parse analysis */
473+
/*allother statementsjust get wrapped in a CMD_UTILITY Query */
460474
result= false;
461475
break;
462476
}
463477

464478
returnresult;
465479
}
466480

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

1966-
/* Never invalidatetransaction control commands */
1967-
if (IsTransactionStmtPlan(plansource))
1968+
/* Never invalidateif parse/plan would be a no-op anyway */
1969+
if (!StmtPlanRequiresRevalidation(plansource))
19681970
continue;
19691971

19701972
/*
@@ -2048,8 +2050,8 @@ PlanCacheObjectCallback(Datum arg, int cacheid, uint32 hashvalue)
20482050
if (!plansource->is_valid)
20492051
continue;
20502052

2051-
/* Never invalidatetransaction control commands */
2052-
if (IsTransactionStmtPlan(plansource))
2053+
/* Never invalidateif parse/plan would be a no-op anyway */
2054+
if (!StmtPlanRequiresRevalidation(plansource))
20532055
continue;
20542056

20552057
/*
@@ -2158,7 +2160,6 @@ ResetPlanCache(void)
21582160
{
21592161
CachedPlanSource*plansource=dlist_container(CachedPlanSource,
21602162
node,iter.cur);
2161-
ListCell*lc;
21622163

21632164
Assert(plansource->magic==CACHEDPLANSOURCE_MAGIC);
21642165

@@ -2170,32 +2171,16 @@ ResetPlanCache(void)
21702171
* We *must not* mark transaction control statements as invalid,
21712172
* particularly not ROLLBACK, because they may need to be executed in
21722173
* aborted transactions when we can't revalidate them (cf bug #5269).
2174+
* In general there's no point in invalidating statements for which a
2175+
* new parse analysis/rewrite/plan cycle would certainly give the same
2176+
* results.
21732177
*/
2174-
if (IsTransactionStmtPlan(plansource))
2178+
if (!StmtPlanRequiresRevalidation(plansource))
21752179
continue;
21762180

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

22012186
/* 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