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

[Pavel Stehule]

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

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",