From: Tom Lane Date: Mon, 9 Aug 2010 18:51:12 +0000 (+0000) Subject: Fix incorrect logic in plpgsql for cleanup after evaluation of non-simple X-Git-Tag: REL7_4_30~20 X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=ef2b138b89d9ce83c1153e22ec6b07e2a76728b1;p=postgresql 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 c58fba0b56..0e71a7cf95 100644 --- a/src/pl/plpgsql/src/pl_exec.c +++ b/src/pl/plpgsql/src/pl_exec.c @@ -3,7 +3,7 @@ * procedural language * * IDENTIFICATION - * $Header: /cvsroot/pgsql/src/pl/plpgsql/src/pl_exec.c,v 1.93.2.4 2010/07/05 09:27:57 heikki Exp $ + * $Header: /cvsroot/pgsql/src/pl/plpgsql/src/pl_exec.c,v 1.93.2.5 2010/08/09 18:51:12 tgl Exp $ * * This software is copyrighted by Jan Wieck - Hamburg. * @@ -1919,6 +1919,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 @@ -2674,6 +2677,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 @@ -2708,6 +2715,7 @@ exec_assign_value(PLpgSQL_execstate * estate, Datum oldarrayval, coerced_value; ArrayType *newarrayval; + SPITupleTable *save_eval_tuptable; HeapTuple newtup; switch (target->dtype) @@ -2868,6 +2876,16 @@ exec_assign_value(PLpgSQL_execstate * estate, /* * Target is an element of an array * + * 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; + + /* * 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 @@ -2910,8 +2928,23 @@ exec_assign_value(PLpgSQL_execstate * estate, subscripts[nsubscripts - 1 - i], &subisnull); havenullsubscript |= subisnull; + + /* + * 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; + /* * Skip the assignment if we have any nulls, either in the * original array value, the subscripts, or the righthand diff --git a/src/test/regress/expected/plpgsql.out b/src/test/regress/expected/plpgsql.out index a7a380b5c6..16b2ae8305 100644 --- a/src/test/regress/expected/plpgsql.out +++ b/src/test/regress/expected/plpgsql.out @@ -1788,3 +1788,26 @@ SELECT * FROM perform_test; (3 rows) drop table perform_test; +-- 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(); diff --git a/src/test/regress/sql/plpgsql.sql b/src/test/regress/sql/plpgsql.sql index b4d0186458..d603efed93 100644 --- a/src/test/regress/sql/plpgsql.sql +++ b/src/test/regress/sql/plpgsql.sql @@ -1603,4 +1603,26 @@ END;' language 'plpgsql'; SELECT perform_test_func(); SELECT * FROM perform_test; -drop table perform_test; \ No newline at end of file +drop table perform_test; + +-- 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();