Re: [HACKERS] Arrays of domains

Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:
> On 09/28/2017 05:44 PM, Tom Lane wrote:
>> Assuming that that's going to happen for v11, I'm inclined to leave the
>> optimization problem until the dust settles around CaseTestExpr.
>> Does anyone feel that this can't be committed before that's addressed?

> I'm Ok with it as long as we don't forget to revisit this.

I decided to go ahead and build a quick optimization for this case,
as per the attached patch that applies on top of what we previously
discussed.  It brings us back to almost par with HEAD:

    HEAD        Patch        + 04.patch

Q1    5453.235 ms    5440.876 ms    5407.965 ms
Q2    9340.670 ms    10191.194 ms    9407.093 ms
Q3    19078.457 ms    18967.279 ms    19050.392 ms
Q4    48196.338 ms    58547.531 ms    48696.809 ms

Unless Andres feels this is too ugly to live, I'm inclined to commit
the patch with this addition.  If we don't get around to revisiting
CaseTestExpr, I think this is OK, and if we do, this will make sure
that we consider this case in the revisit.

It's probably also worth pointing out that this test case is intentionally
chosen to be about the worst possible case for the patch.  A less-trivial
coercion function would make the overhead proportionally less meaningful.
There's also the point that the old code sometimes applies two layers of
array coercion rather than one.  As an example, coercing int4[] to
varchar(10)[] will do that.  If I replace "x::int8[]" with
"x::varchar(10)[]" in Q2 and Q4 in this test, I get

    HEAD        Patch (+04)

Q2    46929.728 ms    20646.003 ms
Q4    386200.286 ms    155917.572 ms

            regards, tom lane

diff --git a/src/backend/executor/execExprInterp.c b/src/backend/executor/execExprInterp.c
index e0a8998..c5e97ef 100644
*** a/src/backend/executor/execExprInterp.c
--- b/src/backend/executor/execExprInterp.c
***************
*** 34,43 ****
   *
   * For very simple instructions the overhead of the full interpreter
   * "startup", as minimal as it is, is noticeable.  Therefore
!  * ExecReadyInterpretedExpr will choose to implement simple scalar Var
!  * and Const expressions using special fast-path routines (ExecJust*).
!  * Benchmarking shows anything more complex than those may as well use the
!  * "full interpreter".
   *
   * Complex or uncommon instructions are not implemented in-line in
   * ExecInterpExpr(), rather we call out to a helper function appearing later
--- 34,41 ----
   *
   * For very simple instructions the overhead of the full interpreter
   * "startup", as minimal as it is, is noticeable.  Therefore
!  * ExecReadyInterpretedExpr will choose to implement certain simple
!  * opcode patterns using special fast-path routines (ExecJust*).
   *
   * Complex or uncommon instructions are not implemented in-line in
   * ExecInterpExpr(), rather we call out to a helper function appearing later
*************** static Datum ExecJustConst(ExprState *st
*** 149,154 ****
--- 147,153 ----
  static Datum ExecJustAssignInnerVar(ExprState *state, ExprContext *econtext, bool *isnull);
  static Datum ExecJustAssignOuterVar(ExprState *state, ExprContext *econtext, bool *isnull);
  static Datum ExecJustAssignScanVar(ExprState *state, ExprContext *econtext, bool *isnull);
+ static Datum ExecJustApplyFuncToCase(ExprState *state, ExprContext *econtext, bool *isnull);


  /*
*************** ExecReadyInterpretedExpr(ExprState *stat
*** 184,193 ****

      /*
       * Select fast-path evalfuncs for very simple expressions.  "Starting up"
!      * the full interpreter is a measurable overhead for these.  Plain Vars
!      * and Const seem to be the only ones where the intrinsic cost is small
!      * enough that the overhead of ExecInterpExpr matters.  For more complex
!      * expressions it's cheaper to use ExecInterpExpr always.
       */
      if (state->steps_len == 3)
      {
--- 183,190 ----

      /*
       * Select fast-path evalfuncs for very simple expressions.  "Starting up"
!      * the full interpreter is a measurable overhead for these, and these
!      * patterns occur often enough to be worth optimizing.
       */
      if (state->steps_len == 3)
      {
*************** ExecReadyInterpretedExpr(ExprState *stat
*** 230,235 ****
--- 227,239 ----
              state->evalfunc = ExecJustAssignScanVar;
              return;
          }
+         else if (step0 == EEOP_CASE_TESTVAL &&
+                  step1 == EEOP_FUNCEXPR_STRICT &&
+                  state->steps[0].d.casetest.value)
+         {
+             state->evalfunc = ExecJustApplyFuncToCase;
+             return;
+         }
      }
      else if (state->steps_len == 2 &&
               state->steps[0].opcode == EEOP_CONST)
*************** ExecJustAssignScanVar(ExprState *state,
*** 1811,1816 ****
--- 1815,1857 ----
      return 0;
  }

+ /* Evaluate CASE_TESTVAL and apply a strict function to it */
+ static Datum
+ ExecJustApplyFuncToCase(ExprState *state, ExprContext *econtext, bool *isnull)
+ {
+     ExprEvalStep *op = &state->steps[0];
+     FunctionCallInfo fcinfo;
+     bool       *argnull;
+     int            argno;
+     Datum        d;
+
+     /*
+      * XXX with some redesign of the CaseTestExpr mechanism, maybe we could
+      * get rid of this data shuffling?
+      */
+     *op->resvalue = *op->d.casetest.value;
+     *op->resnull = *op->d.casetest.isnull;
+
+     op++;
+
+     fcinfo = op->d.func.fcinfo_data;
+     argnull = fcinfo->argnull;
+
+     /* strict function, so check for NULL args */
+     for (argno = 0; argno < op->d.func.nargs; argno++)
+     {
+         if (argnull[argno])
+         {
+             *isnull = true;
+             return (Datum) 0;
+         }
+     }
+     fcinfo->isnull = false;
+     d = op->d.func.fn_addr(fcinfo);
+     *isnull = fcinfo->isnull;
+     return d;
+ }
+

  /*
   * Do one-time initialization of interpretation machinery.

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers