Hello.
At Tue, 5 Nov 2019 22:14:40 +0100, Pavel Stehule <pavel.steh...@gmail.com>
wrote in
> Hi
>
> pá 23. 8. 2019 v 16:32 odesílatel Konstantin Knizhnik <
> k.knizh...@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.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
