Re: WIP Patch: Precalculate stable functions, infrastructure v1

From: Marina Polyakova <m(dot)polyakova(at)postgrespro(dot)ru>
To: Andres Freund <andres(at)anarazel(dot)de>
Cc: Aleksander Alekseev <a(dot)alekseev(at)postgrespro(dot)ru>, Alexander Korotkov <a(dot)korotkov(at)postgrespro(dot)ru>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Subject: Re: WIP Patch: Precalculate stable functions, infrastructure v1
Date: 2017-05-22 15:32:17
Message-ID: ebde9ef220f088b91b24df55f94f0394@postgrespro.ru
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: pgsql-hackers

> 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

Attachment Content-Type Size
v4-0001-Precalculate-stable-functions-infrastructure.patch text/x-diff 25.2 KB
v4-0002-Precalculate-stable-functions-planning-and-execut.patch text/x-diff 93.9 KB
v4-0003-Precalculate-stable-functions-costs.patch text/x-diff 4.6 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Andres Freund 2017-05-22 15:42:27 Re: [HACKERS] Concurrent ALTER SEQUENCE RESTART Regression
Previous Message Andres Freund 2017-05-22 15:30:48 plpgsql, caching, resowners and jit