Fix incorrect logic in plpgsql for cleanup after evaluation of non-simple

expressions.  We need to deal with this when handling subscripts in an array
assignment, and also when catching an exception.  In an Assert-enabled build
these omissions led to Assert failures, but I think in a normal build the
only consequence would be short-term memory leakage; which may explain why
this wasn't reported from the field long ago.

Back-patch to all supported versions.  7.4 doesn't have exceptions, but
otherwise these bugs go all the way back.

Heikki Linnakangas and Tom Lane

diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index 648c5e6458160a591bc5425a35088ee5bff0a4ce..72083fda9a6970a64c518a7f71a060386ed6f2ec 100644 (file)
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *       $PostgreSQL: pgsql/src/pl/plpgsql/src/pl_exec.c,v 1.202.2.8 2010/07/05 09:27:31 heikki Exp $
+ *       $PostgreSQL: pgsql/src/pl/plpgsql/src/pl_exec.c,v 1.202.2.9 2010/08/09 18:50:37 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -1053,6 +1053,9 @@ exec_stmt_block(PLpgSQL_execstate *estate, PLpgSQL_stmt_block *block)
                         */
                        SPI_restore_connection();
 
+                       /* Must clean up the econtext too */
+                       exec_eval_cleanup(estate);
+
                        /* Look for a matching exception handler */
                        foreach(e, block->exceptions->exc_list)
                        {
@@ -2392,6 +2395,9 @@ plpgsql_estate_setup(PLpgSQL_execstate *estate,
  *
  * NB: the result of the evaluation is no longer valid after this is done,
  * unless it is a pass-by-value datatype.
+ *
+ * NB: if you change this code, see also the hacks in exec_assign_value's
+ * PLPGSQL_DTYPE_ARRAYELEM case.
  * ----------
  */
 static void
@@ -3390,6 +3396,10 @@ exec_assign_expr(PLpgSQL_execstate *estate, PLpgSQL_datum *target,
 
 /* ----------
  * exec_assign_value                   Put a value into a target field
+ *
+ * Note: in some code paths, this may leak memory in the eval_econtext;
+ * we assume that will be cleaned up later by exec_eval_cleanup.  We cannot
+ * call exec_eval_cleanup here for fear of destroying the input Datum value.
  * ----------
  */
 static void
@@ -3648,6 +3658,9 @@ exec_assign_value(PLpgSQL_execstate *estate,
 
                case PLPGSQL_DTYPE_ARRAYELEM:
                        {
+                               /*
+                                * Target is an element of an array
+                                */
                                int                     nsubscripts;
                                int                     i;
                                PLpgSQL_expr *subscripts[MAXDIM];
@@ -3663,10 +3676,19 @@ exec_assign_value(PLpgSQL_execstate *estate,
                                                        coerced_value;
                                ArrayType  *oldarrayval;
                                ArrayType  *newarrayval;
+                               SPITupleTable *save_eval_tuptable;
+
+                               /*
+                                * We need to do subscript evaluation, which might require
+                                * evaluating general expressions; and the caller might have
+                                * done that too in order to prepare the input Datum.  We
+                                * have to save and restore the caller's SPI_execute result,
+                                * if any.
+                                */
+                               save_eval_tuptable = estate->eval_tuptable;
+                               estate->eval_tuptable = NULL;
 
                                /*
-                                * Target is an element of an array
-                                *
                                 * To handle constructs like x[1][2] := something, we have to
                                 * be prepared to deal with a chain of arrayelem datums. Chase
                                 * back to find the base array datum, and save the subscript
@@ -3720,8 +3742,23 @@ exec_assign_value(PLpgSQL_execstate *estate,
                                                ereport(ERROR,
                                                                (errcode(ERRCODE_NULL_VALUE_NOT_ALLOWED),
                                                                 errmsg("array subscript in assignment must not be NULL")));
+
+                                       /*
+                                        * Clean up in case the subscript expression wasn't simple.
+                                        * We can't do exec_eval_cleanup, but we can do this much
+                                        * (which is safe because the integer subscript value is
+                                        * surely pass-by-value), and we must do it in case the
+                                        * next subscript expression isn't simple either.
+                                        */
+                                       if (estate->eval_tuptable != NULL)
+                                               SPI_freetuptable(estate->eval_tuptable);
+                                       estate->eval_tuptable = NULL;
                                }
 
+                               /* Now we can restore caller's SPI_execute result if any. */
+                               Assert(estate->eval_tuptable == NULL);
+                               estate->eval_tuptable = save_eval_tuptable;
+
                                /* Coerce source value to match array element type. */
                                coerced_value = exec_simple_cast_value(value,
                                                                                                           valtype,

diff --git a/src/test/regress/expected/plpgsql.out b/src/test/regress/expected/plpgsql.out
index 78466426f1c00b6ea8c72ded1e896da231a80af8..549eb0f885226384e3eeb3403e0c6a3b97af4898 100644 (file)
--- a/src/test/regress/expected/plpgsql.out
+++ b/src/test/regress/expected/plpgsql.out
@@ -3128,3 +3128,46 @@ select * from ret_query2(8);
  c9f0f895fb98ab9159f51fd0297e236d |  8 | t
 (9 rows)
 
+-- Test for appropriate cleanup of non-simple expression evaluations
+-- (bug in all versions prior to August 2010)
+CREATE FUNCTION nonsimple_expr_test() RETURNS text[] AS $$
+DECLARE
+  arr text[];
+  lr text;
+  i integer;
+BEGIN
+  arr := array[array['foo','bar'], array['baz', 'quux']];
+  lr := 'fool';
+  i := 1;
+  -- use sub-SELECTs to make expressions non-simple
+  arr[(SELECT i)][(SELECT i+1)] := (SELECT lr);
+  RETURN arr;
+END;
+$$ LANGUAGE plpgsql;
+SELECT nonsimple_expr_test();
+   nonsimple_expr_test   
+-------------------------
+ {{foo,fool},{baz,quux}}
+(1 row)
+
+DROP FUNCTION nonsimple_expr_test();
+CREATE FUNCTION nonsimple_expr_test() RETURNS integer AS $$
+declare
+   i integer NOT NULL := 0;
+begin
+  begin
+    i := (SELECT NULL::integer);  -- should throw error
+  exception
+    WHEN OTHERS THEN
+      i := (SELECT 1::integer);
+  end;
+  return i;
+end;
+$$ LANGUAGE plpgsql;
+SELECT nonsimple_expr_test();
+ nonsimple_expr_test 
+---------------------
+                   1
+(1 row)
+
+DROP FUNCTION nonsimple_expr_test();

diff --git a/src/test/regress/sql/plpgsql.sql b/src/test/regress/sql/plpgsql.sql
index 3c7459b2b7c8234c4b61bda0a0a2185a2105d37e..6fcf7fa1c0c13afea767ffdf4d7cdad0888b248c 100644 (file)
--- a/src/test/regress/sql/plpgsql.sql
+++ b/src/test/regress/sql/plpgsql.sql
@@ -2580,4 +2580,44 @@ begin
 end;
 $$ language plpgsql;
 
-select * from ret_query2(8);
\ No newline at end of file
+select * from ret_query2(8);
+
+-- Test for appropriate cleanup of non-simple expression evaluations
+-- (bug in all versions prior to August 2010)
+
+CREATE FUNCTION nonsimple_expr_test() RETURNS text[] AS $$
+DECLARE
+  arr text[];
+  lr text;
+  i integer;
+BEGIN
+  arr := array[array['foo','bar'], array['baz', 'quux']];
+  lr := 'fool';
+  i := 1;
+  -- use sub-SELECTs to make expressions non-simple
+  arr[(SELECT i)][(SELECT i+1)] := (SELECT lr);
+  RETURN arr;
+END;
+$$ LANGUAGE plpgsql;
+
+SELECT nonsimple_expr_test();
+
+DROP FUNCTION nonsimple_expr_test();
+
+CREATE FUNCTION nonsimple_expr_test() RETURNS integer AS $$
+declare
+   i integer NOT NULL := 0;
+begin
+  begin
+    i := (SELECT NULL::integer);  -- should throw error
+  exception
+    WHEN OTHERS THEN
+      i := (SELECT 1::integer);
+  end;
+  return i;
+end;
+$$ LANGUAGE plpgsql;
+
+SELECT nonsimple_expr_test();
+
+DROP FUNCTION nonsimple_expr_test();