Fix error-cleanup mistakes in exec_stmt_call().

tglsfdc · tglsfdc · commit f26c06a4046b · 2018-11-09T22:04:14.000-05:00
Commit 15c7293 was a couple bricks shy of a load: we need to ensure that expr->plan gets reset to NULL on any error exit, if it's not supposed to be saved. Also ensure that the stmt->target calculation gets redone if needed. The easy way to exhibit a problem is to set up code that violates the writable-argument restriction and then execute it twice. But error exits out of, eg, setup_param_list() could also break it. Make the existing PG_TRY block cover all of that code to be sure. Per report from Pavel Stehule. Discussion: https://postgr.es/m/CAFj8pRAeXNTO43W2Y0Cn0YOVFPv1WpYyOqQrrzUiN6s=dn7gCg@mail.gmail.com
diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
@@ -2072,39 +2072,50 @@ static int
 exec_stmt_call(PLpgSQL_execstate *estate, PLpgSQL_stmt_call *stmt)
 {
 	PLpgSQL_expr *expr = stmt->expr;
-	ParamListInfo paramLI;
-	LocalTransactionId before_lxid;
+	volatile LocalTransactionId before_lxid;
 	LocalTransactionId after_lxid;
-	bool		pushed_active_snap = false;
-	int			rc;
+	volatile bool pushed_active_snap = false;
+	volatile int rc;
 
-	if (expr->plan == NULL)
+	/* PG_TRY to ensure we clear the plan link, if needed, on failure */
+	PG_TRY();
 	{
-		SPIPlanPtr	plan;
+		SPIPlanPtr	plan = expr->plan;
+		ParamListInfo paramLI;
 
-		/*
-		 * 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.
-		 */
-		exec_prepare_plan(estate, expr, 0, estate->atomic);
+		if (plan == NULL)
+		{
 
-		/*
-		 * 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;
+			/*
+			 * 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.
+			 */
+			exec_prepare_plan(estate, expr, 0, estate->atomic);
+
+			/*
+			 * 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;
+		}
 
 		/*
 		 * 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.  (We do this each time the
-		 * plan changes, in case the procedure's argument list has changed.)
+		 * exec_move_row() to do the assignments.
 		 */
-		if (stmt->is_call)
+		if (stmt->is_call && stmt->target == NULL)
 		{
 			Node	   *node;
 			FuncExpr   *funcexpr;
@@ -2206,24 +2217,21 @@ exec_stmt_call(PLpgSQL_execstate *estate, PLpgSQL_stmt_call *stmt)
 
 			stmt->target = (PLpgSQL_variable *) row;
 		}
-	}
 
-	paramLI = setup_param_list(estate, expr);
+		paramLI = setup_param_list(estate, expr);
 
-	before_lxid = MyProc->lxid;
+		before_lxid = MyProc->lxid;
 
-	/*
-	 * Set snapshot only for non-read-only procedures, similar to SPI
-	 * behavior.
-	 */
-	if (!estate->readonly_func)
-	{
-		PushActiveSnapshot(GetTransactionSnapshot());
-		pushed_active_snap = true;
-	}
+		/*
+		 * Set snapshot only for non-read-only procedures, similar to SPI
+		 * behavior.
+		 */
+		if (!estate->readonly_func)
+		{
+			PushActiveSnapshot(GetTransactionSnapshot());
+			pushed_active_snap = true;
+		}
 
-	PG_TRY();
-	{
 		rc = SPI_execute_plan_with_paramlist(expr->plan, paramLI,
 											 estate->readonly_func, 0);
 	}
