pá 8. 11. 2019 v 14:31 odesílatel Konstantin Knizhnik <
k.knizh...@postgrespro.ru> napsal:
>
>
> On 07.11.2019 15:09, Pavel Stehule wrote:
>
>
>
> č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
>
>
> 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".
>
Is question how to implement this feature safely. Isn't possible to use
same check for functional indexes?
>
> --
> Konstantin Knizhnik
> Postgres Professional: http://www.postgrespro.com
> The Russian Postgres Company
>
>
