From d3eaab3ef0d552a2f6555b0424a32dc9e77fb17c Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Mon, 17 Aug 2015 19:39:35 -0400 Subject: [PATCH] Fix performance bug from conflict between two previous improvements. My expanded-objects patch (commit 1dc5ebc9077ab742) included code to make plpgsql pass expanded-object variables as R/W pointers to certain functions that are trusted for modifying such variables in-place. However, that optimization got broken by commit 6c82d8d1fdb1f126, which arranged to share a single ParamListInfo across most expressions evaluated by a plpgsql function. We don't want a R/W pointer to be passed to other functions just because we decided one function was safe! Fortunately, the breakage was in the other direction, of never passing a R/W pointer at all, because we'd always have pre-initialized the shared array slot with a R/O pointer. So it was still functionally correct, but we were back to O(N^2) performance for repeated use of "a := a || x". To fix, force an unshared param array to be used when the R/W param optimization is active. Commit 6c82d8d1fdb1f126 is in HEAD only, so no need for a back-patch. --- src/pl/plpgsql/src/pl_exec.c | 38 ++++++++++++++++++++++++++++-------- 1 file changed, 30 insertions(+), 8 deletions(-) diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c index 8bc5e6f29fd..fb9333606d4 100644 --- a/src/pl/plpgsql/src/pl_exec.c +++ b/src/pl/plpgsql/src/pl_exec.c @@ -5454,12 +5454,18 @@ exec_eval_simple_expr(PLpgSQL_execstate *estate, } /* - * Set up ParamListInfo to pass to executor. For safety, save and restore - * estate->paramLI->parserSetupArg around our use of the param list. + * Set up ParamListInfo to pass to executor. We need an unshared list if + * it's going to include any R/W expanded-object pointer. For safety, + * save and restore estate->paramLI->parserSetupArg around our use of the + * param list. */ save_setup_arg = estate->paramLI->parserSetupArg; - paramLI = setup_param_list(estate, expr); + if (expr->rwparam >= 0) + paramLI = setup_unshared_param_list(estate, expr); + else + paramLI = setup_param_list(estate, expr); + econtext->ecxt_param_list_info = paramLI; /* @@ -5478,6 +5484,9 @@ exec_eval_simple_expr(PLpgSQL_execstate *estate, /* Assorted cleanup */ expr->expr_simple_in_use = false; + if (paramLI && paramLI != estate->paramLI) + pfree(paramLI); + estate->paramLI->parserSetupArg = save_setup_arg; if (!estate->readonly_func) @@ -5504,11 +5513,15 @@ exec_eval_simple_expr(PLpgSQL_execstate *estate, * * We share a single ParamListInfo array across all SPI calls made from this * estate, except calls creating cursors, which use setup_unshared_param_list - * (see its comments for reasons why). A shared array is generally OK since - * any given slot in the array would need to contain the same current datum - * value no matter which query or expression we're evaluating. However, - * paramLI->parserSetupArg points to the specific PLpgSQL_expr being - * evaluated. This is not an issue for statement-level callers, but + * (see its comments for reasons why), and calls that pass a R/W expanded + * object pointer. A shared array is generally OK since any given slot in + * the array would need to contain the same current datum value no matter + * which query or expression we're evaluating; but of course that doesn't + * hold when a specific variable is being passed as a R/W pointer, because + * other expressions in the same function probably don't want to do that. + * + * Note that paramLI->parserSetupArg points to the specific PLpgSQL_expr + * being evaluated. This is not an issue for statement-level callers, but * lower-level callers must save and restore estate->paramLI->parserSetupArg * just in case there's an active evaluation at an outer call level. * @@ -5539,6 +5552,11 @@ setup_param_list(PLpgSQL_execstate *estate, PLpgSQL_expr *expr) */ Assert(expr->plan != NULL); + /* + * Expressions with R/W parameters can't use the shared param list. + */ + Assert(expr->rwparam == -1); + /* * We only need a ParamListInfo if the expression has parameters. In * principle we should test with bms_is_empty(), but we use a not-null @@ -5605,6 +5623,10 @@ setup_param_list(PLpgSQL_execstate *estate, PLpgSQL_expr *expr) * want it to copy anything that's not used by the specific cursor; that * could result in uselessly copying some large values. * + * We also use this for expressions that are passing a R/W object pointer + * to some trusted function. We don't want the R/W pointer to get into the + * shared param list, where it could get passed to some less-trusted function. + * * Caller should pfree the result after use, if it's not NULL. */ static ParamListInfo