Fix memory leak in plpgsql's CALL processing.

tglsfdc · tglsfdc · commit 13a1901bad47 · 2020-09-29T11:18:31.000-04:00
When executing a CALL or DO in a non-atomic context (i.e., not inside a function or query), plpgsql creates a new plan each time through, as a rather hacky solution to some resource management issues. But it failed to free this plan until exit of the current procedure or DO block, resulting in serious memory bloat in procedures that called other procedures many times. Fix by remembering to free the plan, and by being more honest about restoring the previous state (otherwise, recursive procedure calls have a problem). There was also a smaller leak associated with recalculation of the "target" list of output variables. Fix that by using the statement- lifespan context to hold non-permanent values. Back-patch to v11 where procedures were introduced. Pavel Stehule and Tom Lane Discussion: https://postgr.es/m/CAFj8pRDiiU1dqym+_P4_GuTWm76knJu7z9opWayBJTC0nQGUUA@mail.gmail.com
diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
@@ -2102,55 +2102,82 @@ exec_stmt_perform(PLpgSQL_execstate *estate, PLpgSQL_stmt_perform *stmt)
 
 /*
  * exec_stmt_call
+ *
+ * NOTE: this is used for both CALL and DO statements.
  */
 static int
 exec_stmt_call(PLpgSQL_execstate *estate, PLpgSQL_stmt_call *stmt)
 {
 	PLpgSQL_expr *expr = stmt->expr;
+	SPIPlanPtr	orig_plan = expr->plan;
+	bool		local_plan;
+	PLpgSQL_variable *volatile cur_target = stmt->target;
 	volatile LocalTransactionId before_lxid;
 	LocalTransactionId after_lxid;
 	volatile bool pushed_active_snap = false;
 	volatile int rc;
 
+	/*
+	 * If not in atomic context, we make a local plan that we'll just use for
+	 * this invocation, and will free at the end.  Otherwise, transaction ends
+	 * would cause errors about plancache leaks.
+	 *
+	 * XXX This would be fixable with some plancache/resowner surgery
+	 * elsewhere, but for now we'll just work around this here.
+	 */
+	local_plan = !estate->atomic;
+
 	/* PG_TRY to ensure we clear the plan link, if needed, on failure */
 	PG_TRY();
 	{
 		SPIPlanPtr	plan = expr->plan;
 		ParamListInfo paramLI;
 
-		if (plan == NULL)
+		/*
+		 * Make a plan if we don't have one, or if we need a local one.  Note
+		 * that we'll overwrite expr->plan either way; the PG_TRY block will
+		 * ensure we undo that on the way out, if the plan is local.
+		 */
+		if (plan == NULL || local_plan)
 		{
+			/* Don't let SPI save the plan if it's going to be local */
+			exec_prepare_plan(estate, expr, 0, !local_plan);
+			plan = expr->plan;
 
 			/*
-			 * Don't save the plan if not in atomic context.  Otherwise,
-			 * transaction ends would cause errors about plancache leaks.
-			 *
-			 * XXX This would be fixable with some plancache/resowner surgery
-			 * elsewhere, but for now we'll just work around this here.
+			 * A CALL or DO can never be a simple expression.  (If it could
+			 * be, we'd have to worry about saving/restoring the previous
+			 * values of the related expr fields, not just expr->plan.)
 			 */
-			exec_prepare_plan(estate, expr, 0, estate->atomic);
+			Assert(!expr->expr_simple_expr);
 
 			/*
 			 * The procedure call could end transactions, which would upset
 			 * the snapshot management in SPI_execute*, so don't let it do it.
 			 * Instead, we set the snapshots ourselves below.
 			 */
-			plan = expr->plan;
 			plan->no_snapshots = true;
 
 			/*
 			 * Force target to be recalculated whenever the plan changes, in
 			 * case the procedure's argument list has changed.
 			 */
 			stmt->target = NULL;
+			cur_target = NULL;
 		}
 
 		/*
 		 * We construct a DTYPE_ROW datum representing the plpgsql variables
 		 * associated with the procedure's output arguments.  Then we can use
 		 * exec_move_row() to do the assignments.
+		 *
+		 * If we're using a local plan, also make a local target; otherwise,
+		 * since the above code will force a new plan each time through, we'd
+		 * repeatedly leak the memory for the target.  (Note: we also leak the
+		 * target when a plan change is forced, but that isn't so likely to
+		 * cause excessive memory leaks.)
 		 */
-		if (stmt->is_call && stmt->target == NULL)
+		if (stmt->is_call && cur_target == NULL)
 		{
 			Node	   *node;
 			FuncExpr   *funcexpr;
@@ -2165,6 +2192,9 @@ exec_stmt_call(PLpgSQL_execstate *estate, PLpgSQL_stmt_call *stmt)
 			int			i;
 			ListCell   *lc;
 
+			/* Use eval_mcontext for any cruft accumulated here */
+			oldcontext = MemoryContextSwitchTo(get_eval_mcontext(estate));
+
 			/*
 			 * Get the parsed CallStmt, and look up the called procedure
 			 */
@@ -2196,17 +2226,20 @@ exec_stmt_call(PLpgSQL_execstate *estate, PLpgSQL_stmt_call *stmt)
 			ReleaseSysCache(func_tuple);
 
 			/*
-			 * Begin constructing row Datum
+			 * Begin constructing row Datum; keep it in fn_cxt if it's to be
+			 * long-lived.
 			 */
-			oldcontext = MemoryContextSwitchTo(estate->func->fn_cxt);
+			if (!local_plan)
+				MemoryContextSwitchTo(estate->func->fn_cxt);
 
 			row = (PLpgSQL_row *) palloc0(sizeof(PLpgSQL_row));
 			row->dtype = PLPGSQL_DTYPE_ROW;
 			row->refname = "(unnamed row)";
 			row->lineno = -1;
 			row->varnos = (int *) palloc(sizeof(int) * list_length(funcargs));
 
-			MemoryContextSwitchTo(oldcontext);
+			if (!local_plan)
+				MemoryContextSwitchTo(get_eval_mcontext(estate));
 
 			/*
 			 * Examine procedure's argument list.  Each output arg position
@@ -2250,7 +2283,13 @@ exec_stmt_call(PLpgSQL_execstate *estate, PLpgSQL_stmt_call *stmt)
 
 			row->nfields = nfields;
 
-			stmt->target = (PLpgSQL_variable *) row;
+			cur_target = (PLpgSQL_variable *) row;
+
+			/* We can save and re-use the target datum, if it's not local */
+			if (!local_plan)
+				stmt->target = cur_target;
+
+			MemoryContextSwitchTo(oldcontext);
 		}
 
 		paramLI = setup_param_list(estate, expr);
@@ -2273,17 +2312,27 @@ exec_stmt_call(PLpgSQL_execstate *estate, PLpgSQL_stmt_call *stmt)
 	PG_CATCH();
 	{
 		/*
-		 * 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 we are using a local plan, restore the old plan link.
 		 */
-		if (expr->plan && !expr->plan->saved)
-			expr->plan = NULL;
+		if (local_plan)
+			expr->plan = orig_plan;
 		PG_RE_THROW();
 	}
 	PG_END_TRY();
 
-	if (expr->plan && !expr->plan->saved)
-		expr->plan = NULL;
+	/*
+	 * If we are using a local plan, restore the old plan link; then free the
+	 * local plan to avoid memory leaks.  (Note that the error exit path above
+	 * just clears the link without risking calling SPI_freeplan; we expect
+	 * that xact cleanup will take care of the mess in that case.)
+	 */
+	if (local_plan)
+	{
+		SPIPlanPtr	plan = expr->plan;
+
+		expr->plan = orig_plan;
+		SPI_freeplan(plan);
+	}
 
 	if (rc < 0)
 		elog(ERROR, "SPI_execute_plan_with_paramlist failed executing query \"%s\": %s",
@@ -2319,10 +2368,10 @@ exec_stmt_call(PLpgSQL_execstate *estate, PLpgSQL_stmt_call *stmt)
 	{
 		SPITupleTable *tuptab = SPI_tuptable;
 
-		if (!stmt->target)
+		if (!cur_target)
 			elog(ERROR, "DO statement returned a row");
 
-		exec_move_row(estate, stmt->target, tuptab->vals[0], tuptab->tupdesc);
+		exec_move_row(estate, cur_target, tuptab->vals[0], tuptab->tupdesc);
 	}
 	else if (SPI_processed > 1)
 		elog(ERROR, "procedure call returned more than one row");
