čt 9. 7. 2020 v 8:28 odesílatel Amit Khandekar <[email protected]>
napsal:
> On Wed, 17 Jun 2020 at 13:54, Pavel Stehule <[email protected]>
> wrote:
> >
> >
> >
> > st 17. 6. 2020 v 7:52 odesílatel Amit Khandekar <[email protected]>
> napsal:
> >>
> >> On Wed, 10 Jun 2020 at 17:12, Pavel Stehule <[email protected]>
> wrote:
> >> > st 10. 6. 2020 v 12:26 odesílatel Amit Khandekar <
> [email protected]> 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
diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index d4a3d58daa..22c0376795 100644
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -2150,15 +2150,16 @@ static int
exec_stmt_call(PLpgSQL_execstate *estate, PLpgSQL_stmt_call *stmt)
{
PLpgSQL_expr *expr = stmt->expr;
+ volatile SPIPlanPtr plan = expr->plan;
volatile LocalTransactionId before_lxid;
LocalTransactionId after_lxid;
volatile bool pushed_active_snap = false;
volatile int rc;
+ volatile bool plan_owner = false;
/* PG_TRY to ensure we clear the plan link, if needed, on failure */
PG_TRY();
{
- SPIPlanPtr plan = expr->plan;
ParamListInfo paramLI;
if (plan == NULL)
@@ -2173,6 +2174,16 @@ exec_stmt_call(PLpgSQL_execstate *estate, PLpgSQL_stmt_call *stmt)
*/
exec_prepare_plan(estate, expr, 0, estate->atomic);
+ /*
+ * Not saved plan should be explicitly released. Without it, any
+ * not recursive usage of CALL statemenst leak plan in SPI memory.
+ * The created plan can be reused when procedure is called recursively,
+ * and releasing plan can be done only in recursion root call, when
+ * expression has not assigned plan. Where a plan was created, then
+ * there plan can be released.
+ */
+ plan_owner = true;
+
/*
* The procedure call could end transactions, which would upset
* the snapshot management in SPI_execute*, so don't let it do it.
@@ -2319,14 +2330,30 @@ exec_stmt_call(PLpgSQL_execstate *estate, PLpgSQL_stmt_call *stmt)
* If we aren't saving the plan, unset the pointer. Note that it
* could have been unset already, in case of a recursive call.
*/
- if (expr->plan && !expr->plan->saved)
+ if (plan && !plan->saved)
+ {
+ if (plan_owner)
+ SPI_freeplan(plan);
+
expr->plan = NULL;
+ }
PG_RE_THROW();
}
PG_END_TRY();
- if (expr->plan && !expr->plan->saved)
+ 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;
+ }
if (rc < 0)
elog(ERROR, "SPI_execute_plan_with_paramlist failed executing query \"%s\": %s",
