Fix plpgsql's handling of "simple" expression evaluation.
authorTom Lane <[email protected]>
Thu, 28 Oct 2010 17:01:07 +0000 (13:01 -0400)
committerTom Lane <[email protected]>
Thu, 28 Oct 2010 17:02:53 +0000 (13:02 -0400)
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.

src/pl/plpgsql/src/pl_exec.c
src/pl/plpgsql/src/plpgsql.h
src/test/regress/expected/plpgsql.out
src/test/regress/sql/plpgsql.sql

index 8d0c62559b6344506948e3e726a07982a5983e84..87a9f9de0e2e3db47dfd5a04d55a11d1731225f3 100644 (file)
@@ -4383,7 +4383,18 @@ loop_exit:
  *                                                             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
@@ -4419,6 +4430,12 @@ exec_eval_simple_expr(PLpgSQL_execstate *estate,
        if (expr->expr_simple_expr == NULL)
                return false;
 
+       /*
+        * If expression is in use in current xact, don't touch it.
+        */
+       if (expr->expr_simple_in_use && expr->expr_simple_lxid == curlxid)
+               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
@@ -4454,6 +4471,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_lxid = curlxid;
        }
 
@@ -4508,6 +4526,11 @@ exec_eval_simple_expr(PLpgSQL_execstate *estate,
                PushActiveSnapshot(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
         */
@@ -4515,6 +4538,10 @@ exec_eval_simple_expr(PLpgSQL_execstate *estate,
                                                   econtext,
                                                   isNull,
                                                   NULL);
+
+       /* Assorted cleanup */
+       expr->expr_simple_in_use = false;
+
        MemoryContextSwitchTo(oldcontext);
 
        if (!estate->readonly_func)
@@ -5184,6 +5211,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_lxid = InvalidLocalTransactionId;
        /* Also stash away the expression result type */
        expr->expr_simple_type = exprType((Node *) tle->expr);
index c7d43a1f80725d40008a975ed48e42b56751ed8e..11ffc8d95261e57b8601e76392394a721d7726f6 100644 (file)
@@ -206,10 +206,12 @@ typedef struct PLpgSQL_expr
 
        /*
         * if expr is simple AND prepared in current transaction,
-        * expr_simple_state is valid. Test validity by seeing if expr_simple_lxid
-        * matches current LXID.
+        * expr_simple_state and expr_simple_in_use are valid. Test validity by
+        * seeing if expr_simple_lxid matches current LXID.  (If not,
+        * expr_simple_state probably points at garbage!)
         */
-       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 */
        LocalTransactionId expr_simple_lxid;
 
        /* params to pass to expr */
index 9a9e4d35a62062839486e7f617755275d13c0fe8..314281fe6edff358113781cdbc1a6601c4576a4a 100644 (file)
@@ -3780,6 +3780,53 @@ 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);
 -- Test handling of string literals.
 set standard_conforming_strings = off;
 create or replace function strtest() returns text as $$
index afd9c528ca2b525c56f196c2da875a4aa813a926..7a5ee7c0b5be580362e2e7d0a4c53527c83e0fdc 100644 (file)
@@ -3046,6 +3046,48 @@ 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);
+
 -- Test handling of string literals.
 
 set standard_conforming_strings = off;