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
