Re: function calls optimization - Mailing list pgsql-hackers

Hi,

On 2019-11-03 21:56:31 +0100, Andrzej Barszcz wrote:
> Main goal of this patch is to avoid repeated calls of immutable/stable
> functions.
> This patch is against version 10.10.
> I guess same logic could be implemented up till version 12.

If you want actual development feedback, you're more likely to get that
when posting patches against the master branch.


> --- src/include/nodes/execnodes.h    2019-08-05 23:16:54.000000000 +0200
> +++ src/include/nodes/execnodes.h    2019-11-03 20:05:34.338305825 +0100
> @@ -882,6 +883,39 @@ typedef struct PlanState
>      TupleTableSlot *ps_ResultTupleSlot; /* slot for my result tuples */
>      ExprContext *ps_ExprContext;    /* node's expression-evaluation context */
>      ProjectionInfo *ps_ProjInfo;    /* info for doing tuple projection */
> +#ifdef OPTFUNCALLS
> +    /* was_called - list of  ExprEvalStep* or FuncExpr* depending on execution stage
> +     * 
> +     * Stage I. ExecInitExprRec()
> +     *    List gathers all not volatile, not set returning, not window FuncExpr*,
> +     *    equal nodes occupy one position in the list. Position in this list ( counting from 1 )
> +     *    and planstate are remembered in actual ExprEvalStep*
> +     *
> +     *     For query: select f(n),f(n) from t  - was_called->length will be 1 and ptr_value 
> +     *         will be FuncExpr* node of f(n)
> +     *
> +     *     For query: select f(n),g(n),f(n) from t - list->length == 2
> +     *
> +     * Stage II. ExecProcnode()
> +     *    For every planstate->was_called list changes its interpretation - from now on
> +     *    it is a list of ExprEvalStep* . Before executing real execProcnode
> +     *    every element of this list ( ptr_value ) is set to NULL. We don't know which
> +     *    function will be called first
> +     *
> +     * Stage III. ExecInterpExpr() case EEOP_FUNCEXPR
> +     *    ExprEvalStep.position > 0  means that in planstate->was_called could be ExprEvalStep*
> +     *    which was done yet or NULL.
> +     *
> +     *    NULL means that eval step is entered first time and:
> +     *        1. real function must be called
> +     *        2. ExprEvalStep has to be remembered in planstate->was_called at position
> +     *        step->position - 1
> +     *
> +     *    NOT NULL means that in planstate->was_called is ExprEvalStep* with ready result, so
> +     *    there is no need to call function
> +     */
> +    List *was_called;
> +#endif
>  } PlanState;

I don't think the above describes a way to do this that is
acceptable. For one, I think this needs to happen at plan time, not for
every single execution of the statement. Nor do I think is it ok to make
ExecProcNode() etc slower for this feature - that's a very crucial
routine (similar with EEOP_FUNCEXPR checking the cache, but there we
could just have a different step type doing so).

Have you looked any of the previous work on such caching? I strongly
suggest doing so if you're interested in getting such a feature into
postgres. E.g. there's a fair bit of relevant discussion in
https://www.postgresql.org/message-id/da87bb6a014e029176a04f6e50033cfb%40postgrespro.ru
e.g. between Tom and me further down.


>  /* ----------------
> --- src/include/executor/execExpr.h    2019-08-05 23:16:54.000000000 +0200
> +++ src/include/executor/execExpr.h    2019-11-03 20:04:03.739025142 +0100
> @@ -561,6 +561,10 @@ typedef struct ExprEvalStep
>              AlternativeSubPlanState *asstate;
>          }            alternative_subplan;
>      }            d;
> +#ifdef OPTFUNCALLS
> +    PlanState *planstate;    /* parent PlanState for this expression */
> +    int position;        /* position in planstate->was_called counted from 1 */
> +#endif
>  } ExprEvalStep;

This is not ok. We cannot just make every single ExprEvalStep larger for
this feature. Nor is clear why this is even needed - every ExprEvalStep
is associated with an ExprState, and ExprState already has a reference
to the parent planstate.


Greetings,

Andres Freund