so 11. 7. 2020 v 7:38 odesílatel Pavel Stehule <pavel.steh...@gmail.com>
napsal:
>
>
> čt 9. 7. 2020 v 8:28 odesílatel Amit Khandekar <amitdkhan...@gmail.com>
> napsal:
>
>> On Wed, 17 Jun 2020 at 13:54, Pavel Stehule <pavel.steh...@gmail.com>
>> wrote:
>> >
>> >
>> >
>> > st 17. 6. 2020 v 7:52 odesílatel Amit Khandekar <amitdkhan...@gmail.com>
>> napsal:
>> >>
>> >> On Wed, 10 Jun 2020 at 17:12, Pavel Stehule <pavel.steh...@gmail.com>
>> wrote:
>> >> > st 10. 6. 2020 v 12:26 odesílatel Amit Khandekar <
>> amitdkhan...@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
>
