Re: Caching for stable expressions with constant arguments v6 - Mailing list pgsql-hackers

On Sat, Mar 10, 2012 at 02:05, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Marti Raudsepp <marti@juffo.org> writes:
>> [ cacheexpr-v8.patch ]
>
> A few comments

Whoa, that's quite a few. Thanks for the review.

> * There's a lot of stuff that seems wrong in detail in
> eval_const_expressions_mutator, eg it'll try to wrap a plain vanilla
> Const with a CacheExpr.  I see you've hacked that case inside
> insert_cache itself, but that seems like evidence of a poorly thought
> out recursion invariant.  The function should have a better notion than
> that of whether caching is useful for a given subtree.

Well my logic was this: accessing params and consts is just as fast as
accessing the cache -- there's no evaluation involved. So there's no
point in inserting CacheExpr in front of those. All other kinds of
expressions require some sort of evaluation, so they are cached, if
they weren't already constant-folded.

> In general I'd
> not expect it to insert a CacheExpr unless there is a Param or stable
> function call somewhere below the current point, but it seems not to be
> tracking that.

I think if you continue on your "Param or stable function" train of
thought, then it boils down to "expressions that can't be
constant-folded". And that's how it already works now --
constant-foldable expressions get reduced to a Const, which isn't
cached. There's no need to track it specially since we can check
whether we've got a Const node.

> You probably need at least a three-way indicator
> returned from each recursion level: subexpression is not cacheable
> (eg it contains a Var or volatile function); subexpression is cacheable
> but there is no reason to do so (default situation); or subexpression is
> cacheable and should be cached (it contains a Param or stable function).

That could be done -- give a special cachability result in code paths
that return a constant -- but that seems like complicating the logic
and doesn't tell us anything we don't already know.

----

> * I'm having a hard time understanding what you did to simplify_function
> and friends; that's been whacked around quite a bit, in ways that are
> neither clearly correct nor clearly related to the goal of the patch.

Sure, I'll split this out as a separate patch and send it up.

> * I believe the unconditional datumCopy call in ExecEvalCacheExpr will
> dump core if the value is null and the type is pass-by-reference.

datumCopy already has a check for NULL pointer. But good point about
skipping type properties lookup -- will change.

> * I think you should consider getting rid of the extra argument to
> ExecInitExpr, as well as the enable flag in CacheExprState, and instead
> driving cache-enable off a new flag added to EState

Will do.

> * In the same vein, I think it would be better to avoid adding
> the extra argument to eval_const_expressions_mutator and instead pass
> that state around in the existing "context" struct argument.

I'll have to see how ugly it gets to save & restore the cachability
field in recursive calls.

> I am entirely unimpressed with the fact that you can't pass
> the extra argument through expression_tree_mutator and thus have to dumb
> the code down to fail to cache underneath any node type for which
> there's not explicit coding in eval_const_expressions_mutator.

Well I deliberately chose a whitelisting approach. Expression types
that aren't important enough to be constant-folded probably aren't
that important for caching either.

Do you think it's safe to assume that expression types we don't know
about are inherently cachable?

> +  * NOTE: This function is not indempotent. Calling it twice over an expression
> * It seems like some of the ugliness here is due to thinking that
> selectivity functions can't cope with CacheExprs.  Wouldn't it be a lot
> better to make them cope?

I thought centralizing this CacheExpr-stripping to one place was a
better idea than spraying it all around the code base. We can skip the
copy *and* the check this way.

Hmm, if I passed the context to insert_cache then we could avoid this problem.

We could do the same for RelabelType in estimation mode, but I don't
know how safe that is.

> * I don't think it's a good idea for
> simplify_and_arguments/simplify_or_arguments to arbitrarily reorder the
> arguments based on cacheability.  I know that we tell people not to
> depend on order of evaluation in AND/OR lists, but that doesn't mean
> they listen

Fair enough, will change.

> * I don't believe CacheExprs can be treated as always simple by
> ruleutils' isSimpleNode

Ok.

> * I don't think I believe either of the changes you made to plpgsql's
> exec_simple_check_node.

Will change.

Regards,
Marti