4 files changed, 123 insertions, 4 deletions

diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index 7b9e2f44757..71c09e8ace6 100644
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -4187,7 +4187,18 @@ exec_run_select(PLpgSQL_execstate *estate,
  *								a Datum by directly calling ExecEvalExpr().
  *
  * If successful, store results into *result, *isNull, *rettype and return
- * TRUE.  If the expression is not simple (any more), return FALSE.
+ * TRUE.  If the expression cannot be handled by simple evaluation,
+ * return FALSE.
+ *
+ * Because we only store one execution tree for a simple expression, we
+ * can't handle recursion cases.  So, if we see the tree is already busy
+ * with an evaluation in the current xact, we just return FALSE and let the
+ * caller run the expression the hard way.  (Other alternatives such as
+ * creating a new tree for a recursive call either introduce memory leaks,
+ * or add enough bookkeeping to be doubtful wins anyway.)  Another case that
+ * is covered by the expr_simple_in_use test is where a previous execution
+ * of the tree was aborted by an error: the tree may contain bogus state
+ * so we dare not re-use it.
  *
  * It is possible though unlikely for a simple expression to become non-simple
  * (consider for example redefining a trivial view).  We must handle that for
@@ -4223,6 +4234,13 @@ exec_eval_simple_expr(PLpgSQL_execstate *estate,
 		return false;
 
 	/*
+	 * If expression is in use in current xact, don't touch it.
+	 */
+	if (expr->expr_simple_in_use &&
+		expr->expr_simple_id == estate->eval_estate_simple_id)
+		return false;
+
+	/*
 	 * Revalidate cached plan, so that we will notice if it became stale. (We
 	 * also need to hold a refcount while using the plan.)	Note that even if
 	 * replanning occurs, the length of plancache_list can't change, since it
@@ -4257,6 +4275,7 @@ exec_eval_simple_expr(PLpgSQL_execstate *estate,
 	{
 		expr->expr_simple_state = ExecPrepareExpr(expr->expr_simple_expr,
 												  estate->eval_estate);
+		expr->expr_simple_in_use = false;
 		expr->expr_simple_id = estate->eval_estate_simple_id;
 	}
 
@@ -4317,17 +4336,26 @@ exec_eval_simple_expr(PLpgSQL_execstate *estate,
 		}
 
 		/*
+		 * Mark expression as busy for the duration of the ExecEvalExpr call.
+		 */
+		expr->expr_simple_in_use = true;
+
+		/*
 		 * Finally we can call the executor to evaluate the expression
 		 */
 		*result = ExecEvalExpr(expr->expr_simple_state,
 							   econtext,
 							   isNull,
 							   NULL);
+
+		/* Assorted cleanup */
+		expr->expr_simple_in_use = false;
 		MemoryContextSwitchTo(oldcontext);
 	}
 	PG_CATCH();
 	{
 		/* Restore global vars and propagate error */
+		/* note we intentionally don't reset expr_simple_in_use here */
 		ActiveSnapshot = saveActiveSnapshot;
 		PG_RE_THROW();
 	}
@@ -4967,6 +4995,7 @@ exec_simple_check_plan(PLpgSQL_expr *expr)
 	 */
 	expr->expr_simple_expr = tle->expr;
 	expr->expr_simple_state = NULL;
+	expr->expr_simple_in_use = false;
 	expr->expr_simple_id = -1;
 	/* Also stash away the expression result type */
 	expr->expr_simple_type = exprType((Node *) tle->expr);
diff --git a/src/pl/plpgsql/src/plpgsql.h b/src/pl/plpgsql/src/plpgsql.h
index b954f88f247..94fc39f36c5 100644
--- a/src/pl/plpgsql/src/plpgsql.h
+++ b/src/pl/plpgsql/src/plpgsql.h
@@ -183,10 +183,11 @@ typedef struct PLpgSQL_expr
 
 	/*
 	 * if expr is simple AND prepared in current eval_estate,
-	 * expr_simple_state is valid.	Test validity by seeing if expr_simple_id
-	 * matches eval_estate_simple_id.
+	 * expr_simple_state and expr_simple_in_use are valid. Test validity by
+	 * seeing if expr_simple_id matches eval_estate_simple_id.
 	 */
-	ExprState  *expr_simple_state;
+	ExprState  *expr_simple_state;		/* eval tree for expr_simple_expr */
+	bool		expr_simple_in_use;		/* true if eval tree is active */
 	long int	expr_simple_id;
 
 	/* params to pass to expr */
diff --git a/src/test/regress/expected/plpgsql.out b/src/test/regress/expected/plpgsql.out
index 549eb0f8852..a6ba6ab1b06 100644
--- a/src/test/regress/expected/plpgsql.out
+++ b/src/test/regress/expected/plpgsql.out
@@ -3171,3 +3171,50 @@ SELECT nonsimple_expr_test();
 (1 row)
 
 DROP FUNCTION nonsimple_expr_test();
+--
+-- Test cases involving recursion and error recovery in simple expressions
+-- (bugs in all versions before October 2010).  The problems are most
+-- easily exposed by mutual recursion between plpgsql and sql functions.
+--
+create function recurse(float8) returns float8 as
+$$
+begin
+  if ($1 < 10) then
+    return sql_recurse($1 + 1);
+  else
+    return $1;
+  end if;
+end;
+$$ language plpgsql;
+-- "limit" is to prevent this from being inlined
+create function sql_recurse(float8) returns float8 as
+$$ select recurse($1) limit 1; $$ language sql;
+select recurse(0);
+ recurse 
+---------
+      10
+(1 row)
+
+create function error1(text) returns text language sql as
+$$ SELECT relname::text FROM pg_class c WHERE c.oid = $1::regclass $$;
+create function error2(p_name_table text) returns text language plpgsql as $$
+begin
+  return error1(p_name_table);
+end$$;
+BEGIN;
+create table public.stuffs (stuff text);
+SAVEPOINT a;
+select error2('nonexistent.stuffs');
+ERROR:  schema "nonexistent" does not exist
+CONTEXT:  SQL function "error1" statement 1
+PL/pgSQL function "error2" line 2 at RETURN
+ROLLBACK TO a;
+select error2('public.stuffs');
+ error2 
+--------
+ stuffs
+(1 row)
+
+rollback;
+drop function error2(p_name_table text);
+drop function error1(text);
diff --git a/src/test/regress/sql/plpgsql.sql b/src/test/regress/sql/plpgsql.sql
index 6fcf7fa1c0c..00040ed55d5 100644
--- a/src/test/regress/sql/plpgsql.sql
+++ b/src/test/regress/sql/plpgsql.sql
@@ -2621,3 +2621,45 @@ $$ LANGUAGE plpgsql;
 SELECT nonsimple_expr_test();
 
 DROP FUNCTION nonsimple_expr_test();
+
+--
+-- Test cases involving recursion and error recovery in simple expressions
+-- (bugs in all versions before October 2010).  The problems are most
+-- easily exposed by mutual recursion between plpgsql and sql functions.
+--
+
+create function recurse(float8) returns float8 as
+$$
+begin
+  if ($1 < 10) then
+    return sql_recurse($1 + 1);
+  else
+    return $1;
+  end if;
+end;
+$$ language plpgsql;
+
+-- "limit" is to prevent this from being inlined
+create function sql_recurse(float8) returns float8 as
+$$ select recurse($1) limit 1; $$ language sql;
+
+select recurse(0);
+
+create function error1(text) returns text language sql as
+$$ SELECT relname::text FROM pg_class c WHERE c.oid = $1::regclass $$;
+
+create function error2(p_name_table text) returns text language plpgsql as $$
+begin
+  return error1(p_name_table);
+end$$;
+
+BEGIN;
+create table public.stuffs (stuff text);
+SAVEPOINT a;
+select error2('nonexistent.stuffs');
+ROLLBACK TO a;
+select error2('public.stuffs');
+rollback;
+
+drop function error2(p_name_table text);
+drop function error1(text);