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