Re: Why overhead of SPI is so large? - Mailing list pgsql-hackers

On 07.11.2019 15:09, Pavel Stehule wrote:

čt 7. 11. 2019 v 13:03 odesílatel Kyotaro Horiguchi <horikyota.ntt@gmail.com> napsal:

Hello.

At Tue, 5 Nov 2019 22:14:40 +0100, Pavel Stehule <pavel.stehule@gmail.com> wrote in
> Hi
>
> pá 23. 8. 2019 v 16:32 odesílatel Konstantin Knizhnik <
> k.knizhnik@postgrespro.ru> napsal:
>
> >
> >
> > On 23.08.2019 14:42, Pavel Stehule wrote:
> >
> >
> > In reality it is not IMMUTABLE function. On second hand, there are lot of
> > application that depends on this behave.
> >
> > It is well know trick how to reduce estimation errors related to JOINs.
> > When immutable function has constant parameters, then it is evaluated in
> > planning time.
> >
> > So sometimes was necessary to use
> >
> > SELECT ... FROM tab WHERE foreign_key = immutable_function('constant
> > parameter')
> >
> > instead JOIN.
> >
> > It is ugly, but it is working perfectly. I think so until we will have
> > multi table statistics, this behave should be available in Postgres.
> >
> > Sure, this function should not be used for functional indexes.
> >
> >
> >
> > What about the following version of the patch?
> >
>
> I am sending review of this small patch.
>
> This small patch reduce a overhead of usage buildin immutable functions in
> volatile functions with simple trick. Starts snapshot only when it is
> necessary.
>
> In decrease runtime time about 25 % on this small example.
>
> do $$
> declare i int;
> begin
>   i := 0;
>   while i < 10000000
>   loop
>     i := i + 1;
>   end loop;
> end;
> $$;
>
> If there are more expressions, then speedup can be more interesting. If
> there are other bottlenecks, then the speedup will be less. 25% is not bad,
> so we want to this feature.
>
> I believe so similar method can be used more aggressively with more
> significant performance benefit, but this is low hanging fruit and isn't
> reason to wait for future.
>
> This patch doesn't introduce any new feature, so new tests and new doc is
> not necessary.
> The patch is readable, well  formatted, only comments are too long. I fixed
> it.
> All tests passed
> I fixed few warnings, and I reformated little bit function
> expr_needs_snapshot to use if instead case, what is more usual in these
> cases.
>
> I think so this code can be marked as ready for commit

I have some comments on this.

expr_needs_snapshot checks out some of the node already checked out in
exec_simple_check_plan but not all. However I don't see the criteria
of the choice.

I might be too worrying, but maybe we should write the function in
white-listed way, that is, expr_needs_snapshot returns false only if
the whole tree consists of only the node types known to the
function. And any unknown node makes the function return true
immediately.

has sense

There are  62 cases handled by expression_tree_walker.
I have inspected all of them and considered only these 6 to be "dangerous": intentionally require snapshot.
FuncExpr, OpExpr, Query, SubPlan, AlternativeSubPlan, SubLink.

So if I use "white-list" approach, I have to enumerate all other 62-6=56 cases in expr_needs_snapshot function.
Frankly speaking I do not this that it is good idea. New nodes are rarely added to the Postgres and expression_tree_walker
in any case has to be changed to handle this new nodes. There are many other places where tree walker is used to check presence (or absence)
of some properties in the tree. And none is this places assume that some newly introduced node may require special handling of this property check.

In any case, if you think that explicit enumerations of this 56 cases (or some subset of them) is good idea, then I will do it.
If you have concerns about some particular nodes, I can add them to  "black list".

-- 
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company