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

Commit1c1cbe2

Browse files
committed
Rethink the "read/write parameter" mechanism in pl/pgsql.
Performance issues with the preceding patch to re-implement arrayelement assignment within pl/pgsql led me to realize that the read/writeparameter mechanism is misdesigned. Instead of requiring the assignmentsource expression to be such that *all* its references to the targetvariable could be passed as R/W, we really want to identify *one*reference to the target variable to be passed as R/W, allowing any otherones to be passed read/only as they would be by default. As long as theR/W reference is a direct argument to the top-level (hence last to beexecuted) function in the expression, there is no harm in R/O referencesbeing passed to other lower parts of the expression. Nor is there anyuse-case for more than one argument of the top-level function being R/W.Hence, rewrite that logic to identify one single Param that referencesthe target variable, and make only that Param pass a read/writereference, not any other Params referencing the target variable.Discussion:https://postgr.es/m/4165684.1607707277@sss.pgh.pa.us
1 parent1788828 commit1c1cbe2

File tree

4 files changed

+106
-96
lines changed

4 files changed

+106
-96
lines changed

‎src/backend/utils/adt/arrayfuncs.c

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2582,8 +2582,11 @@ array_set_element_expanded(Datum arraydatum,
25822582

25832583
/*
25842584
* Copy new element into array's context, if needed (we assume it's
2585-
* already detoasted, so no junk should be created). If we fail further
2586-
* down, this memory is leaked, but that's reasonably harmless.
2585+
* already detoasted, so no junk should be created). Doing this before
2586+
* we've made any significant changes ensures that our behavior is sane
2587+
* even when the source is a reference to some element of this same array.
2588+
* If we fail further down, this memory is leaked, but that's reasonably
2589+
* harmless.
25872590
*/
25882591
if (!eah->typbyval&& !isNull)
25892592
{

‎src/pl/plpgsql/src/pl_exec.c

Lines changed: 87 additions & 90 deletions
Original file line numberDiff line numberDiff line change
@@ -333,8 +333,7 @@ static void exec_prepare_plan(PLpgSQL_execstate *estate,
333333
boolkeepplan);
334334
staticvoidexec_simple_check_plan(PLpgSQL_execstate*estate,PLpgSQL_expr*expr);
335335
staticvoidexec_save_simple_expr(PLpgSQL_expr*expr,CachedPlan*cplan);
336-
staticvoidexec_check_rw_parameter(PLpgSQL_expr*expr,inttarget_dno);
337-
staticboolcontains_target_param(Node*node,int*target_dno);
336+
staticvoidexec_check_rw_parameter(PLpgSQL_expr*expr);
338337
staticboolexec_eval_simple_expr(PLpgSQL_execstate*estate,
339338
PLpgSQL_expr*expr,
340339
Datum*result,
@@ -4190,13 +4189,6 @@ exec_prepare_plan(PLpgSQL_execstate *estate,
41904189

41914190
/* Check to see if it's a simple expression */
41924191
exec_simple_check_plan(estate,expr);
4193-
4194-
/*
4195-
* Mark expression as not using a read-write param. exec_assign_value has
4196-
* to take steps to override this if appropriate; that seems cleaner than
4197-
* adding parameters to all other callers.
4198-
*/
4199-
expr->rwparam=-1;
42004192
}
42014193

42024194

@@ -5024,16 +5016,23 @@ exec_assign_expr(PLpgSQL_execstate *estate, PLpgSQL_datum *target,
50245016
int32valtypmod;
50255017

50265018
/*
5027-
* If first time through, create a plan for this expression, and then see
5028-
* if we can pass the target variable as a read-write parameter to the
5029-
* expression. (This is a bit messy, but it seems cleaner than modifying
5030-
* the API of exec_eval_expr for the purpose.)
5019+
* If first time through, create a plan for this expression.
50315020
*/
50325021
if (expr->plan==NULL)
50335022
{
5034-
exec_prepare_plan(estate,expr,0, true);
5023+
/*
5024+
* Mark the expression as being an assignment source, if target is a
5025+
* simple variable. (This is a bit messy, but it seems cleaner than
5026+
* modifying the API of exec_prepare_plan for the purpose. We need to
5027+
* stash the target dno into the expr anyway, so that it will be
5028+
* available if we have to replan.)
5029+
*/
50355030
if (target->dtype==PLPGSQL_DTYPE_VAR)
5036-
exec_check_rw_parameter(expr,target->dno);
5031+
expr->target_param=target->dno;
5032+
else
5033+
expr->target_param=-1;/* should be that already */
5034+
5035+
exec_prepare_plan(estate,expr,0, true);
50375036
}
50385037

50395038
value=exec_eval_expr(estate,expr,&isnull,&valtype,&valtypmod);
@@ -6098,6 +6097,7 @@ exec_eval_simple_expr(PLpgSQL_execstate *estate,
60986097
ReleaseCachedPlan(cplan, true);
60996098
/* Mark expression as non-simple, and fail */
61006099
expr->expr_simple_expr=NULL;
6100+
expr->expr_rw_param=NULL;
61016101
return false;
61026102
}
61036103

@@ -6109,10 +6109,6 @@ exec_eval_simple_expr(PLpgSQL_execstate *estate,
61096109

61106110
/* Extract desired scalar expression from cached plan */
61116111
exec_save_simple_expr(expr,cplan);
6112-
6113-
/* better recheck r/w safety, as it could change due to inlining */
6114-
if (expr->rwparam >=0)
6115-
exec_check_rw_parameter(expr,expr->rwparam);
61166112
}
61176113

61186114
/*
@@ -6385,20 +6381,18 @@ plpgsql_param_fetch(ParamListInfo params,
63856381
prm->pflags=PARAM_FLAG_CONST;
63866382

63876383
/*
6388-
* If it's a read/write expanded datum, convert reference to read-only,
6389-
* unless it's safe to pass as read-write.
6384+
* If it's a read/write expanded datum, convert reference to read-only.
6385+
* (There's little point in trying to optimize read/write parameters,
6386+
* given the cases in which this function is used.)
63906387
*/
6391-
if (dno!=expr->rwparam)
6392-
{
6393-
if (datum->dtype==PLPGSQL_DTYPE_VAR)
6394-
prm->value=MakeExpandedObjectReadOnly(prm->value,
6395-
prm->isnull,
6396-
((PLpgSQL_var*)datum)->datatype->typlen);
6397-
elseif (datum->dtype==PLPGSQL_DTYPE_REC)
6398-
prm->value=MakeExpandedObjectReadOnly(prm->value,
6399-
prm->isnull,
6400-
-1);
6401-
}
6388+
if (datum->dtype==PLPGSQL_DTYPE_VAR)
6389+
prm->value=MakeExpandedObjectReadOnly(prm->value,
6390+
prm->isnull,
6391+
((PLpgSQL_var*)datum)->datatype->typlen);
6392+
elseif (datum->dtype==PLPGSQL_DTYPE_REC)
6393+
prm->value=MakeExpandedObjectReadOnly(prm->value,
6394+
prm->isnull,
6395+
-1);
64026396

64036397
returnprm;
64046398
}
@@ -6441,7 +6435,7 @@ plpgsql_param_compile(ParamListInfo params, Param *param,
64416435
*/
64426436
if (datum->dtype==PLPGSQL_DTYPE_VAR)
64436437
{
6444-
if (dno!=expr->rwparam&&
6438+
if (param!=expr->expr_rw_param&&
64456439
((PLpgSQL_var*)datum)->datatype->typlen==-1)
64466440
scratch.d.cparam.paramfunc=plpgsql_param_eval_var_ro;
64476441
else
@@ -6451,14 +6445,14 @@ plpgsql_param_compile(ParamListInfo params, Param *param,
64516445
scratch.d.cparam.paramfunc=plpgsql_param_eval_recfield;
64526446
elseif (datum->dtype==PLPGSQL_DTYPE_PROMISE)
64536447
{
6454-
if (dno!=expr->rwparam&&
6448+
if (param!=expr->expr_rw_param&&
64556449
((PLpgSQL_var*)datum)->datatype->typlen==-1)
64566450
scratch.d.cparam.paramfunc=plpgsql_param_eval_generic_ro;
64576451
else
64586452
scratch.d.cparam.paramfunc=plpgsql_param_eval_generic;
64596453
}
64606454
elseif (datum->dtype==PLPGSQL_DTYPE_REC&&
6461-
dno!=expr->rwparam)
6455+
param!=expr->expr_rw_param)
64626456
scratch.d.cparam.paramfunc=plpgsql_param_eval_generic_ro;
64636457
else
64646458
scratch.d.cparam.paramfunc=plpgsql_param_eval_generic;
@@ -7930,6 +7924,7 @@ exec_simple_check_plan(PLpgSQL_execstate *estate, PLpgSQL_expr *expr)
79307924
* Initialize to "not simple".
79317925
*/
79327926
expr->expr_simple_expr=NULL;
7927+
expr->expr_rw_param=NULL;
79337928

79347929
/*
79357930
* Check the analyzed-and-rewritten form of the query to see if we will be
@@ -8108,6 +8103,12 @@ exec_save_simple_expr(PLpgSQL_expr *expr, CachedPlan *cplan)
81088103
expr->expr_simple_typmod=exprTypmod((Node*)tle_expr);
81098104
/* We also want to remember if it is immutable or not */
81108105
expr->expr_simple_mutable=contain_mutable_functions((Node*)tle_expr);
8106+
8107+
/*
8108+
* Lastly, check to see if there's a possibility of optimizing a
8109+
* read/write parameter.
8110+
*/
8111+
exec_check_rw_parameter(expr);
81118112
}
81128113

81138114
/*
@@ -8119,25 +8120,36 @@ exec_save_simple_expr(PLpgSQL_expr *expr, CachedPlan *cplan)
81198120
* value as a read/write pointer and let the function modify the value
81208121
* in-place.
81218122
*
8122-
* This function checks for a safe expression, and sets expr->rwparam to the
8123-
* dno of the target variable (x) if safe, or -1 if not safe.
8123+
* This function checks for a safe expression, and sets expr->expr_rw_param
8124+
* to the address of any Param within the expression that can be passed as
8125+
* read/write (there can be only one); or to NULL when there is no safe Param.
8126+
*
8127+
* Note that this mechanism intentionally applies the safety labeling to just
8128+
* one Param; the expression could contain other Params referencing the target
8129+
* variable, but those must still be treated as read-only.
8130+
*
8131+
* Also note that we only apply this optimization within simple expressions.
8132+
* There's no point in it for non-simple expressions, because the
8133+
* exec_run_select code path will flatten any expanded result anyway.
8134+
* Also, it's safe to assume that an expr_simple_expr tree won't get copied
8135+
* somewhere before it gets compiled, so that looking for pointer equality
8136+
* to expr_rw_param will work for matching the target Param. That'd be much
8137+
* shakier in the general case.
81248138
*/
81258139
staticvoid
8126-
exec_check_rw_parameter(PLpgSQL_expr*expr,inttarget_dno)
8140+
exec_check_rw_parameter(PLpgSQL_expr*expr)
81278141
{
8142+
inttarget_dno;
81288143
Oidfuncid;
81298144
List*fargs;
81308145
ListCell*lc;
81318146

81328147
/* Assume unsafe */
8133-
expr->rwparam=-1;
8148+
expr->expr_rw_param=NULL;
81348149

8135-
/*
8136-
* If the expression isn't simple, there's no point in trying to optimize
8137-
* (because the exec_run_select code path will flatten any expanded result
8138-
* anyway). Even without that, this seems like a good safety restriction.
8139-
*/
8140-
if (expr->expr_simple_expr==NULL)
8150+
/* Done if expression isn't an assignment source */
8151+
target_dno=expr->target_param;
8152+
if (target_dno<0)
81418153
return;
81428154

81438155
/*
@@ -8147,9 +8159,12 @@ exec_check_rw_parameter(PLpgSQL_expr *expr, int target_dno)
81478159
if (!bms_is_member(target_dno,expr->paramnos))
81488160
return;
81498161

8162+
/* Shouldn't be here for non-simple expression */
8163+
Assert(expr->expr_simple_expr!=NULL);
8164+
81508165
/*
81518166
* Top level of expression must be a simple FuncExpr, OpExpr, or
8152-
* SubscriptingRef.
8167+
* SubscriptingRef, else we can't optimize.
81538168
*/
81548169
if (IsA(expr->expr_simple_expr,FuncExpr))
81558170
{
@@ -8174,22 +8189,20 @@ exec_check_rw_parameter(PLpgSQL_expr *expr, int target_dno)
81748189
F_ARRAY_SUBSCRIPT_HANDLER)
81758190
return;
81768191

8177-
/*refexpr canbe a simple Param, otherwise must not contain target */
8178-
if (!(sbsref->refexpr&&IsA(sbsref->refexpr,Param))&&
8179-
contains_target_param((Node*)sbsref->refexpr,&target_dno))
8180-
return;
8192+
/*We canoptimize the refexpr if it's the target, otherwise not */
8193+
if (sbsref->refexpr&&IsA(sbsref->refexpr,Param))
8194+
{
8195+
Param*param= (Param*)sbsref->refexpr;
81818196

8182-
/* the other subexpressions must not contain target */
8183-
if (contains_target_param((Node*)sbsref->refupperindexpr,
8184-
&target_dno)||
8185-
contains_target_param((Node*)sbsref->reflowerindexpr,
8186-
&target_dno)||
8187-
contains_target_param((Node*)sbsref->refassgnexpr,
8188-
&target_dno))
8189-
return;
8197+
if (param->paramkind==PARAM_EXTERN&&
8198+
param->paramid==target_dno+1)
8199+
{
8200+
/* Found the Param we want to pass as read/write */
8201+
expr->expr_rw_param=param;
8202+
return;
8203+
}
8204+
}
81908205

8191-
/* OK, we can pass target as a read-write parameter */
8192-
expr->rwparam=target_dno;
81938206
return;
81948207
}
81958208
else
@@ -8205,44 +8218,28 @@ exec_check_rw_parameter(PLpgSQL_expr *expr, int target_dno)
82058218
return;
82068219

82078220
/*
8208-
* The target variable (in the form of a Param) must only appear as a
8209-
* direct argument of the top-level function.
8221+
* The target variable (in the form of a Param) must appear as a direct
8222+
* argument of the top-level function. References further down in the
8223+
* tree can't be optimized; but on the other hand, they don't invalidate
8224+
* optimizing the top-level call, since that will be executed last.
82108225
*/
82118226
foreach(lc,fargs)
82128227
{
82138228
Node*arg= (Node*)lfirst(lc);
82148229

8215-
/* A Param is OK, whether it's the target variable or not */
82168230
if (arg&&IsA(arg,Param))
8217-
continue;
8218-
/* Otherwise, argument expression must not reference target */
8219-
if (contains_target_param(arg,&target_dno))
8220-
return;
8221-
}
8222-
8223-
/* OK, we can pass target as a read-write parameter */
8224-
expr->rwparam=target_dno;
8225-
}
8226-
8227-
/*
8228-
* Recursively check for a Param referencing the target variable
8229-
*/
8230-
staticbool
8231-
contains_target_param(Node*node,int*target_dno)
8232-
{
8233-
if (node==NULL)
8234-
return false;
8235-
if (IsA(node,Param))
8236-
{
8237-
Param*param= (Param*)node;
8231+
{
8232+
Param*param= (Param*)arg;
82388233

8239-
if (param->paramkind==PARAM_EXTERN&&
8240-
param->paramid==*target_dno+1)
8241-
return true;
8242-
return false;
8234+
if (param->paramkind==PARAM_EXTERN&&
8235+
param->paramid==target_dno+1)
8236+
{
8237+
/* Found the Param we want to pass as read/write */
8238+
expr->expr_rw_param=param;
8239+
return;
8240+
}
8241+
}
82438242
}
8244-
returnexpression_tree_walker(node,contains_target_param,
8245-
(void*)target_dno);
82468243
}
82478244

82488245
/* ----------

‎src/pl/plpgsql/src/pl_gram.y

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2820,7 +2820,7 @@ read_sql_construct(int until,
28202820
expr->parseMode= parsemode;
28212821
expr->plan= NULL;
28222822
expr->paramnos= NULL;
2823-
expr->rwparam= -1;
2823+
expr->target_param= -1;
28242824
expr->ns= plpgsql_ns_top();
28252825
pfree(ds.data);
28262826

@@ -3067,7 +3067,7 @@ make_execsql_stmt(int firsttoken, int location)
30673067
expr->parseMode= RAW_PARSE_DEFAULT;
30683068
expr->plan= NULL;
30693069
expr->paramnos= NULL;
3070-
expr->rwparam= -1;
3070+
expr->target_param= -1;
30713071
expr->ns= plpgsql_ns_top();
30723072
pfree(ds.data);
30733073

@@ -3949,7 +3949,7 @@ read_cursor_args(PLpgSQL_var *cursor, int until)
39493949
expr->parseMode= RAW_PARSE_PLPGSQL_EXPR;
39503950
expr->plan= NULL;
39513951
expr->paramnos= NULL;
3952-
expr->rwparam= -1;
3952+
expr->target_param= -1;
39533953
expr->ns = plpgsql_ns_top();
39543954
pfree(ds.data);
39553955

‎src/pl/plpgsql/src/plpgsql.h

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -221,7 +221,6 @@ typedef struct PLpgSQL_expr
221221
RawParseModeparseMode;/* raw_parser() mode to use */
222222
SPIPlanPtrplan;/* plan, or NULL if not made yet */
223223
Bitmapset*paramnos;/* all dnos referenced by this query */
224-
intrwparam;/* dno of read/write param, or -1 if none */
225224

226225
/* function containing this expr (not set until we first parse query) */
227226
structPLpgSQL_function*func;
@@ -235,6 +234,17 @@ typedef struct PLpgSQL_expr
235234
int32expr_simple_typmod;/* result typmod, if simple */
236235
boolexpr_simple_mutable;/* true if simple expr is mutable */
237236

237+
/*
238+
* These fields are used to optimize assignments to expanded-datum
239+
* variables. If this expression is the source of an assignment to a
240+
* simple variable, target_param holds that variable's dno; else it's -1.
241+
* If we match a Param within expr_simple_expr to such a variable, that
242+
* Param's address is stored in expr_rw_param; then expression code
243+
* generation will allow the value for that Param to be passed read/write.
244+
*/
245+
inttarget_param;/* dno of assign target, or -1 if none */
246+
Param*expr_rw_param;/* read/write Param within expr, if any */
247+
238248
/*
239249
* If the expression was ever determined to be simple, we remember its
240250
* CachedPlanSource and CachedPlan here. If expr_simple_plan_lxid matches

0 commit comments

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