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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
