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

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

> On Wed, 17 Jun 2020 at 13:54, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
> wrote:
> >
> >
> >
> > st 17. 6. 2020 v 7:52 odesílatel Amit Khandekar <amitdkhan(dot)pg(at)gmail(dot)com>
> napsal:
> >>
> >> On Wed, 10 Jun 2020 at 17:12, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
> wrote:
> >> > st 10. 6. 2020 v 12:26 odesílatel Amit Khandekar <
> amitdkhan(dot)pg(at)gmail(dot)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