Re: [PATCH] Caching for stable expressions with constant arguments v3

On Sun, Dec 4, 2011 at 22:53, Heikki Linnakangas
<heikki(dot)linnakangas(at)enterprisedb(dot)com> wrote:
> This seems to have bitrotted, thanks to the recent refactoring in
> eval_const_expressions().

Luckily the conflicts are mostly whitespace changes, so shouldn't be
hard to fix. I'll try to come up with an updated patch today or
tomorrow.

> I wonder if it would be better to add the CacheExpr nodes to the tree as a
> separate pass, instead of shoehorning it into eval_const_expressions? I
> think would be more readable that way, even though a separate pass would be
> more expensive. And there are callers of eval_const_expressions() that have
> no use for the caching, like process_implied_equality().

Per Tom's comment, I won't split out the cache insertion for now.

The context struct has a boolean 'cache' attribute that controls
whether caching is desired or not. I think this could be exposed to
the caller as an eval_const_expressions() argument.

> This comment in RelationGetExpressions() also worries me:
[...]
> Do the injected CacheExprs screw up that equality? Or the constraint
> exclusion logic in predtest.c?

I suspect these cases are guaranteed not to produce any CacheExprs.
They're always immutable expressions. If they contain Var references
they're stored as is (not cachable); if not, they're folded to a
constant.

But I will have to double-check all the callers; it might be a good
idea to disable caching anyway in these cases.

Regards,
Marti