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
Pavel
> -Amit
>
diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index f41d675d65..88b8c7882a 100644
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -2132,15 +2132,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)
@@ -2155,6 +2156,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.
@@ -2301,14 +2312,23 @@ 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);
expr->plan = NULL;
+ }
if (rc < 0)
elog(ERROR, "SPI_execute_plan_with_paramlist failed executing query \"%s\": %s",
