Re: WIP Patch: Precalculate stable functions, infrastructure v1

> Hi,

Hello!

> I've not followed this thread, but just scanned this quickly because it
> affects execExpr* stuff.

Thank you very much for your comments! Thanks to them I have made v4 of
the patches (as in the previous one, only planning and execution part is
changed).

> 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)

> 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?

Thanks, in new version I did all of it in this way.

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

I agree with you and in new version context is changed only for copying
datum of result value (if it's a pointer, its data should be allocated
in per_query_memory, or we will lost it for next tuples).

> Is this actually a meaningful path? Shouldn't always have done const
> evaluation before adding CachedExpr's?

eval_const_expressions_mutator is used several times, and one of them in
functions for selectivity evaluation (set_baserel_size_estimates ->
clauselist_selectivity -> clause_selectivity -> restriction_selectivity
-> ... -> get_restriction_variable -> estimate_expression_value ->
eval_const_expressions_mutator). In set_baserel_size_estimates function
right after selectivity evaluation there's costs evaluation and cached
expressions should be replaced before costs. I'm not sure that it is a
good idea to insert cached expressions replacement in
set_baserel_size_estimates, because in comments to it it's said "The
rel's targetlist and restrictinfo list must have been constructed
already, and rel->tuples must be set." and its file costsize.c is
entitled as "Routines to compute (and set) relation sizes and path
costs". So I have inserted cached expressions replacement just before it
(but I'm not sure that I have seen all places where it should be
inserted). What do you think about all of this?

--
Marina Polyakova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company