Re: [HACKERS] WIP Patch: Precalculate stable functions,infrastructure v1 - Mailing list pgsql-hackers

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