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

Marti Raudsepp <marti@juffo.org> writes:
> On Sat, Mar 10, 2012 at 02:05, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> * 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.

I think this is overkill.  Inserting a CacheExpr is far from free:
it costs cycles to make that node, cycles to copy it around, cycles
to recurse through it during subsequent processing.  We should not
be putting them in to save trivial amounts of execution time.  So
I don't believe your argument that there is no such thing as a
non-Const, non-volatile expression that shouldn't be cached.
Examples of things that clearly are not expensive enough to deserve
a cache node are RelabelType and PlaceHolderVar (the latter won't
even be there at runtime).

More generally, though, I think that caching something like, say,
"Param int4pl 1" is probably a net loss once you consider all the
added overhead.  I'm not going to argue for putting explicit cost
considerations into the first version, but I don't want the
infrastructure to be such that it's impossible to bolt that on later.

>> * 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.

You're assuming that a null Datum necessarily has an all-zero value,
which is not a safe assumption; in many situations the datum word
will be uninitialized.  The null-pointer check in datumCopy is not
particularly useful IMO.  It's probably a hangover from fifteen years
ago when the system's null-handling was a lot shakier than it is now.

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

Presumably, anything that doesn't contain a Var nor set off
contain_volatile_functions() should be safe to cache.  I do not
care for the assumption that this set of expression types is identical
to the set that eval_const_expressions bothers to deal with.

>> * 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.

How exactly do you figure that this avoids needing to know about them
elsewhere?  Not everything in the selectivity code starts out by doing
estimate_expression_value.  Perhaps more to the point, the stuff that
*does* do that is generally going to punt if it doesn't get a plain
Const back, so eliminating CacheExprs from non-Const cases isn't helping
it anyway.
        regards, tom lane