Re: WIP Patch: Precalculate stable functions, infrastructure v1

Hi,

On 2017-05-18 19:00:09 +0300, Marina Polyakova wrote:
> > Here's v2 of the patches. Changes from v1:
>
> And here there's v3 of planning and execution: common executor steps for all
> types of cached expression.

I've not followed this thread, but just scanned this quickly because it
affects execExpr* stuff.

> + case T_CachedExpr:
> + {
> + int adjust_jump;
> +
> + /*
> + * Allocate and fill scratch memory used by all steps of
> + * CachedExpr evaluation.
> + */
> + scratch.d.cachedexpr.isExecuted = (bool *) palloc(sizeof(bool));
> + scratch.d.cachedexpr.resnull = (bool *) palloc(sizeof(bool));
> + scratch.d.cachedexpr.resvalue = (Datum *) palloc(sizeof(Datum));
> +
> + *scratch.d.cachedexpr.isExecuted = false;
> + *scratch.d.cachedexpr.resnull = false;
> + *scratch.d.cachedexpr.resvalue = (Datum) 0;

Looks like having something like struct CachedExprState would be better,
than these separate allocations? That also allows to aleviate some size
concerns when adding new fields (see below)

> @@ -279,6 +279,7 @@ ExecInterpExpr(ExprState *state, ExprContext *econtext, bool *isnull)
> TupleTableSlot *innerslot;
> TupleTableSlot *outerslot;
> TupleTableSlot *scanslot;
> + MemoryContext oldContext; /* for EEOP_CACHEDEXPR_* */

I'd rather not have this on function scope - a) the stack pressure in
ExecInterpExpr is quite noticeable in profiles already b) this is going
to trigger warnings because of unused vars, because the compiler doesn't
understand that EEOP_CACHEDEXPR_IF_CACHED always follows
EEOP_CACHEDEXPR_SUBEXPR_END.

How about instead storing oldcontext in the expression itself?

I'm also not sure how acceptable it is to just assume it's ok to leave
stuff in per_query_memory, in some cases that could prove to be
problematic.

> + case T_CachedExpr:
> + {
> + CachedExpr *cachedexpr = (CachedExpr *) node;
> + Node *new_subexpr = eval_const_expressions_mutator(
> + get_subexpr(cachedexpr), context);
> + CachedExpr *new_cachedexpr;
> +
> + /*
> + * If unsafe transformations are used cached expression should
> + * be always simplified.
> + */
> + if (context->estimate)
> + Assert(IsA(new_subexpr, Const));
> +
> + if (IsA(new_subexpr, Const))
> + {
> + /* successfully simplified it */
> + return new_subexpr;
> + }
> + else
> + {
> + /*
> + * The expression cannot be simplified any further, so build
> + * and return a replacement CachedExpr node using the
> + * possibly-simplified arguments of subexpression.
> + */

Is this actually a meaningful path? Shouldn't always have done const
evaluation before adding CachedExpr's?

Greetings,

Andres Freund