Horton

Thread

Commits

  1. Fix memory leak in plpgsql's CALL processing.

  1. calling procedures is slow and consumes extra much memory against calling function

    Pavel Stehule <pavel.stehule@gmail.com> — 2020-05-10T20:20:53Z

    Hi

I try to use procedures in Orafce package, and I did some easy performance
tests. I found some hard problems:

1. test case

create or replace procedure p1(inout r int, inout v int) as $$
begin v := random() * r; end
$$ language plpgsql;

This command requires

do $$
declare r int default 100; x int;
begin
  for i in 1..300000 loop
     call p1(r, x);
  end loop;
end;
$$;

about 2.2GB RAM and 10 sec.

When I rewrite same to functions then

create or replace function p1func2(inout r int, inout v int) as $$
begin v := random() * r; end
$$ language plpgsql;

do $$
declare r int default 100; x int; re record;
begin
  for i in 1..300000 loop
     re := p1func2(r, x);
  end loop;
end;
$$;

Then execution is about 1 sec, and memory requirements are +/- zero.

Minimally it looks so CALL statements has a memory issue.

Regards

Pavel

  2. Re: calling procedures is slow and consumes extra much memory against calling function

    Pavel Stehule <pavel.stehule@gmail.com> — 2020-05-11T05:25:20Z

    Hi

ne 10. 5. 2020 v 22:20 odesílatel Pavel Stehule <pavel.stehule@gmail.com>
napsal:

> Hi
>
> I try to use procedures in Orafce package, and I did some easy performance
> tests. I found some hard problems:
>
> 1. test case
>
> create or replace procedure p1(inout r int, inout v int) as $$
> begin v := random() * r; end
> $$ language plpgsql;
>
> This command requires
>
> do $$
> declare r int default 100; x int;
> begin
>   for i in 1..300000 loop
>      call p1(r, x);
>   end loop;
> end;
> $$;
>
> about 2.2GB RAM and 10 sec.
>
> When I rewrite same to functions then
>
> create or replace function p1func2(inout r int, inout v int) as $$
> begin v := random() * r; end
> $$ language plpgsql;
>
> do $$
> declare r int default 100; x int; re record;
> begin
>   for i in 1..300000 loop
>      re := p1func2(r, x);
>   end loop;
> end;
> $$;
>
> Then execution is about 1 sec, and memory requirements are +/- zero.
>
> Minimally it looks so CALL statements has a memory issue.
>

The problem is in plpgsql implementation of CALL statement

In non atomic case -  case of using procedures from DO block, the
expression plan is not cached, and plan is generating any time. This is
reason why it is slow.

Unfortunately, generated plans are not released until SPI_finish. Attached
patch fixed this issue.

Regards

Pavel


> Regards
>
> Pavel
>
>

  3. Re: calling procedures is slow and consumes extra much memory against calling function

    Michael Paquier <michael@paquier.xyz> — 2020-05-11T06:07:34Z

    On Sun, May 10, 2020 at 10:20:53PM +0200, Pavel Stehule wrote:
> When I rewrite same to functions then
>
> create or replace function p1func2(inout r int, inout v int) as $$
> begin v := random() * r; end
> $$ language plpgsql;
>
> Then execution is about 1 sec, and memory requirements are +/- zero.
>
> Minimally it looks so CALL statements has a memory issue.

Behavior not limited to plpgsql.  A plain SQL function shows the same
leak patterns:
create or replace procedure p1_sql(in r int, in v int)
  as $$ SELECT r + v; $$ language sql;
  And I cannot get valgrind to complain about lost references, so this
  looks like some missing memory context handling.

Also, I actually don't quite get why the context created by
CreateExprContext() cannot be freed before the procedure returns.  A
short test shows no problems in calling FreeExprContext() at the end
of ExecuteCallStmt(), but that does not address everything.  Perhaps a
lack of tests with pass-by-reference expressions and procedures?

Peter?
--
Michael

  4. Re: calling procedures is slow and consumes extra much memory against calling function

    Pavel Stehule <pavel.stehule@gmail.com> — 2020-05-11T06:07:48Z

    po 11. 5. 2020 v 7:25 odesílatel Pavel Stehule <pavel.stehule@gmail.com>
napsal:

> Hi
>
> ne 10. 5. 2020 v 22:20 odesílatel Pavel Stehule <pavel.stehule@gmail.com>
> napsal:
>
>> Hi
>>
>> I try to use procedures in Orafce package, and I did some easy
>> performance tests. I found some hard problems:
>>
>> 1. test case
>>
>> create or replace procedure p1(inout r int, inout v int) as $$
>> begin v := random() * r; end
>> $$ language plpgsql;
>>
>> This command requires
>>
>> do $$
>> declare r int default 100; x int;
>> begin
>>   for i in 1..300000 loop
>>      call p1(r, x);
>>   end loop;
>> end;
>> $$;
>>
>> about 2.2GB RAM and 10 sec.
>>
>> When I rewrite same to functions then
>>
>> create or replace function p1func2(inout r int, inout v int) as $$
>> begin v := random() * r; end
>> $$ language plpgsql;
>>
>> do $$
>> declare r int default 100; x int; re record;
>> begin
>>   for i in 1..300000 loop
>>      re := p1func2(r, x);
>>   end loop;
>> end;
>> $$;
>>
>> Then execution is about 1 sec, and memory requirements are +/- zero.
>>
>> Minimally it looks so CALL statements has a memory issue.
>>
>
> The problem is in plpgsql implementation of CALL statement
>
> In non atomic case -  case of using procedures from DO block, the
> expression plan is not cached, and plan is generating any time. This is
> reason why it is slow.
>
> Unfortunately, generated plans are not released until SPI_finish. Attached
> patch fixed this issue.
>

But now, recursive calling doesn't work :-(. So this patch is not enough



> Regards
>
> Pavel
>
>
>> Regards
>>
>> Pavel
>>
>>

  5. Re: calling procedures is slow and consumes extra much memory against calling function

    Pavel Stehule <pavel.stehule@gmail.com> — 2020-05-15T18:36:20Z

    Hi


> The problem is in plpgsql implementation of CALL statement
>>
>> In non atomic case -  case of using procedures from DO block, the
>> expression plan is not cached, and plan is generating any time. This is
>> reason why it is slow.
>>
>> Unfortunately, generated plans are not released until SPI_finish.
>> Attached patch fixed this issue.
>>
>
> But now, recursive calling doesn't work :-(. So this patch is not enough
>

Attached patch is working - all tests passed

It doesn't solve performance, and doesn't solve all memory problems, but
significantly reduce memory requirements from 5007 bytes to 439 bytes per
one CALL

Regards

Pavel


>
>
>> Regards
>>
>> Pavel
>>
>>
>>> Regards
>>>
>>> Pavel
>>>
>>>

  6. Re: calling procedures is slow and consumes extra much memory against calling function

    Ranier Vilela <ranier.vf@gmail.com> — 2020-05-15T22:33:45Z

    Em dom., 10 de mai. de 2020 às 17:21, Pavel Stehule <pavel.stehule@gmail.com>
escreveu:

> Hi
>
> I try to use procedures in Orafce package, and I did some easy performance
> tests. I found some hard problems:
>
> 1. test case
>
> create or replace procedure p1(inout r int, inout v int) as $$
> begin v := random() * r; end
> $$ language plpgsql;
>
> This command requires
>
> do $$
> declare r int default 100; x int;
> begin
>   for i in 1..300000 loop
>      call p1(r, x);
>   end loop;
> end;
> $$;
>
> about 2.2GB RAM and 10 sec.
>
I am having a consistent result of 3 secs, with a modified version
(exec_stmt_call) of your patch.
But my notebook is (Core 5, 8GB and SSD), could it be a difference in the
testing hardware?

regards,
Ranier Vilela

  7. Re: calling procedures is slow and consumes extra much memory against calling function

    Pavel Stehule <pavel.stehule@gmail.com> — 2020-05-16T03:06:34Z

    so 16. 5. 2020 v 0:34 odesílatel Ranier Vilela <ranier.vf@gmail.com> napsal:

> Em dom., 10 de mai. de 2020 às 17:21, Pavel Stehule <
> pavel.stehule@gmail.com> escreveu:
>
>> Hi
>>
>> I try to use procedures in Orafce package, and I did some easy
>> performance tests. I found some hard problems:
>>
>> 1. test case
>>
>> create or replace procedure p1(inout r int, inout v int) as $$
>> begin v := random() * r; end
>> $$ language plpgsql;
>>
>> This command requires
>>
>> do $$
>> declare r int default 100; x int;
>> begin
>>   for i in 1..300000 loop
>>      call p1(r, x);
>>   end loop;
>> end;
>> $$;
>>
>> about 2.2GB RAM and 10 sec.
>>
> I am having a consistent result of 3 secs, with a modified version
> (exec_stmt_call) of your patch.
> But my notebook is (Core 5, 8GB and SSD), could it be a difference in the
> testing hardware?
>

My notebook is old T520, and more I have a configured Postgres with
--enable-cassert option.

regards

Pavel


> regards,
> Ranier Vilela
>

  8. Re: calling procedures is slow and consumes extra much memory against calling function

    Ranier Vilela <ranier.vf@gmail.com> — 2020-05-16T03:54:47Z

    Em sáb., 16 de mai. de 2020 às 00:07, Pavel Stehule <pavel.stehule@gmail.com>
escreveu:

>
>
> so 16. 5. 2020 v 0:34 odesílatel Ranier Vilela <ranier.vf@gmail.com>
> napsal:
>
>> Em dom., 10 de mai. de 2020 às 17:21, Pavel Stehule <
>> pavel.stehule@gmail.com> escreveu:
>>
>>> Hi
>>>
>>> I try to use procedures in Orafce package, and I did some easy
>>> performance tests. I found some hard problems:
>>>
>>> 1. test case
>>>
>>> create or replace procedure p1(inout r int, inout v int) as $$
>>> begin v := random() * r; end
>>> $$ language plpgsql;
>>>
>>> This command requires
>>>
>>> do $$
>>> declare r int default 100; x int;
>>> begin
>>>   for i in 1..300000 loop
>>>      call p1(r, x);
>>>   end loop;
>>> end;
>>> $$;
>>>
>>> about 2.2GB RAM and 10 sec.
>>>
>> I am having a consistent result of 3 secs, with a modified version
>> (exec_stmt_call) of your patch.
>> But my notebook is (Core 5, 8GB and SSD), could it be a difference in the
>> testing hardware?
>>
>
> My notebook is old T520, and more I have a configured Postgres with
> --enable-cassert option.
>
The hardware is definitely making a difference, but if you have time and
don't mind testing it,
I can send you a patch, not that the modifications are a big deal, but
maybe they'll help.

regards,
Ranier Vilela

  9. Re: calling procedures is slow and consumes extra much memory against calling function

    Pavel Stehule <pavel.stehule@gmail.com> — 2020-05-16T04:10:03Z

    so 16. 5. 2020 v 5:55 odesílatel Ranier Vilela <ranier.vf@gmail.com> napsal:

> Em sáb., 16 de mai. de 2020 às 00:07, Pavel Stehule <
> pavel.stehule@gmail.com> escreveu:
>
>>
>>
>> so 16. 5. 2020 v 0:34 odesílatel Ranier Vilela <ranier.vf@gmail.com>
>> napsal:
>>
>>> Em dom., 10 de mai. de 2020 às 17:21, Pavel Stehule <
>>> pavel.stehule@gmail.com> escreveu:
>>>
>>>> Hi
>>>>
>>>> I try to use procedures in Orafce package, and I did some easy
>>>> performance tests. I found some hard problems:
>>>>
>>>> 1. test case
>>>>
>>>> create or replace procedure p1(inout r int, inout v int) as $$
>>>> begin v := random() * r; end
>>>> $$ language plpgsql;
>>>>
>>>> This command requires
>>>>
>>>> do $$
>>>> declare r int default 100; x int;
>>>> begin
>>>>   for i in 1..300000 loop
>>>>      call p1(r, x);
>>>>   end loop;
>>>> end;
>>>> $$;
>>>>
>>>> about 2.2GB RAM and 10 sec.
>>>>
>>> I am having a consistent result of 3 secs, with a modified version
>>> (exec_stmt_call) of your patch.
>>> But my notebook is (Core 5, 8GB and SSD), could it be a difference in
>>> the testing hardware?
>>>
>>
>> My notebook is old T520, and more I have a configured Postgres with
>> --enable-cassert option.
>>
> The hardware is definitely making a difference, but if you have time and
> don't mind testing it,
> I can send you a patch, not that the modifications are a big deal, but
> maybe they'll help.
>

send me a patch, please

Pavel


>
> regards,
> Ranier Vilela
>

  10. Re: calling procedures is slow and consumes extra much memory against calling function

    Ranier Vilela <ranier.vf@gmail.com> — 2020-05-16T11:39:18Z

    Em sáb., 16 de mai. de 2020 às 01:10, Pavel Stehule <pavel.stehule@gmail.com>
escreveu:

>
>
> so 16. 5. 2020 v 5:55 odesílatel Ranier Vilela <ranier.vf@gmail.com>
> napsal:
>
>> Em sáb., 16 de mai. de 2020 às 00:07, Pavel Stehule <
>> pavel.stehule@gmail.com> escreveu:
>>
>>>
>>>
>>> so 16. 5. 2020 v 0:34 odesílatel Ranier Vilela <ranier.vf@gmail.com>
>>> napsal:
>>>
>>>> Em dom., 10 de mai. de 2020 às 17:21, Pavel Stehule <
>>>> pavel.stehule@gmail.com> escreveu:
>>>>
>>>>> Hi
>>>>>
>>>>> I try to use procedures in Orafce package, and I did some easy
>>>>> performance tests. I found some hard problems:
>>>>>
>>>>> 1. test case
>>>>>
>>>>> create or replace procedure p1(inout r int, inout v int) as $$
>>>>> begin v := random() * r; end
>>>>> $$ language plpgsql;
>>>>>
>>>>> This command requires
>>>>>
>>>>> do $$
>>>>> declare r int default 100; x int;
>>>>> begin
>>>>>   for i in 1..300000 loop
>>>>>      call p1(r, x);
>>>>>   end loop;
>>>>> end;
>>>>> $$;
>>>>>
>>>>> about 2.2GB RAM and 10 sec.
>>>>>
>>>> I am having a consistent result of 3 secs, with a modified version
>>>> (exec_stmt_call) of your patch.
>>>> But my notebook is (Core 5, 8GB and SSD), could it be a difference in
>>>> the testing hardware?
>>>>
>>>
>>> My notebook is old T520, and more I have a configured Postgres with
>>> --enable-cassert option.
>>>
>> The hardware is definitely making a difference, but if you have time and
>> don't mind testing it,
>> I can send you a patch, not that the modifications are a big deal, but
>> maybe they'll help.
>>
> With more testing, I found that latency increases response time.
With 3 (secs) the test is with localhost.
With 6 (secs) the test is with tcp (local, not between pcs).

Anyway, I would like to know if we have the number of parameters
previously, why use List instead of Arrays?
It would not be faster to create plpgsql variables.

regards,
Ranier Vilela

  11. Re: calling procedures is slow and consumes extra much memory against calling function

    Pavel Stehule <pavel.stehule@gmail.com> — 2020-05-16T12:34:42Z

    so 16. 5. 2020 v 13:40 odesílatel Ranier Vilela <ranier.vf@gmail.com>
napsal:

> Em sáb., 16 de mai. de 2020 às 01:10, Pavel Stehule <
> pavel.stehule@gmail.com> escreveu:
>
>>
>>
>> so 16. 5. 2020 v 5:55 odesílatel Ranier Vilela <ranier.vf@gmail.com>
>> napsal:
>>
>>> Em sáb., 16 de mai. de 2020 às 00:07, Pavel Stehule <
>>> pavel.stehule@gmail.com> escreveu:
>>>
>>>>
>>>>
>>>> so 16. 5. 2020 v 0:34 odesílatel Ranier Vilela <ranier.vf@gmail.com>
>>>> napsal:
>>>>
>>>>> Em dom., 10 de mai. de 2020 às 17:21, Pavel Stehule <
>>>>> pavel.stehule@gmail.com> escreveu:
>>>>>
>>>>>> Hi
>>>>>>
>>>>>> I try to use procedures in Orafce package, and I did some easy
>>>>>> performance tests. I found some hard problems:
>>>>>>
>>>>>> 1. test case
>>>>>>
>>>>>> create or replace procedure p1(inout r int, inout v int) as $$
>>>>>> begin v := random() * r; end
>>>>>> $$ language plpgsql;
>>>>>>
>>>>>> This command requires
>>>>>>
>>>>>> do $$
>>>>>> declare r int default 100; x int;
>>>>>> begin
>>>>>>   for i in 1..300000 loop
>>>>>>      call p1(r, x);
>>>>>>   end loop;
>>>>>> end;
>>>>>> $$;
>>>>>>
>>>>>> about 2.2GB RAM and 10 sec.
>>>>>>
>>>>> I am having a consistent result of 3 secs, with a modified version
>>>>> (exec_stmt_call) of your patch.
>>>>> But my notebook is (Core 5, 8GB and SSD), could it be a difference in
>>>>> the testing hardware?
>>>>>
>>>>
>>>> My notebook is old T520, and more I have a configured Postgres with
>>>> --enable-cassert option.
>>>>
>>> The hardware is definitely making a difference, but if you have time and
>>> don't mind testing it,
>>> I can send you a patch, not that the modifications are a big deal, but
>>> maybe they'll help.
>>>
>> With more testing, I found that latency increases response time.
> With 3 (secs) the test is with localhost.
> With 6 (secs) the test is with tcp (local, not between pcs).
>
> Anyway, I would like to know if we have the number of parameters
> previously, why use List instead of Arrays?
> It would not be faster to create plpgsql variables.
>

Why you check SPI_processed?

+ if (SPI_processed == 1)
+ {
+ if (!stmt->target)
+ elog(ERROR, "DO statement returned a row, query \"%s\"", expr->query);
+ }
+ else if (SPI_processed > 1)
+ elog(ERROR, "Procedure call returned more than one row, query \"%s\"",
expr->query);


CALL cannot to return rows, so these checks has not sense



> regards,
> Ranier Vilela
>

  12. Re: calling procedures is slow and consumes extra much memory against calling function

    Ranier Vilela <ranier.vf@gmail.com> — 2020-05-16T13:23:09Z

    Em sáb., 16 de mai. de 2020 às 09:35, Pavel Stehule <pavel.stehule@gmail.com>
escreveu:

>
>
> so 16. 5. 2020 v 13:40 odesílatel Ranier Vilela <ranier.vf@gmail.com>
> napsal:
>
>> Em sáb., 16 de mai. de 2020 às 01:10, Pavel Stehule <
>> pavel.stehule@gmail.com> escreveu:
>>
>>>
>>>
>>> so 16. 5. 2020 v 5:55 odesílatel Ranier Vilela <ranier.vf@gmail.com>
>>> napsal:
>>>
>>>> Em sáb., 16 de mai. de 2020 às 00:07, Pavel Stehule <
>>>> pavel.stehule@gmail.com> escreveu:
>>>>
>>>>>
>>>>>
>>>>> so 16. 5. 2020 v 0:34 odesílatel Ranier Vilela <ranier.vf@gmail.com>
>>>>> napsal:
>>>>>
>>>>>> Em dom., 10 de mai. de 2020 às 17:21, Pavel Stehule <
>>>>>> pavel.stehule@gmail.com> escreveu:
>>>>>>
>>>>>>> Hi
>>>>>>>
>>>>>>> I try to use procedures in Orafce package, and I did some easy
>>>>>>> performance tests. I found some hard problems:
>>>>>>>
>>>>>>> 1. test case
>>>>>>>
>>>>>>> create or replace procedure p1(inout r int, inout v int) as $$
>>>>>>> begin v := random() * r; end
>>>>>>> $$ language plpgsql;
>>>>>>>
>>>>>>> This command requires
>>>>>>>
>>>>>>> do $$
>>>>>>> declare r int default 100; x int;
>>>>>>> begin
>>>>>>>   for i in 1..300000 loop
>>>>>>>      call p1(r, x);
>>>>>>>   end loop;
>>>>>>> end;
>>>>>>> $$;
>>>>>>>
>>>>>>> about 2.2GB RAM and 10 sec.
>>>>>>>
>>>>>> I am having a consistent result of 3 secs, with a modified version
>>>>>> (exec_stmt_call) of your patch.
>>>>>> But my notebook is (Core 5, 8GB and SSD), could it be a difference in
>>>>>> the testing hardware?
>>>>>>
>>>>>
>>>>> My notebook is old T520, and more I have a configured Postgres with
>>>>> --enable-cassert option.
>>>>>
>>>> The hardware is definitely making a difference, but if you have time
>>>> and don't mind testing it,
>>>> I can send you a patch, not that the modifications are a big deal, but
>>>> maybe they'll help.
>>>>
>>> With more testing, I found that latency increases response time.
>> With 3 (secs) the test is with localhost.
>> With 6 (secs) the test is with tcp (local, not between pcs).
>>
>> Anyway, I would like to know if we have the number of parameters
>> previously, why use List instead of Arrays?
>> It would not be faster to create plpgsql variables.
>>
>
> Why you check SPI_processed?
>
> + if (SPI_processed == 1)
> + {
> + if (!stmt->target)
> + elog(ERROR, "DO statement returned a row, query \"%s\"", expr->query);
> + }
> + else if (SPI_processed > 1)
> + elog(ERROR, "Procedure call returned more than one row, query \"%s\"",
> expr->query);
>
>
> CALL cannot to return rows, so these checks has not sense
>
Looking at the original file, this already done, from line 2351,
I just put all the tests together to, if applicable, get out quickly.

regards,
Ranier Vilela

  13. Re: calling procedures is slow and consumes extra much memory against calling function

    Pavel Stehule <pavel.stehule@gmail.com> — 2020-05-16T14:06:40Z

    so 16. 5. 2020 v 15:24 odesílatel Ranier Vilela <ranier.vf@gmail.com>
napsal:

> Em sáb., 16 de mai. de 2020 às 09:35, Pavel Stehule <
> pavel.stehule@gmail.com> escreveu:
>
>>
>>
>> so 16. 5. 2020 v 13:40 odesílatel Ranier Vilela <ranier.vf@gmail.com>
>> napsal:
>>
>>> Em sáb., 16 de mai. de 2020 às 01:10, Pavel Stehule <
>>> pavel.stehule@gmail.com> escreveu:
>>>
>>>>
>>>>
>>>> so 16. 5. 2020 v 5:55 odesílatel Ranier Vilela <ranier.vf@gmail.com>
>>>> napsal:
>>>>
>>>>> Em sáb., 16 de mai. de 2020 às 00:07, Pavel Stehule <
>>>>> pavel.stehule@gmail.com> escreveu:
>>>>>
>>>>>>
>>>>>>
>>>>>> so 16. 5. 2020 v 0:34 odesílatel Ranier Vilela <ranier.vf@gmail.com>
>>>>>> napsal:
>>>>>>
>>>>>>> Em dom., 10 de mai. de 2020 às 17:21, Pavel Stehule <
>>>>>>> pavel.stehule@gmail.com> escreveu:
>>>>>>>
>>>>>>>> Hi
>>>>>>>>
>>>>>>>> I try to use procedures in Orafce package, and I did some easy
>>>>>>>> performance tests. I found some hard problems:
>>>>>>>>
>>>>>>>> 1. test case
>>>>>>>>
>>>>>>>> create or replace procedure p1(inout r int, inout v int) as $$
>>>>>>>> begin v := random() * r; end
>>>>>>>> $$ language plpgsql;
>>>>>>>>
>>>>>>>> This command requires
>>>>>>>>
>>>>>>>> do $$
>>>>>>>> declare r int default 100; x int;
>>>>>>>> begin
>>>>>>>>   for i in 1..300000 loop
>>>>>>>>      call p1(r, x);
>>>>>>>>   end loop;
>>>>>>>> end;
>>>>>>>> $$;
>>>>>>>>
>>>>>>>> about 2.2GB RAM and 10 sec.
>>>>>>>>
>>>>>>> I am having a consistent result of 3 secs, with a modified version
>>>>>>> (exec_stmt_call) of your patch.
>>>>>>> But my notebook is (Core 5, 8GB and SSD), could it be a difference
>>>>>>> in the testing hardware?
>>>>>>>
>>>>>>
>>>>>> My notebook is old T520, and more I have a configured Postgres with
>>>>>> --enable-cassert option.
>>>>>>
>>>>> The hardware is definitely making a difference, but if you have time
>>>>> and don't mind testing it,
>>>>> I can send you a patch, not that the modifications are a big deal, but
>>>>> maybe they'll help.
>>>>>
>>>> With more testing, I found that latency increases response time.
>>> With 3 (secs) the test is with localhost.
>>> With 6 (secs) the test is with tcp (local, not between pcs).
>>>
>>> Anyway, I would like to know if we have the number of parameters
>>> previously, why use List instead of Arrays?
>>> It would not be faster to create plpgsql variables.
>>>
>>
>> Why you check SPI_processed?
>>
>> + if (SPI_processed == 1)
>> + {
>> + if (!stmt->target)
>> + elog(ERROR, "DO statement returned a row, query \"%s\"", expr->query);
>> + }
>> + else if (SPI_processed > 1)
>> + elog(ERROR, "Procedure call returned more than one row, query \"%s\"",
>> expr->query);
>>
>>
>> CALL cannot to return rows, so these checks has not sense
>>
> Looking at the original file, this already done, from line 2351,
> I just put all the tests together to, if applicable, get out quickly.
>

It's little bit messy. Is not good to mix bugfix and refactoring things
together



> regards,
> Ranier Vilela
>

  14. Re: calling procedures is slow and consumes extra much memory against calling function

    Ranier Vilela <ranier.vf@gmail.com> — 2020-05-16T14:20:32Z

    Em sáb., 16 de mai. de 2020 às 11:07, Pavel Stehule <pavel.stehule@gmail.com>
escreveu:

>
>
> so 16. 5. 2020 v 15:24 odesílatel Ranier Vilela <ranier.vf@gmail.com>
> napsal:
>
>> Em sáb., 16 de mai. de 2020 às 09:35, Pavel Stehule <
>> pavel.stehule@gmail.com> escreveu:
>>
>>>
>>>
>>> so 16. 5. 2020 v 13:40 odesílatel Ranier Vilela <ranier.vf@gmail.com>
>>> napsal:
>>>
>>>> Em sáb., 16 de mai. de 2020 às 01:10, Pavel Stehule <
>>>> pavel.stehule@gmail.com> escreveu:
>>>>
>>>>>
>>>>>
>>>>> so 16. 5. 2020 v 5:55 odesílatel Ranier Vilela <ranier.vf@gmail.com>
>>>>> napsal:
>>>>>
>>>>>> Em sáb., 16 de mai. de 2020 às 00:07, Pavel Stehule <
>>>>>> pavel.stehule@gmail.com> escreveu:
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> so 16. 5. 2020 v 0:34 odesílatel Ranier Vilela <ranier.vf@gmail.com>
>>>>>>> napsal:
>>>>>>>
>>>>>>>> Em dom., 10 de mai. de 2020 às 17:21, Pavel Stehule <
>>>>>>>> pavel.stehule@gmail.com> escreveu:
>>>>>>>>
>>>>>>>>> Hi
>>>>>>>>>
>>>>>>>>> I try to use procedures in Orafce package, and I did some easy
>>>>>>>>> performance tests. I found some hard problems:
>>>>>>>>>
>>>>>>>>> 1. test case
>>>>>>>>>
>>>>>>>>> create or replace procedure p1(inout r int, inout v int) as $$
>>>>>>>>> begin v := random() * r; end
>>>>>>>>> $$ language plpgsql;
>>>>>>>>>
>>>>>>>>> This command requires
>>>>>>>>>
>>>>>>>>> do $$
>>>>>>>>> declare r int default 100; x int;
>>>>>>>>> begin
>>>>>>>>>   for i in 1..300000 loop
>>>>>>>>>      call p1(r, x);
>>>>>>>>>   end loop;
>>>>>>>>> end;
>>>>>>>>> $$;
>>>>>>>>>
>>>>>>>>> about 2.2GB RAM and 10 sec.
>>>>>>>>>
>>>>>>>> I am having a consistent result of 3 secs, with a modified version
>>>>>>>> (exec_stmt_call) of your patch.
>>>>>>>> But my notebook is (Core 5, 8GB and SSD), could it be a difference
>>>>>>>> in the testing hardware?
>>>>>>>>
>>>>>>>
>>>>>>> My notebook is old T520, and more I have a configured Postgres with
>>>>>>> --enable-cassert option.
>>>>>>>
>>>>>> The hardware is definitely making a difference, but if you have time
>>>>>> and don't mind testing it,
>>>>>> I can send you a patch, not that the modifications are a big deal,
>>>>>> but maybe they'll help.
>>>>>>
>>>>> With more testing, I found that latency increases response time.
>>>> With 3 (secs) the test is with localhost.
>>>> With 6 (secs) the test is with tcp (local, not between pcs).
>>>>
>>>> Anyway, I would like to know if we have the number of parameters
>>>> previously, why use List instead of Arrays?
>>>> It would not be faster to create plpgsql variables.
>>>>
>>>
>>> Why you check SPI_processed?
>>>
>>> + if (SPI_processed == 1)
>>> + {
>>> + if (!stmt->target)
>>> + elog(ERROR, "DO statement returned a row, query \"%s\"", expr->query);
>>> + }
>>> + else if (SPI_processed > 1)
>>> + elog(ERROR, "Procedure call returned more than one row, query \"%s\"",
>>> expr->query);
>>>
>>>
>>> CALL cannot to return rows, so these checks has not sense
>>>
>> Looking at the original file, this already done, from line 2351,
>> I just put all the tests together to, if applicable, get out quickly.
>>
>
> It's little bit messy. Is not good to mix bugfix and refactoring things
> together
>
Ok, I can understand that.

regards,
Ranier Vilela

  15. Re: calling procedures is slow and consumes extra much memory against calling function

    Amit Khandekar <amitdkhan.pg@gmail.com> — 2020-06-10T09:52:25Z

    On Sat, 16 May 2020 at 00:07, Pavel Stehule <pavel.stehule@gmail.com> wrote:
>
> Hi
>
>>>
>>> The problem is in plpgsql implementation of CALL statement
>>>
>>> In non atomic case -  case of using procedures from DO block, the expression plan is not cached, and plan is generating any time. This is reason why it is slow.
>>>
>>> Unfortunately, generated plans are not released until SPI_finish. Attached patch fixed this issue.
>>
>>
>> But now, recursive calling doesn't work :-(. So this patch is not enough
>
>
> Attached patch is working - all tests passed

Could you show an example testcase that tests this recursive scenario,
with which your earlier patch fails the test, and this v2 patch passes
it ? I am trying to understand the recursive scenario and the re-use
of expr->plan.

>
> It doesn't solve performance, and doesn't solve all memory problems, but significantly reduce memory requirements from 5007 bytes to 439 bytes per one CALL

So now  this patch's intention is to reduce memory consumption, and it
doesn't target slowness improvement, right ?

-- 
Thanks,
-Amit Khandekar
Huawei Technologies

  16. Re: calling procedures is slow and consumes extra much memory against calling function

    Pavel Stehule <pavel.stehule@gmail.com> — 2020-06-10T11:42:11Z

    st 10. 6. 2020 v 12:26 odesílatel Amit Khandekar <amitdkhan.pg@gmail.com>
napsal:

> On Sat, 16 May 2020 at 00:07, Pavel Stehule <pavel.stehule@gmail.com>
> wrote:
> >
> > Hi
> >
> >>>
> >>> The problem is in plpgsql implementation of CALL statement
> >>>
> >>> In non atomic case -  case of using procedures from DO block, the
> expression plan is not cached, and plan is generating any time. This is
> reason why it is slow.
> >>>
> >>> Unfortunately, generated plans are not released until SPI_finish.
> Attached patch fixed this issue.
> >>
> >>
> >> But now, recursive calling doesn't work :-(. So this patch is not enough
> >
> >
> > Attached patch is working - all tests passed
>
> Could you show an example testcase that tests this recursive scenario,
> with which your earlier patch fails the test, and this v2 patch passes
> it ? I am trying to understand the recursive scenario and the re-use
> of expr->plan.
>

it hangs on plpgsql tests. So you can apply first version of patch

and "make check"


> >
> > It doesn't solve performance, and doesn't solve all memory problems, but
> significantly reduce memory requirements from 5007 bytes to 439 bytes per
> one CALL
>
> So now  this patch's intention is to reduce memory consumption, and it
> doesn't target slowness improvement, right ?
>

yes. There is a problem with planning every execution when the procedure
was called from not top context.



> --
> Thanks,
> -Amit Khandekar
> Huawei Technologies
>

  17. Re: calling procedures is slow and consumes extra much memory against calling function

    Amit Khandekar <amitdkhan.pg@gmail.com> — 2020-06-17T05:52:04Z

    On Wed, 10 Jun 2020 at 17:12, Pavel Stehule <pavel.stehule@gmail.com> wrote:
> st 10. 6. 2020 v 12:26 odesílatel Amit Khandekar <amitdkhan.pg@gmail.com> napsal:
>> Could you show an example testcase that tests this recursive scenario,
>> with which your earlier patch fails the test, and this v2 patch passes
>> it ? I am trying to understand the recursive scenario and the re-use
>> of expr->plan.
>
>
> it hangs on plpgsql tests. So you can apply first version of patch
>
> and "make check"

I could not reproduce the make check hang with the v1 patch. But I
could see a crash with the below testcase. So I understand the purpose
of the plan_owner variable that you introduced in v2.

Consider this recursive test :

create or replace procedure p1(in r int) as $$
begin
   RAISE INFO 'r : % ', r;
   if r < 3 then
      call p1(r+1);
   end if;
end
$$ language plpgsql;

do $$
declare r int default 1;
begin
    call p1(r);
end;
$$;

In p1() with r=2, when the stmt "call p1(r+1)" is being executed,
consider this code of exec_stmt_call() with your v2 patch applied:
if (expr->plan && !expr->plan->saved)
{
   if (plan_owner)
      SPI_freeplan(expr->plan);
   expr->plan = NULL;
}

Here, plan_owner is false. So SPI_freeplan() will not be called, and
expr->plan is set to NULL. Now I have observed that the stmt pointer
and expr pointer is shared between the p1() execution at this r=2
level and the p1() execution at r=1 level. So after the above code is
executed at r=2, when the upper level (r=1) exec_stmt_call() lands to
the same above code snippet, it gets the same expr pointer, but it's
expr->plan is already set to NULL without being freed. From this
logic, it looks like the plan won't get freed whenever the expr/stmt
pointers are shared across recursive levels, since expr->plan is set
to NULL at the lowermost level ? Basically, the handle to the plan is
lost so no one at the upper recursion level can explicitly free it
using SPI_freeplan(), right ? This looks the same as the main issue
where the plan does not get freed for non-recursive calls. I haven't
got a chance to check if we can develop a testcase for this, similar
to your testcase where the memory keeps on increasing.

-Amit

  18. Re: calling procedures is slow and consumes extra much memory against calling function

    Pavel Stehule <pavel.stehule@gmail.com> — 2020-06-17T08:23:44Z

    st 17. 6. 2020 v 7:52 odesílatel Amit Khandekar <amitdkhan.pg@gmail.com>
napsal:

> On Wed, 10 Jun 2020 at 17:12, Pavel Stehule <pavel.stehule@gmail.com>
> wrote:
> > st 10. 6. 2020 v 12:26 odesílatel Amit Khandekar <amitdkhan.pg@gmail.com>
> napsal:
> >> Could you show an example testcase that tests this recursive scenario,
> >> with which your earlier patch fails the test, and this v2 patch passes
> >> it ? I am trying to understand the recursive scenario and the re-use
> >> of expr->plan.
> >
> >
> > it hangs on plpgsql tests. So you can apply first version of patch
> >
> > and "make check"
>
> I could not reproduce the make check hang with the v1 patch. But I
> could see a crash with the below testcase. So I understand the purpose
> of the plan_owner variable that you introduced in v2.
>
> Consider this recursive test :
>
> create or replace procedure p1(in r int) as $$
> begin
>    RAISE INFO 'r : % ', r;
>    if r < 3 then
>       call p1(r+1);
>    end if;
> end
> $$ language plpgsql;
>
> do $$
> declare r int default 1;
> begin
>     call p1(r);
> end;
> $$;
>
> In p1() with r=2, when the stmt "call p1(r+1)" is being executed,
> consider this code of exec_stmt_call() with your v2 patch applied:
> if (expr->plan && !expr->plan->saved)
> {
>    if (plan_owner)
>       SPI_freeplan(expr->plan);
>    expr->plan = NULL;
> }
>
> Here, plan_owner is false. So SPI_freeplan() will not be called, and
> expr->plan is set to NULL. Now I have observed that the stmt pointer
> and expr pointer is shared between the p1() execution at this r=2
> level and the p1() execution at r=1 level. So after the above code is
> executed at r=2, when the upper level (r=1) exec_stmt_call() lands to
> the same above code snippet, it gets the same expr pointer, but it's
> expr->plan is already set to NULL without being freed. From this
> logic, it looks like the plan won't get freed whenever the expr/stmt
> pointers are shared across recursive levels, since expr->plan is set
> to NULL at the lowermost level ? Basically, the handle to the plan is
> lost so no one at the upper recursion level can explicitly free it
> using SPI_freeplan(), right ? This looks the same as the main issue
> where the plan does not get freed for non-recursive calls. I haven't
> got a chance to check if we can develop a testcase for this, similar
> to your testcase where the memory keeps on increasing.
>

This is a good consideration.

I am sending updated patch

Pavel



> -Amit
>

  19. Re: calling procedures is slow and consumes extra much memory against calling function

    Amit Khandekar <amitdkhan.pg@gmail.com> — 2020-07-09T06:27:34Z

    On Wed, 17 Jun 2020 at 13:54, Pavel Stehule <pavel.stehule@gmail.com> wrote:
>
>
>
> st 17. 6. 2020 v 7:52 odesílatel Amit Khandekar <amitdkhan.pg@gmail.com> napsal:
>>
>> On Wed, 10 Jun 2020 at 17:12, Pavel Stehule <pavel.stehule@gmail.com> wrote:
>> > st 10. 6. 2020 v 12:26 odesílatel Amit Khandekar <amitdkhan.pg@gmail.com> napsal:
>> >> Could you show an example testcase that tests this recursive scenario,
>> >> with which your earlier patch fails the test, and this v2 patch passes
>> >> it ? I am trying to understand the recursive scenario and the re-use
>> >> of expr->plan.
>> >
>> >
>> > it hangs on plpgsql tests. So you can apply first version of patch
>> >
>> > and "make check"
>>
>> I could not reproduce the make check hang with the v1 patch. But I
>> could see a crash with the below testcase. So I understand the purpose
>> of the plan_owner variable that you introduced in v2.
>>
>> Consider this recursive test :
>>
>> create or replace procedure p1(in r int) as $$
>> begin
>>    RAISE INFO 'r : % ', r;
>>    if r < 3 then
>>       call p1(r+1);
>>    end if;
>> end
>> $$ language plpgsql;
>>
>> do $$
>> declare r int default 1;
>> begin
>>     call p1(r);
>> end;
>> $$;
>>
>> In p1() with r=2, when the stmt "call p1(r+1)" is being executed,
>> consider this code of exec_stmt_call() with your v2 patch applied:
>> if (expr->plan && !expr->plan->saved)
>> {
>>    if (plan_owner)
>>       SPI_freeplan(expr->plan);
>>    expr->plan = NULL;
>> }
>>
>> Here, plan_owner is false. So SPI_freeplan() will not be called, and
>> expr->plan is set to NULL. Now I have observed that the stmt pointer
>> and expr pointer is shared between the p1() execution at this r=2
>> level and the p1() execution at r=1 level. So after the above code is
>> executed at r=2, when the upper level (r=1) exec_stmt_call() lands to
>> the same above code snippet, it gets the same expr pointer, but it's
>> expr->plan is already set to NULL without being freed. From this
>> logic, it looks like the plan won't get freed whenever the expr/stmt
>> pointers are shared across recursive levels, since expr->plan is set
>> to NULL at the lowermost level ? Basically, the handle to the plan is
>> lost so no one at the upper recursion level can explicitly free it
>> using SPI_freeplan(), right ? This looks the same as the main issue
>> where the plan does not get freed for non-recursive calls. I haven't
>> got a chance to check if we can develop a testcase for this, similar
>> to your testcase where the memory keeps on increasing.
>
>
> This is a good consideration.
>
> I am sending updated patch

Checked the latest patch. Looks like using a local plan rather than
expr->plan pointer for doing the checks does seem to resolve the issue
I raised. That made me think of another scenario :

Now we are checking for plan value and then null'ifying the expr->plan
value. What if expr->plan is different from plan ? Is it possible ? I
was thinking of such scenarios. But couldn't find one. As long as a
plan is always created with saved=true for all levels, or with
saved=false for all levels, we are ok. If we can have a mix of saved
and unsaved plans at different recursion levels, then expr->plan can
be different from the outer local plan because then the expr->plan
will not be set to NULL in the inner level, while the outer level may
have created its own plan. But I think a mix of saved and unsaved
plans are not possible. If you agree, then I think we should at least
have an assert that looks like :

    if (plan && !plan->saved)
    {
        if (plan_owner)
            SPI_freeplan(plan);

        /* If expr->plan  is present, it must be the same plan that we
allocated */
       Assert ( !expr->plan || plan == expr->plan) );

        expr->plan = NULL;
    }

Other than this, I have no other issues. I understand that we have to
do this special handling only for this exec_stmt_call() because it is
only here that we call exec_prepare_plan() with keep_plan = false, so
doing special handling for freeing the plan seems to make sense.

  20. Re: calling procedures is slow and consumes extra much memory against calling function

    Pavel Stehule <pavel.stehule@gmail.com> — 2020-07-11T05:38:21Z

    čt 9. 7. 2020 v 8:28 odesílatel Amit Khandekar <amitdkhan.pg@gmail.com>
napsal:

> On Wed, 17 Jun 2020 at 13:54, Pavel Stehule <pavel.stehule@gmail.com>
> wrote:
> >
> >
> >
> > st 17. 6. 2020 v 7:52 odesílatel Amit Khandekar <amitdkhan.pg@gmail.com>
> napsal:
> >>
> >> On Wed, 10 Jun 2020 at 17:12, Pavel Stehule <pavel.stehule@gmail.com>
> wrote:
> >> > st 10. 6. 2020 v 12:26 odesílatel Amit Khandekar <
> amitdkhan.pg@gmail.com> napsal:
> >> >> Could you show an example testcase that tests this recursive
> scenario,
> >> >> with which your earlier patch fails the test, and this v2 patch
> passes
> >> >> it ? I am trying to understand the recursive scenario and the re-use
> >> >> of expr->plan.
> >> >
> >> >
> >> > it hangs on plpgsql tests. So you can apply first version of patch
> >> >
> >> > and "make check"
> >>
> >> I could not reproduce the make check hang with the v1 patch. But I
> >> could see a crash with the below testcase. So I understand the purpose
> >> of the plan_owner variable that you introduced in v2.
> >>
> >> Consider this recursive test :
> >>
> >> create or replace procedure p1(in r int) as $$
> >> begin
> >>    RAISE INFO 'r : % ', r;
> >>    if r < 3 then
> >>       call p1(r+1);
> >>    end if;
> >> end
> >> $$ language plpgsql;
> >>
> >> do $$
> >> declare r int default 1;
> >> begin
> >>     call p1(r);
> >> end;
> >> $$;
> >>
> >> In p1() with r=2, when the stmt "call p1(r+1)" is being executed,
> >> consider this code of exec_stmt_call() with your v2 patch applied:
> >> if (expr->plan && !expr->plan->saved)
> >> {
> >>    if (plan_owner)
> >>       SPI_freeplan(expr->plan);
> >>    expr->plan = NULL;
> >> }
> >>
> >> Here, plan_owner is false. So SPI_freeplan() will not be called, and
> >> expr->plan is set to NULL. Now I have observed that the stmt pointer
> >> and expr pointer is shared between the p1() execution at this r=2
> >> level and the p1() execution at r=1 level. So after the above code is
> >> executed at r=2, when the upper level (r=1) exec_stmt_call() lands to
> >> the same above code snippet, it gets the same expr pointer, but it's
> >> expr->plan is already set to NULL without being freed. From this
> >> logic, it looks like the plan won't get freed whenever the expr/stmt
> >> pointers are shared across recursive levels, since expr->plan is set
> >> to NULL at the lowermost level ? Basically, the handle to the plan is
> >> lost so no one at the upper recursion level can explicitly free it
> >> using SPI_freeplan(), right ? This looks the same as the main issue
> >> where the plan does not get freed for non-recursive calls. I haven't
> >> got a chance to check if we can develop a testcase for this, similar
> >> to your testcase where the memory keeps on increasing.
> >
> >
> > This is a good consideration.
> >
> > I am sending updated patch
>
> Checked the latest patch. Looks like using a local plan rather than
> expr->plan pointer for doing the checks does seem to resolve the issue
> I raised. That made me think of another scenario :
>
> Now we are checking for plan value and then null'ifying the expr->plan
> value. What if expr->plan is different from plan ? Is it possible ? I
> was thinking of such scenarios. But couldn't find one. As long as a
> plan is always created with saved=true for all levels, or with
> saved=false for all levels, we are ok. If we can have a mix of saved
> and unsaved plans at different recursion levels, then expr->plan can
> be different from the outer local plan because then the expr->plan
> will not be set to NULL in the inner level, while the outer level may
> have created its own plan. But I think a mix of saved and unsaved
> plans are not possible. If you agree, then I think we should at least
> have an assert that looks like :
>
>     if (plan && !plan->saved)
>     {
>         if (plan_owner)
>             SPI_freeplan(plan);
>
>         /* If expr->plan  is present, it must be the same plan that we
> allocated */
>        Assert ( !expr->plan || plan == expr->plan) );
>
>         expr->plan = NULL;
>     }
>
> Other than this, I have no other issues. I understand that we have to
> do this special handling only for this exec_stmt_call() because it is
> only here that we call exec_prepare_plan() with keep_plan = false, so
> doing special handling for freeing the plan seems to make sense.
>

attached patch with assert.

all regress tests passed. I think this short patch can be applied on older
releases as bugfix.

This weekend I'll try to check different strategy - try to save a plan and
release it at the end of the transaction.

Regards

Pavel

  21. Re: calling procedures is slow and consumes extra much memory against calling function

    Pavel Stehule <pavel.stehule@gmail.com> — 2020-07-12T13:18:47Z

    so 11. 7. 2020 v 7:38 odesílatel Pavel Stehule <pavel.stehule@gmail.com>
napsal:

>
>
> čt 9. 7. 2020 v 8:28 odesílatel Amit Khandekar <amitdkhan.pg@gmail.com>
> napsal:
>
>> On Wed, 17 Jun 2020 at 13:54, Pavel Stehule <pavel.stehule@gmail.com>
>> wrote:
>> >
>> >
>> >
>> > st 17. 6. 2020 v 7:52 odesílatel Amit Khandekar <amitdkhan.pg@gmail.com>
>> napsal:
>> >>
>> >> On Wed, 10 Jun 2020 at 17:12, Pavel Stehule <pavel.stehule@gmail.com>
>> wrote:
>> >> > st 10. 6. 2020 v 12:26 odesílatel Amit Khandekar <
>> amitdkhan.pg@gmail.com> napsal:
>> >> >> Could you show an example testcase that tests this recursive
>> scenario,
>> >> >> with which your earlier patch fails the test, and this v2 patch
>> passes
>> >> >> it ? I am trying to understand the recursive scenario and the re-use
>> >> >> of expr->plan.
>> >> >
>> >> >
>> >> > it hangs on plpgsql tests. So you can apply first version of patch
>> >> >
>> >> > and "make check"
>> >>
>> >> I could not reproduce the make check hang with the v1 patch. But I
>> >> could see a crash with the below testcase. So I understand the purpose
>> >> of the plan_owner variable that you introduced in v2.
>> >>
>> >> Consider this recursive test :
>> >>
>> >> create or replace procedure p1(in r int) as $$
>> >> begin
>> >>    RAISE INFO 'r : % ', r;
>> >>    if r < 3 then
>> >>       call p1(r+1);
>> >>    end if;
>> >> end
>> >> $$ language plpgsql;
>> >>
>> >> do $$
>> >> declare r int default 1;
>> >> begin
>> >>     call p1(r);
>> >> end;
>> >> $$;
>> >>
>> >> In p1() with r=2, when the stmt "call p1(r+1)" is being executed,
>> >> consider this code of exec_stmt_call() with your v2 patch applied:
>> >> if (expr->plan && !expr->plan->saved)
>> >> {
>> >>    if (plan_owner)
>> >>       SPI_freeplan(expr->plan);
>> >>    expr->plan = NULL;
>> >> }
>> >>
>> >> Here, plan_owner is false. So SPI_freeplan() will not be called, and
>> >> expr->plan is set to NULL. Now I have observed that the stmt pointer
>> >> and expr pointer is shared between the p1() execution at this r=2
>> >> level and the p1() execution at r=1 level. So after the above code is
>> >> executed at r=2, when the upper level (r=1) exec_stmt_call() lands to
>> >> the same above code snippet, it gets the same expr pointer, but it's
>> >> expr->plan is already set to NULL without being freed. From this
>> >> logic, it looks like the plan won't get freed whenever the expr/stmt
>> >> pointers are shared across recursive levels, since expr->plan is set
>> >> to NULL at the lowermost level ? Basically, the handle to the plan is
>> >> lost so no one at the upper recursion level can explicitly free it
>> >> using SPI_freeplan(), right ? This looks the same as the main issue
>> >> where the plan does not get freed for non-recursive calls. I haven't
>> >> got a chance to check if we can develop a testcase for this, similar
>> >> to your testcase where the memory keeps on increasing.
>> >
>> >
>> > This is a good consideration.
>> >
>> > I am sending updated patch
>>
>> Checked the latest patch. Looks like using a local plan rather than
>> expr->plan pointer for doing the checks does seem to resolve the issue
>> I raised. That made me think of another scenario :
>>
>> Now we are checking for plan value and then null'ifying the expr->plan
>> value. What if expr->plan is different from plan ? Is it possible ? I
>> was thinking of such scenarios. But couldn't find one. As long as a
>> plan is always created with saved=true for all levels, or with
>> saved=false for all levels, we are ok. If we can have a mix of saved
>> and unsaved plans at different recursion levels, then expr->plan can
>> be different from the outer local plan because then the expr->plan
>> will not be set to NULL in the inner level, while the outer level may
>> have created its own plan. But I think a mix of saved and unsaved
>> plans are not possible. If you agree, then I think we should at least
>> have an assert that looks like :
>>
>>     if (plan && !plan->saved)
>>     {
>>         if (plan_owner)
>>             SPI_freeplan(plan);
>>
>>         /* If expr->plan  is present, it must be the same plan that we
>> allocated */
>>        Assert ( !expr->plan || plan == expr->plan) );
>>
>>         expr->plan = NULL;
>>     }
>>
>> Other than this, I have no other issues. I understand that we have to
>> do this special handling only for this exec_stmt_call() because it is
>> only here that we call exec_prepare_plan() with keep_plan = false, so
>> doing special handling for freeing the plan seems to make sense.
>>
>
> attached patch with assert.
>
> all regress tests passed. I think this short patch can be applied on older
> releases as bugfix.
>
> This weekend I'll try to check different strategy - try to save a plan and
> release it at the end of the transaction.
>

I check it, and this state of patch is good enough for this moment. Another
fix needs more invasive changes to handling plan cache.

Regards

Pavel


> Regards
>
> Pavel
>

  22. Re: calling procedures is slow and consumes extra much memory against calling function

    Pavel Stehule <pavel.stehule@gmail.com> — 2020-07-16T19:08:09Z

    Hi

I am sending another patch that tries to allow CachedPlans for CALL
statements. I think this patch is very accurate, but it is not nice,
because it is smudging very precious reference counting for CachedPlans.

Current issue:
==========

I found a problem with repeated CALL statements from DO command. For every
execution of a CALL statement a plan is created that is released at the
time of the end of DO block.

create or replace procedure insert_into_foo(i int)
as $$
begin
  insert into foo values(i, i || 'Ahoj');
  if i % 1000 = 0 then raise notice '%', i;
    --commit;
  end if;
end;
$$ language plpgsql;

and DO

do $$
begin
  for i in 1..500000
  loop
    call insert_into_foo(i);
  end loop;
end
$$;

Requires about 2.5GB RAM (execution time is 18 sec). The problem is "long
transaction" with 500M iteration of CALL statement.

If I try to remove a comment before COMMIT - then I get 500 transactions.
But still it needs 2.5GB memory.

The reason for this behaviour is disabling plan cache for CALL statements
executed in atomic mode.

So I wrote patch 1, that releases the not saved plan immediately. This
patch is very simple, and fixes memory issues. It is a little bit faster
(14 sec), and Postgres consumes about 200KB.

Patch 1 is simple, clean, nice but execution of CALL statements is slow due
repeated planning.

I tried to fix this issue another way - by little bit different work with
plan cache reference counting. Current design expects only statements
wrapped inside transactions. It is not designed for new possibilities in
CALL statements, when more transactions can be finished inside one
statement. Principally - cached plans should not be reused in different
transactions (simple expressions are an exception). So if we try to use
cached plans for CALL statements, there is no clean responsibility who has
to close a cached plan. It can be SPI (when execution stays in the same
transaction), or resource owner (when transaction is finished inside
execution of SPI).

The Peter wrote a comment about it

<--><--><-->/*
<--><--><--> * Don't save the plan if not in atomic context.  Otherwise,
<--><--><--> * transaction ends would cause errors about plancache leaks.
<--><--><--> *

This comment is not fully accurate. If we try to save the plan, then
execution (with finished transaction inside) ends by segfault. Cached plan
is released on transaction end (by resource owner) and related memory
context is released. But next time this structure is accessed. There is
only a warning about unclosed plan cache (it maybe depends on other things).

I wrote a patch 2 that marks CALL statement related plans as "fragile". In
this case the plan is cached every time. There is a special mark "fragile",
that blocks immediately releasing related memory context, and it blocks
warnings and errors because for this case we expect closing plan cache by
resource owner or by SPI statement.

It reduces well CPU and memory overhead - execution time (in one big
transaction is only 8sec) - memory overhead is +/- 0

Patch2 is not too clear, and too readable although I think so it is more
correct. It better fixes SPI behaviour against new state - possibility to
commit, rollback inside procedures (inside SPI call).

All regress tests passed.

Regards

Pavel

  23. Re: calling procedures is slow and consumes extra much memory against calling function

    Michael Paquier <michael@paquier.xyz> — 2020-09-17T05:36:53Z

    On Thu, Jul 16, 2020 at 09:08:09PM +0200, Pavel Stehule wrote:
> I am sending another patch that tries to allow CachedPlans for CALL
> statements. I think this patch is very accurate, but it is not nice,
> because it is smudging very precious reference counting for CachedPlans.

Amit, you are registered as a reviewer of this patch for two months
now.  Are you planning to look at it more?  If you are not planning to
do so, that's fine, but it may be better to remove your name as
reviewer then.
--
Michael

  24. Re: calling procedures is slow and consumes extra much memory against calling function

    Amit Khandekar <amitdkhan.pg@gmail.com> — 2020-09-17T09:05:44Z

    On Thu, 17 Sep 2020 at 11:07, Michael Paquier <michael@paquier.xyz> wrote:
>
> On Thu, Jul 16, 2020 at 09:08:09PM +0200, Pavel Stehule wrote:
> > I am sending another patch that tries to allow CachedPlans for CALL
> > statements. I think this patch is very accurate, but it is not nice,
> > because it is smudging very precious reference counting for CachedPlans.
>
> Amit, you are registered as a reviewer of this patch for two months
> now.  Are you planning to look at it more?  If you are not planning to
> do so, that's fine, but it may be better to remove your name as
> reviewer then.

Thanks Michael for reminding. I *had* actually planned to do some more
review. But I think I might end up not getting bandwidth for this one
during this month. So I have removed my name. But I have kept my name
as reviewer for bitmaps and correlation :
"https://commitfest.postgresql.org/29/2310/ since I do plan to do some
review on that one.

Thanks,
-Amit Khandekar
Huawei Technologies

  25. Re: calling procedures is slow and consumes extra much memory against calling function

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-09-28T01:04:06Z

    Pavel Stehule <pavel.stehule@gmail.com> writes:
> I am sending another patch that tries to allow CachedPlans for CALL
> statements. I think this patch is very accurate, but it is not nice,
> because it is smudging very precious reference counting for CachedPlans.

I spent some time testing this.  Although the #1 patch gets rid of
the major memory leak of cached plans, the original test case still
shows a pretty substantial leak across repeated executions of a CALL.
The reason is that the stanza for rebuilding stmt->target also gets
executed each time through, and that leaks not only the relatively
small PLpgSQL_row datum but also a bunch of catalog lookup cruft
created on the way to building the datum.  Basically this code forgot
that plpgsql's outer execution layer can't assume that it's running
in a short-lived context.

I attach a revised #1 that takes care of that problem, and also
cleans up what seems to me to be pretty sloppy thinking in both
the original code and Pavel's #1 patch: we should be restoring
the previous value of expr->plan, not cavalierly assuming that
it was necessarily NULL.  I didn't care for looking at the plan's
"saved" field to decide what was happening, either.  We really
should have a local flag variable clearly defining which behavior
it is that we're implementing.

With this patch, I see zero memory bloat on Pavel's original example,
even with a much larger repeat count.

I don't like much of anything about plpgsql-stmt_call-fix-2.patch.
It feels confused and poorly documented, possibly because "fragile"
is not a very clear term for whatever property it is you're trying to
attribute to plans.  But in any case, I think it's fixing the problem
in the wrong place.  I think the right way to fix it probably is to
manage a CALL's saved plan the same as every other plpgsql plan,
but arrange for the transient refcount on that plan to be held by a
ResourceOwner that is not a child of any transaction resowner, but
rather belongs to the procedure's execution and will be released on
the way out of the procedure.

In any case, I doubt we'd risk back-patching either the #2 patch
or any other approach to avoiding the repeat planning.  We need a
back-patchable fix that at least tamps down the memory bloat,
and this seems like it'd do.

			regards, tom lane

  26. Re: calling procedures is slow and consumes extra much memory against calling function

    Pavel Stehule <pavel.stehule@gmail.com> — 2020-09-28T09:14:39Z

    po 28. 9. 2020 v 3:04 odesílatel Tom Lane <tgl@sss.pgh.pa.us> napsal:

> Pavel Stehule <pavel.stehule@gmail.com> writes:
> > I am sending another patch that tries to allow CachedPlans for CALL
> > statements. I think this patch is very accurate, but it is not nice,
> > because it is smudging very precious reference counting for CachedPlans.
>
> I spent some time testing this.  Although the #1 patch gets rid of
> the major memory leak of cached plans, the original test case still
> shows a pretty substantial leak across repeated executions of a CALL.
> The reason is that the stanza for rebuilding stmt->target also gets
> executed each time through, and that leaks not only the relatively
> small PLpgSQL_row datum but also a bunch of catalog lookup cruft
> created on the way to building the datum.  Basically this code forgot
> that plpgsql's outer execution layer can't assume that it's running
> in a short-lived context.
>
> I attach a revised #1 that takes care of that problem, and also
> cleans up what seems to me to be pretty sloppy thinking in both
> the original code and Pavel's #1 patch: we should be restoring
> the previous value of expr->plan, not cavalierly assuming that
> it was necessarily NULL.  I didn't care for looking at the plan's
> "saved" field to decide what was happening, either.  We really
> should have a local flag variable clearly defining which behavior
> it is that we're implementing.
>
> With this patch, I see zero memory bloat on Pavel's original example,
> even with a much larger repeat count.
>
> I don't like much of anything about plpgsql-stmt_call-fix-2.patch.
> It feels confused and poorly documented, possibly because "fragile"
> is not a very clear term for whatever property it is you're trying to
> attribute to plans.  But in any case, I think it's fixing the problem
> in the wrong place.  I think the right way to fix it probably is to
> manage a CALL's saved plan the same as every other plpgsql plan,
> but arrange for the transient refcount on that plan to be held by a
> ResourceOwner that is not a child of any transaction resowner, but
> rather belongs to the procedure's execution and will be released on
> the way out of the procedure.
>
> In any case, I doubt we'd risk back-patching either the #2 patch
> or any other approach to avoiding the repeat planning.  We need a
> back-patchable fix that at least tamps down the memory bloat,
> and this seems like it'd do.
>

I agree with these conclusions.  I'll try to look if I can do #2 patch
better for pg14. Probably it can fix more issues related to CALL statement,
and I agree so this should not be backapatched.

It can be great to use CALL without memory leaks (and it can be better (in
future) if the performance of CALL statements should be good).

Thank you for enhancing and fixing this patch

Regards

Pavel


>                         regards, tom lane
>
>

  27. Re: calling procedures is slow and consumes extra much memory against calling function

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-09-29T15:20:35Z

    Pavel Stehule <pavel.stehule@gmail.com> writes:
> I agree with these conclusions.  I'll try to look if I can do #2 patch
> better for pg14. Probably it can fix more issues related to CALL statement,
> and I agree so this should not be backapatched.

I've pushed this and marked the CF entry committed.  Please start a
new thread and new CF entry whenever you have a more ambitious patch.

			regards, tom lane

  28. Re: calling procedures is slow and consumes extra much memory against calling function

    Pavel Stehule <pavel.stehule@gmail.com> — 2020-09-29T16:39:02Z

    út 29. 9. 2020 v 17:20 odesílatel Tom Lane <tgl@sss.pgh.pa.us> napsal:

> Pavel Stehule <pavel.stehule@gmail.com> writes:
> > I agree with these conclusions.  I'll try to look if I can do #2 patch
> > better for pg14. Probably it can fix more issues related to CALL
> statement,
> > and I agree so this should not be backapatched.
>
> I've pushed this and marked the CF entry committed.  Please start a
> new thread and new CF entry whenever you have a more ambitious patch.
>

Thank you

Pavel


>                         regards, tom lane
>