Re: Rethinking the parameter access hooks for plpgsql's benefit

Robert Haas <robertmhaas@gmail.com> writes:
> On the technical side, I do agree that the requirement to allocate and
> zero an array for every new simple expression is unfortunate, but I'm
> not convinced that repeatedly invoking the hook-function is a good way
> to solve that problem.  Calling the hook-function figures to be
> significantly more expensive than an array-fetch, so you can almost
> think of the ParamListInfo entries themselves as a cache of data
> retrieved from the hook function.  In that view of the world, the
> problem here is that initializing the cache is too expensive.  Your
> solution to that is to rip the cache out completely, but what would be
> better is keep the cache and make initializing it cheaper - e.g. by
> sharing one ParamListInfo across all expressions in the same scope; or
> by not zeroing the memory and instead tracking the the first N slots
> are the only ones we've initialized; or $your_idea_here.

Getting back to the actual merits of this patch --- you're right, it
was not a good idea as proposed.  I'd been thinking about the scalar
expression case only, and forgot that the same code is used to pass
parameters to queries, which might evaluate those parameters quite
a large number of times.  So any savings from reducing the setup time
is sure to be swamped if the hook-function gets called repeatedly.
Another problem is that for ROW and REC datums, exec_eval_datum()
pallocs memory that won't get freed till the end of the statement,
so repeat evaluation would cause memory leaks.  So we really need
evaluate-at-most-once semantics in there.

However, this attempt did confirm that we don't need to create a separate
ParamListInfo struct for each evaluation attempt.  So the attached,
greatly stripped-down patch just allocates a ParamListInfo once at
function startup, and then zeroes it each time.  This does nothing for
the memset overhead but it does get rid of the palloc traffic, which
seems to be good for a few percent on simple-assignment-type statements.
AFAICS this is an unconditional win compared to HEAD.  What's more, it
provides at least a basis for doing better later: if we could think of
a reasonably cheap way of tracking which datums get invalidated by an
assignment, we might be able to reset only selected entries in the array
rather than blindly blowing away all of them.

I propose to apply this and leave it at that for now.

            regards, tom lane

diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index f24f55a..0ad32f7 100644
*** a/src/pl/plpgsql/src/pl_exec.c
--- b/src/pl/plpgsql/src/pl_exec.c
*************** exec_stmt_forc(PLpgSQL_execstate *estate
*** 2197,2206 ****
          elog(ERROR, "could not open cursor: %s",
               SPI_result_code_string(SPI_result));

-     /* don't need paramlist any more */
-     if (paramLI)
-         pfree(paramLI);
-
      /*
       * If cursor variable was NULL, store the generated portal name in it
       */
--- 2197,2202 ----
*************** plpgsql_estate_setup(PLpgSQL_execstate *
*** 3169,3174 ****
--- 3165,3180 ----
      estate->datums = palloc(sizeof(PLpgSQL_datum *) * estate->ndatums);
      /* caller is expected to fill the datums array */

+     /* initialize ParamListInfo with one entry per datum, all invalid */
+     estate->paramLI = (ParamListInfo)
+         palloc0(offsetof(ParamListInfoData, params) +
+                 estate->ndatums * sizeof(ParamExternData));
+     estate->paramLI->paramFetch = plpgsql_param_fetch;
+     estate->paramLI->paramFetchArg = (void *) estate;
+     estate->paramLI->parserSetup = (ParserSetupHook) plpgsql_parser_setup;
+     estate->paramLI->parserSetupArg = NULL;        /* filled during use */
+     estate->paramLI->numParams = estate->ndatums;
+
      /* set up for use of appropriate simple-expression EState */
      if (simple_eval_estate)
          estate->simple_eval_estate = simple_eval_estate;
*************** plpgsql_estate_setup(PLpgSQL_execstate *
*** 3179,3185 ****
      estate->eval_processed = 0;
      estate->eval_lastoid = InvalidOid;
      estate->eval_econtext = NULL;
-     estate->cur_expr = NULL;

      estate->err_stmt = NULL;
      estate->err_text = NULL;
--- 3185,3190 ----
*************** exec_stmt_execsql(PLpgSQL_execstate *est
*** 3495,3503 ****
                       (rc == SPI_OK_SELECT) ? errhint("If you want to discard the results of a SELECT, use PERFORM
instead."): 0)); 
      }

-     if (paramLI)
-         pfree(paramLI);
-
      return PLPGSQL_RC_OK;
  }

--- 3500,3505 ----
*************** exec_stmt_open(PLpgSQL_execstate *estate
*** 3864,3871 ****

      if (curname)
          pfree(curname);
-     if (paramLI)
-         pfree(paramLI);

      return PLPGSQL_RC_OK;
  }
--- 3866,3871 ----
*************** exec_assign_c_string(PLpgSQL_execstate *
*** 4050,4056 ****


  /* ----------
!  * exec_assign_value            Put a value into a target field
   *
   * Note: in some code paths, this will leak memory in the eval_econtext;
   * we assume that will be cleaned up later by exec_eval_cleanup.  We cannot
--- 4050,4056 ----


  /* ----------
!  * exec_assign_value            Put a value into a target datum
   *
   * Note: in some code paths, this will leak memory in the eval_econtext;
   * we assume that will be cleaned up later by exec_eval_cleanup.  We cannot
*************** exec_run_select(PLpgSQL_execstate *estat
*** 4909,4916 ****
          if (*portalP == NULL)
              elog(ERROR, "could not open implicit cursor for query \"%s\": %s",
                   expr->query, SPI_result_code_string(SPI_result));
-         if (paramLI)
-             pfree(paramLI);
          return SPI_OK_CURSOR;
      }

--- 4909,4914 ----
*************** exec_run_select(PLpgSQL_execstate *estat
*** 4930,4938 ****
      estate->eval_processed = SPI_processed;
      estate->eval_lastoid = SPI_lastoid;

-     if (paramLI)
-         pfree(paramLI);
-
      return rc;
  }

--- 4928,4933 ----
*************** exec_eval_simple_expr(PLpgSQL_execstate
*** 5140,5146 ****
      LocalTransactionId curlxid = MyProc->lxid;
      CachedPlan *cplan;
      ParamListInfo paramLI;
!     PLpgSQL_expr *save_cur_expr;
      MemoryContext oldcontext;

      /*
--- 5135,5141 ----
      LocalTransactionId curlxid = MyProc->lxid;
      CachedPlan *cplan;
      ParamListInfo paramLI;
!     void       *save_setup_arg;
      MemoryContext oldcontext;

      /*
*************** exec_eval_simple_expr(PLpgSQL_execstate
*** 5216,5229 ****
      }

      /*
!      * Create the param list in econtext's temporary memory context. We won't
!      * need to free it explicitly, since it will go away at the next reset of
!      * that context.
!      *
!      * Just for paranoia's sake, save and restore the prior value of
!      * estate->cur_expr, which setup_param_list() sets.
       */
!     save_cur_expr = estate->cur_expr;

      paramLI = setup_param_list(estate, expr);
      econtext->ecxt_param_list_info = paramLI;
--- 5211,5220 ----
      }

      /*
!      * Set up param list.  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);
      econtext->ecxt_param_list_info = paramLI;
*************** exec_eval_simple_expr(PLpgSQL_execstate
*** 5244,5250 ****
      /* Assorted cleanup */
      expr->expr_simple_in_use = false;

!     estate->cur_expr = save_cur_expr;

      if (!estate->readonly_func)
          PopActiveSnapshot();
--- 5235,5241 ----
      /* Assorted cleanup */
      expr->expr_simple_in_use = false;

!     estate->paramLI->parserSetupArg = save_setup_arg;

      if (!estate->readonly_func)
          PopActiveSnapshot();
*************** exec_eval_simple_expr(PLpgSQL_execstate
*** 5268,5273 ****
--- 5259,5273 ----
  /*
   * Create a ParamListInfo to pass to SPI
   *
+  * We share a single ParamListInfo array across all SPI calls made from this
+  * estate.  This 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 lower-level callers should save and restore
+  * estate->paramLI->parserSetupArg just in case there's an active evaluation
+  * at an outer call level.
+  *
   * We fill in the values for any expression parameters that are plain
   * PLpgSQL_var datums; these are cheap and safe to evaluate, and by setting
   * them with PARAM_FLAG_CONST flags, we allow the planner to use those values
*************** exec_eval_simple_expr(PLpgSQL_execstate
*** 5277,5285 ****
   * the expression that might never be evaluated at runtime.  To handle those
   * parameters, we set up a paramFetch hook for the executor to call when it
   * wants a not-presupplied value.
-  *
-  * The result is a locally palloc'd array that should be pfree'd after use;
-  * but note it can be NULL.
   */
  static ParamListInfo
  setup_param_list(PLpgSQL_execstate *estate, PLpgSQL_expr *expr)
--- 5277,5282 ----
*************** setup_param_list(PLpgSQL_execstate *esta
*** 5293,5314 ****
      Assert(expr->plan != NULL);

      /*
!      * Could we re-use these arrays instead of palloc'ing a new one each time?
!      * However, we'd have to re-fill the array each time anyway, since new
!      * values might have been assigned to the variables.
       */
!     if (!bms_is_empty(expr->paramnos))
      {
          int            dno;

!         paramLI = (ParamListInfo)
!             palloc0(offsetof(ParamListInfoData, params) +
!                     estate->ndatums * sizeof(ParamExternData));
!         paramLI->paramFetch = plpgsql_param_fetch;
!         paramLI->paramFetchArg = (void *) estate;
!         paramLI->parserSetup = (ParserSetupHook) plpgsql_parser_setup;
!         paramLI->parserSetupArg = (void *) expr;
!         paramLI->numParams = estate->ndatums;

          /* Instantiate values for "safe" parameters of the expression */
          dno = -1;
--- 5290,5317 ----
      Assert(expr->plan != NULL);

      /*
!      * 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
!      * test because it's faster.  In current usage bits are never removed from
!      * expr->paramnos, only added, so this test is correct anyway.
       */
!     if (expr->paramnos)
      {
          int            dno;

!         /* Use the common ParamListInfo for all evals in this estate */
!         paramLI = estate->paramLI;
!
!         /*
!          * Reset all entries to "invalid".  It's pretty annoying to have to do
!          * this, but we don't currently track enough information to know which
!          * old entries might be obsolete.  (There are a couple of nontrivial
!          * issues that would have to be dealt with in order to do better here.
!          * First, ROW and RECFIELD datums depend on other datums, and second,
!          * exec_eval_datum() will return short-lived palloc'd values for ROW
!          * and REC datums.)
!          */
!         MemSet(paramLI->params, 0, estate->ndatums * sizeof(ParamExternData));

          /* Instantiate values for "safe" parameters of the expression */
          dno = -1;
*************** setup_param_list(PLpgSQL_execstate *esta
*** 5330,5339 ****

          /*
           * Set up link to active expr where the hook functions can find it.
!          * Callers must save and restore cur_expr if there is any chance that
!          * they are interrupting an active use of parameters.
           */
!         estate->cur_expr = expr;

          /*
           * Also make sure this is set before parser hooks need it.  There is
--- 5333,5342 ----

          /*
           * Set up link to active expr where the hook functions can find it.
!          * Callers must save and restore parserSetupArg if there is any chance
!          * that they are interrupting an active use of parameters.
           */
!         paramLI->parserSetupArg = (void *) expr;

          /*
           * Also make sure this is set before parser hooks need it.  There is
*************** plpgsql_param_fetch(ParamListInfo params
*** 5373,5379 ****

      /* fetch back the hook data */
      estate = (PLpgSQL_execstate *) params->paramFetchArg;
!     expr = estate->cur_expr;
      Assert(params->numParams == estate->ndatums);

      /*
--- 5376,5382 ----

      /* fetch back the hook data */
      estate = (PLpgSQL_execstate *) params->paramFetchArg;
!     expr = (PLpgSQL_expr *) params->parserSetupArg;
      Assert(params->numParams == estate->ndatums);

      /*
diff --git a/src/pl/plpgsql/src/plpgsql.h b/src/pl/plpgsql/src/plpgsql.h
index 4ec4628..66d4da6 100644
*** a/src/pl/plpgsql/src/plpgsql.h
--- b/src/pl/plpgsql/src/plpgsql.h
*************** typedef struct PLpgSQL_execstate
*** 784,789 ****
--- 784,792 ----
      int            ndatums;
      PLpgSQL_datum **datums;

+     /* we pass datums[i] to the executor, when needed, in paramLI->params[i] */
+     ParamListInfo paramLI;
+
      /* EState to use for "simple" expression evaluation */
      EState       *simple_eval_estate;

*************** typedef struct PLpgSQL_execstate
*** 792,798 ****
      uint32        eval_processed;
      Oid            eval_lastoid;
      ExprContext *eval_econtext; /* for executing simple expressions */
-     PLpgSQL_expr *cur_expr;        /* current query/expr being evaluated */

      /* status information for error context reporting */
      PLpgSQL_stmt *err_stmt;        /* current stmt */
--- 795,800 ----