čt 7. 11. 2019 v 13:03 odesílatel Kyotaro Horiguchi <horikyota....@gmail.com>
napsal:
> 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.
>
has sense
Pavel
> regards.
>
> --
> Kyotaro Horiguchi
> NTT Open Source Software Center
>
