Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
Skip to content

Commit 6dfac24

Browse files
committed
When replanning a plpgsql "simple expression", check it's still simple.
The previous coding here assumed that we didn't need to recheck any of the querytree tests made in exec_simple_check_plan(). I think we supposed that those properties were fully determined by the syntax of the source text and hence couldn't change. That is true for most of them, but at least hasTargetSRFs and hasAggs can change by dint of forcibly dropping an originally-referenced function and recreating it with new properties. That leads to "unexpected plan node type" or similar failures. These tests are pretty cheap compared to the cost of replanning, so rather than sweat over exactly which properties need to be rechecked, let's just recheck them all. Hence, factor out those tests into a new function exec_is_simple_query(), and rearrange callers as needed. A second problem in the same area was that if we failed during replanning or during exec_save_simple_expr(), we'd potentially leave behind now-dangling pointers to the old simple expression, potentially resulting in crashes later. To fix, clear those pointers before replanning. The v12 code looks quite different in this area but still has the bug about needing to recheck query simplicity. I chose to back-patch all of the plpgsql_simple.sql test script, which formerly didn't exist in this branch. Per bug #18497 from Nikita Kalinin. Back-patch to all supported branches. Discussion: https://postgr.es/m/18497-fe93b6da82ce31d4@postgresql.org
1 parent c425113 commit 6dfac24

File tree

3 files changed

+138
-56
lines changed

3 files changed

+138
-56
lines changed

src/pl/plpgsql/src/expected/plpgsql_simple.out

+29
Original file line numberDiff line numberDiff line change
@@ -89,3 +89,32 @@ select simplecaller();
8989
4
9090
(1 row)
9191

92+
-- Check case where called function changes from non-SRF to SRF (bug #18497)
93+
create or replace function simplecaller() returns int language plpgsql
94+
as $$
95+
declare x int;
96+
begin
97+
x := simplesql();
98+
return x;
99+
end$$;
100+
select simplecaller();
101+
simplecaller
102+
--------------
103+
4
104+
(1 row)
105+
106+
drop function simplesql();
107+
create function simplesql() returns setof int language sql
108+
as $$select 22 + 22$$;
109+
select simplecaller();
110+
simplecaller
111+
--------------
112+
44
113+
(1 row)
114+
115+
select simplecaller();
116+
simplecaller
117+
--------------
118+
44
119+
(1 row)
120+

src/pl/plpgsql/src/pl_exec.c

+87-56
Original file line numberDiff line numberDiff line change
@@ -339,6 +339,7 @@ static void exec_eval_cleanup(PLpgSQL_execstate *estate);
339339
static void exec_prepare_plan(PLpgSQL_execstate *estate,
340340
PLpgSQL_expr *expr, int cursorOptions);
341341
static void exec_simple_check_plan(PLpgSQL_execstate *estate, PLpgSQL_expr *expr);
342+
static bool exec_is_simple_query(PLpgSQL_expr *expr);
342343
static void exec_save_simple_expr(PLpgSQL_expr *expr, CachedPlan *cplan);
343344
static void exec_check_rw_parameter(PLpgSQL_expr *expr);
344345
static void exec_check_assignable(PLpgSQL_execstate *estate, int dno);
@@ -6094,12 +6095,18 @@ exec_eval_simple_expr(PLpgSQL_execstate *estate,
60946095
* release it, so we don't leak plans intra-transaction.
60956096
*/
60966097
if (expr->expr_simple_plan_lxid == curlxid)
6097-
{
60986098
ReleaseCachedPlan(expr->expr_simple_plan,
60996099
estate->simple_eval_resowner);
6100-
expr->expr_simple_plan = NULL;
6101-
expr->expr_simple_plan_lxid = InvalidLocalTransactionId;
6102-
}
6100+
6101+
/*
6102+
* Reset to "not simple" to leave sane state (with no dangling
6103+
* pointers) in case we fail while replanning. expr_simple_plansource
6104+
* can be left alone however, as that cannot move.
6105+
*/
6106+
expr->expr_simple_expr = NULL;
6107+
expr->expr_rw_param = NULL;
6108+
expr->expr_simple_plan = NULL;
6109+
expr->expr_simple_plan_lxid = InvalidLocalTransactionId;
61036110

61046111
/* Do the replanning work in the eval_mcontext */
61056112
oldcontext = MemoryContextSwitchTo(get_eval_mcontext(estate));
@@ -6115,11 +6122,15 @@ exec_eval_simple_expr(PLpgSQL_execstate *estate,
61156122
Assert(cplan != NULL);
61166123

61176124
/*
6118-
* This test probably can't fail either, but if it does, cope by
6119-
* declaring the plan to be non-simple. On success, we'll acquire a
6120-
* refcount on the new plan, stored in simple_eval_resowner.
6125+
* Recheck exec_is_simple_query, which could now report false in
6126+
* edge-case scenarios such as a non-SRF having been replaced with a
6127+
* SRF. Also recheck CachedPlanAllowsSimpleValidityCheck, just to be
6128+
* sure. If either test fails, cope by declaring the plan to be
6129+
* non-simple. On success, we'll acquire a refcount on the new plan,
6130+
* stored in simple_eval_resowner.
61216131
*/
6122-
if (CachedPlanAllowsSimpleValidityCheck(expr->expr_simple_plansource,
6132+
if (exec_is_simple_query(expr) &&
6133+
CachedPlanAllowsSimpleValidityCheck(expr->expr_simple_plansource,
61236134
cplan,
61246135
estate->simple_eval_resowner))
61256136
{
@@ -6131,9 +6142,6 @@ exec_eval_simple_expr(PLpgSQL_execstate *estate,
61316142
{
61326143
/* Release SPI_plan_get_cached_plan's refcount */
61336144
ReleaseCachedPlan(cplan, CurrentResourceOwner);
6134-
/* Mark expression as non-simple, and fail */
6135-
expr->expr_simple_expr = NULL;
6136-
expr->expr_rw_param = NULL;
61376145
return false;
61386146
}
61396147

@@ -7974,7 +7982,6 @@ exec_simple_check_plan(PLpgSQL_execstate *estate, PLpgSQL_expr *expr)
79747982
{
79757983
List *plansources;
79767984
CachedPlanSource *plansource;
7977-
Query *query;
79787985
CachedPlan *cplan;
79797986
MemoryContext oldcontext;
79807987

@@ -7990,31 +7997,88 @@ exec_simple_check_plan(PLpgSQL_execstate *estate, PLpgSQL_expr *expr)
79907997
* called immediately after creating the CachedPlanSource, we need not
79917998
* worry about the query being stale.
79927999
*/
8000+
if (!exec_is_simple_query(expr))
8001+
return;
8002+
8003+
/* exec_is_simple_query verified that there's just one CachedPlanSource */
8004+
plansources = SPI_plan_get_plan_sources(expr->plan);
8005+
plansource = (CachedPlanSource *) linitial(plansources);
79938006

79948007
/*
7995-
* We can only test queries that resulted in exactly one CachedPlanSource
8008+
* Get the generic plan for the query. If replanning is needed, do that
8009+
* work in the eval_mcontext. (Note that replanning could throw an error,
8010+
* in which case the expr is left marked "not simple", which is fine.)
8011+
*/
8012+
oldcontext = MemoryContextSwitchTo(get_eval_mcontext(estate));
8013+
cplan = SPI_plan_get_cached_plan(expr->plan);
8014+
MemoryContextSwitchTo(oldcontext);
8015+
8016+
/* Can't fail, because we checked for a single CachedPlanSource above */
8017+
Assert(cplan != NULL);
8018+
8019+
/*
8020+
* Verify that plancache.c thinks the plan is simple enough to use
8021+
* CachedPlanIsSimplyValid. Given the restrictions above, it's unlikely
8022+
* that this could fail, but if it does, just treat plan as not simple. On
8023+
* success, save a refcount on the plan in the simple-expression resowner.
8024+
*/
8025+
if (CachedPlanAllowsSimpleValidityCheck(plansource, cplan,
8026+
estate->simple_eval_resowner))
8027+
{
8028+
/* Remember that we have the refcount */
8029+
expr->expr_simple_plansource = plansource;
8030+
expr->expr_simple_plan = cplan;
8031+
expr->expr_simple_plan_lxid = MyProc->vxid.lxid;
8032+
8033+
/* Share the remaining work with the replan code path */
8034+
exec_save_simple_expr(expr, cplan);
8035+
}
8036+
8037+
/*
8038+
* Release the plan refcount obtained by SPI_plan_get_cached_plan. (This
8039+
* refcount is held by the wrong resowner, so we can't just repurpose it.)
8040+
*/
8041+
ReleaseCachedPlan(cplan, CurrentResourceOwner);
8042+
}
8043+
8044+
/*
8045+
* exec_is_simple_query - precheck a query tree to see if it might be simple
8046+
*
8047+
* Check the analyzed-and-rewritten form of a query to see if we will be
8048+
* able to treat it as a simple expression. It is caller's responsibility
8049+
* that the CachedPlanSource be up-to-date.
8050+
*/
8051+
static bool
8052+
exec_is_simple_query(PLpgSQL_expr *expr)
8053+
{
8054+
List *plansources;
8055+
CachedPlanSource *plansource;
8056+
Query *query;
8057+
8058+
/*
8059+
* We can only test queries that resulted in exactly one CachedPlanSource.
79968060
*/
79978061
plansources = SPI_plan_get_plan_sources(expr->plan);
79988062
if (list_length(plansources) != 1)
7999-
return;
8063+
return false;
80008064
plansource = (CachedPlanSource *) linitial(plansources);
80018065

80028066
/*
80038067
* 1. There must be one single querytree.
80048068
*/
80058069
if (list_length(plansource->query_list) != 1)
8006-
return;
8070+
return false;
80078071
query = (Query *) linitial(plansource->query_list);
80088072

80098073
/*
8010-
* 2. It must be a plain SELECT query without any input tables
8074+
* 2. It must be a plain SELECT query without any input tables.
80118075
*/
80128076
if (!IsA(query, Query))
8013-
return;
8077+
return false;
80148078
if (query->commandType != CMD_SELECT)
8015-
return;
8079+
return false;
80168080
if (query->rtable != NIL)
8017-
return;
8081+
return false;
80188082

80198083
/*
80208084
* 3. Can't have any subplans, aggregates, qual clauses either. (These
@@ -8038,51 +8102,18 @@ exec_simple_check_plan(PLpgSQL_execstate *estate, PLpgSQL_expr *expr)
80388102
query->limitOffset ||
80398103
query->limitCount ||
80408104
query->setOperations)
8041-
return;
8105+
return false;
80428106

80438107
/*
8044-
* 4. The query must have a single attribute as result
8108+
* 4. The query must have a single attribute as result.
80458109
*/
80468110
if (list_length(query->targetList) != 1)
8047-
return;
8111+
return false;
80488112

80498113
/*
80508114
* OK, we can treat it as a simple plan.
8051-
*
8052-
* Get the generic plan for the query. If replanning is needed, do that
8053-
* work in the eval_mcontext. (Note that replanning could throw an error,
8054-
* in which case the expr is left marked "not simple", which is fine.)
8055-
*/
8056-
oldcontext = MemoryContextSwitchTo(get_eval_mcontext(estate));
8057-
cplan = SPI_plan_get_cached_plan(expr->plan);
8058-
MemoryContextSwitchTo(oldcontext);
8059-
8060-
/* Can't fail, because we checked for a single CachedPlanSource above */
8061-
Assert(cplan != NULL);
8062-
8063-
/*
8064-
* Verify that plancache.c thinks the plan is simple enough to use
8065-
* CachedPlanIsSimplyValid. Given the restrictions above, it's unlikely
8066-
* that this could fail, but if it does, just treat plan as not simple. On
8067-
* success, save a refcount on the plan in the simple-expression resowner.
8068-
*/
8069-
if (CachedPlanAllowsSimpleValidityCheck(plansource, cplan,
8070-
estate->simple_eval_resowner))
8071-
{
8072-
/* Remember that we have the refcount */
8073-
expr->expr_simple_plansource = plansource;
8074-
expr->expr_simple_plan = cplan;
8075-
expr->expr_simple_plan_lxid = MyProc->vxid.lxid;
8076-
8077-
/* Share the remaining work with the replan code path */
8078-
exec_save_simple_expr(expr, cplan);
8079-
}
8080-
8081-
/*
8082-
* Release the plan refcount obtained by SPI_plan_get_cached_plan. (This
8083-
* refcount is held by the wrong resowner, so we can't just repurpose it.)
80848115
*/
8085-
ReleaseCachedPlan(cplan, CurrentResourceOwner);
8116+
return true;
80868117
}
80878118

80888119
/*

src/pl/plpgsql/src/sql/plpgsql_simple.sql

+22
Original file line numberDiff line numberDiff line change
@@ -80,3 +80,25 @@ create or replace function simplesql() returns int language sql
8080
as $$select 2 + 2$$;
8181

8282
select simplecaller();
83+
84+
85+
-- Check case where called function changes from non-SRF to SRF (bug #18497)
86+
87+
create or replace function simplecaller() returns int language plpgsql
88+
as $$
89+
declare x int;
90+
begin
91+
x := simplesql();
92+
return x;
93+
end$$;
94+
95+
select simplecaller();
96+
97+
drop function simplesql();
98+
99+
create function simplesql() returns setof int language sql
100+
as $$select 22 + 22$$;
101+
102+
select simplecaller();
103+
104+
select simplecaller();

0 commit comments

Comments
 (0)