Fix plpgsql's handling of "simple" expression evaluation.

In general, expression execution state trees aren't re-entrantly usable,
since functions can store private state information in them.
For efficiency reasons, plpgsql tries to cache and reuse state trees for
"simple" expressions.  It can get away with that most of the time, but it
can fail if the state tree is dirty from a previous failed execution (as
in an example from Alvaro) or is being used recursively (as noted by me).

Fix by tracking whether a state tree is in use, and falling back to the
"non-simple" code path if so.  This results in a pretty considerable speed
hit when the non-simple path is taken, but the available alternatives seem
even more unpleasant because they add overhead in the simple path.  Per
idea from Heikki.

Back-patch to all supported branches.

diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index 4267eac00b4efe7b793d835d527c23bd18345279..721c21e8567daf76a717cb5c08695854226e1068 100644 (file)
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -3731,7 +3731,14 @@ exec_eval_expr(PLpgSQL_execstate *estate,
         * directly
         */
        if (expr->expr_simple_expr != NULL)
-               return exec_eval_simple_expr(estate, expr, isNull, rettype);
+       {
+               /*
+                * If expression is in use in current xact, don't touch it.
+                */
+               if (!(expr->expr_simple_in_use &&
+                         expr->expr_simple_xid == GetTopTransactionId()))
+                       return exec_eval_simple_expr(estate, expr, isNull, rettype);
+       }
 
        rc = exec_run_select(estate, expr, 2, NULL);
        if (rc != SPI_OK_SELECT)
@@ -3883,6 +3890,7 @@ exec_eval_simple_expr(PLpgSQL_execstate *estate,
        {
                expr->expr_simple_state = ExecPrepareExpr(expr->expr_simple_expr,
                                                                                                  simple_eval_estate);
+               expr->expr_simple_in_use = false;
                expr->expr_simple_xid = curxid;
        }
 
@@ -3938,6 +3946,11 @@ exec_eval_simple_expr(PLpgSQL_execstate *estate,
                        ActiveSnapshot = CopySnapshot(GetTransactionSnapshot());
                }
 
+               /*
+                * 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
                 */
@@ -3945,11 +3958,15 @@ exec_eval_simple_expr(PLpgSQL_execstate *estate,
                                                          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();
        }
@@ -4545,6 +4562,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_xid = InvalidTransactionId;
        /* 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 5a47b9d629d094d31877c9a57f1b20535c68e7c3..cfd04b8d11876e952f77b4af47f629971b911076 100644 (file)
--- a/src/pl/plpgsql/src/plpgsql.h
+++ b/src/pl/plpgsql/src/plpgsql.h
@@ -201,10 +201,12 @@ typedef struct PLpgSQL_expr
        Expr       *expr_simple_expr;           /* NULL means not a simple expr */
        Oid                     expr_simple_type;
        /*
-        * if expr is simple AND in use in current xact, expr_simple_state is
-        * valid.  Test validity by seeing if expr_simple_xid matches current XID.
+        * if expr is simple AND in use in current xact, expr_simple_state and
+        * expr_simple_in_use are valid.  Test validity by seeing if
+        * expr_simple_xid matches current XID.
         */
-       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 */
        TransactionId expr_simple_xid;
        /* params to pass to expr */
        int                     nparams;

diff --git a/src/test/regress/expected/plpgsql.out b/src/test/regress/expected/plpgsql.out
index 00a6cbb4a1247ff2d01915d9f94a68ae15e00d3c..bfa47eb3ca936fab3886aa8c549a37d37f1a948f 100644 (file)
--- a/src/test/regress/expected/plpgsql.out
+++ b/src/test/regress/expected/plpgsql.out
@@ -2782,3 +2782,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 cf4c8e5b03f6f082f966b75c48ed8232b2a27cdf..11227ec5b73271a27b7892b761357e479c2a98b7 100644 (file)
--- a/src/test/regress/sql/plpgsql.sql
+++ b/src/test/regress/sql/plpgsql.sql
@@ -2335,3 +2335,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);