Thread: Assertion failure with assignment to array elem
postgres=# CREATE OR REPLACE FUNCTION f_test() RETURNS VOID AS $$
DECLARE
arr text[];
lr text;
i integer;
BEGIN
i := 1;
SELECT 'foo' INTO lr;
arr[COALESCE(i, (SELECT NULL::integer))] := COALESCE(lr, (SELECT
NULL::text));
END;
$$ LANGUAGE PLPGSQL;
CREATE FUNCTION
postgres=# SELECT f_test();
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
TRAP: FailedAssertion("!(estate->eval_tuptable == ((void *)0))", File:
"pl_exec.c", Line: 4264)
This happens when both the array subscript and the expression been
assigned are "non-simple". The purpose of the funny-looking COALESCE
expressions in the above example is to force them to be non-simple.
This bug applies to all supported versions.
The problem is that exec_assign_value() is passed a value that came from
the current 'estate', but when exec_assign_value() evaluates the array
subscript, we don't expect there to already be an open result set in
'estate'.
A simple fix would be to make a copy of the value being assigned in
exec_assign_expr and calling exec_eval_cleanup() before the call to
exec_assign_value(). But I wonder if the performance impact would be too
high - one extra copy isn't that expensive, but it would affect every
single assignment of pass-by-reference variables.
--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com
Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:
> TRAP: FailedAssertion("!(estate->eval_tuptable == ((void *)0))", File:
> "pl_exec.c", Line: 4264)
> This happens when both the array subscript and the expression been
> assigned are "non-simple". The purpose of the funny-looking COALESCE
> expressions in the above example is to force them to be non-simple.
Ugh. I'm amazed we didn't find this long ago.
> The problem is that exec_assign_value() is passed a value that came from
> the current 'estate', but when exec_assign_value() evaluates the array
> subscript, we don't expect there to already be an open result set in
> 'estate'.
> A simple fix would be to make a copy of the value being assigned in
> exec_assign_expr and calling exec_eval_cleanup() before the call to
> exec_assign_value(). But I wonder if the performance impact would be too
> high - one extra copy isn't that expensive, but it would affect every
> single assignment of pass-by-reference variables.
Yeah, I don't like that either. What we need to do instead is fix
exec_assign_value so that it can cope with the case of the caller having
an open expression evaluation. We can easily have it save/restore
eval_tuptable. Not resetting eval_econtext is a bit harder, but maybe
we could have a use-count variable ... or even easier, just decree that
the caller has to do exec_eval_cleanup after calling exec_assign_value,
whether or not it had an open expression eval.
regards, tom lane
I wrote:
> Yeah, I don't like that either. What we need to do instead is fix
> exec_assign_value so that it can cope with the case of the caller having
> an open expression evaluation. We can easily have it save/restore
> eval_tuptable. Not resetting eval_econtext is a bit harder, but maybe
> we could have a use-count variable ... or even easier, just decree that
> the caller has to do exec_eval_cleanup after calling exec_assign_value,
> whether or not it had an open expression eval.
After a bit of experimentation, I propose the attached patch. I don't
think any additional resets of eval_econtext are necessary. There are
some callers of exec_assign_value that don't immediately do an
exec_eval_cleanup afterwards, but one will happen soon enough.
regards, tom lane
Index: pl_exec.c
===================================================================
RCS file: /cvsroot/pgsql/src/pl/plpgsql/src/pl_exec.c,v
retrieving revision 1.262
diff -c -r1.262 pl_exec.c
*** pl_exec.c 9 Aug 2010 02:25:05 -0000 1.262
--- pl_exec.c 9 Aug 2010 16:27:56 -0000
***************
*** 2701,2706 ****
--- 2701,2709 ----
*
* 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
***************
*** 3464,3469 ****
--- 3467,3476 ----
/* ----------
* 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
***************
*** 3714,3719 ****
--- 3721,3729 ----
case PLPGSQL_DTYPE_ARRAYELEM:
{
+ /*
+ * Target is an element of an array
+ */
int nsubscripts;
int i;
PLpgSQL_expr *subscripts[MAXDIM];
***************
*** 3729,3738 ****
coerced_value;
ArrayType *oldarrayval;
ArrayType *newarrayval;
/*
- * 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
--- 3739,3757 ----
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;
/*
* 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
***************
*** 3786,3793 ****
--- 3805,3827 ----
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,
On 09/08/10 19:32, Tom Lane wrote:
> I wrote:
>> Yeah, I don't like that either. What we need to do instead is fix
>> exec_assign_value so that it can cope with the case of the caller having
>> an open expression evaluation. We can easily have it save/restore
>> eval_tuptable. Not resetting eval_econtext is a bit harder, but maybe
>> we could have a use-count variable ... or even easier, just decree that
>> the caller has to do exec_eval_cleanup after calling exec_assign_value,
>> whether or not it had an open expression eval.
>
> After a bit of experimentation, I propose the attached patch. I don't
> think any additional resets of eval_econtext are necessary. There are
> some callers of exec_assign_value that don't immediately do an
> exec_eval_cleanup afterwards, but one will happen soon enough.
Ok, I was just trying to figure out that part too, but that works for me.
There's a related issue:
CREATE OR REPLACE FUNCTION dummy() RETURNS integer AS $$ SELECT
NULL::integer; $$ LANGUAGE sql;
postgres=# do $$
declare
i integer NOT NULL := 1;
begin
i := COALESCE(dummy(), (SELECT NULL::integer));
exception
WHEN OTHERS THEN BEGIN
i := COALESCE(dummy(), (SELECT 3::integer));
raise notice 'bar';
END;
end;
$$;
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
The assertion failure is the same. In this case, when we ereport() at
the first assignment (because it tries to assign NULL to a variable that
is declared as NOT NULL), estate->eval_tuptable is not cleaned up. The
2nd assignment traps the same assertion.
This one is simple to fix, we can always call exec_eval_cleanup() before
running the exception handler:
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -1092,6 +1092,8 @@ exec_stmt_block(PLpgSQL_execstate *estate,
PLpgSQL_stmt_block *block)
/* Revert to outer eval_econtext */
estate->eval_econtext = old_eval_econtext;
+ exec_eval_cleanup(estate);
+
/*
* If AtEOSubXact_SPI() popped any SPI context
of the subxact, it
* will have left us in a disconnected state.
We need this hack
--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com
Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:
> There's a related issue:
> ...
> This one is simple to fix, we can always call exec_eval_cleanup() before
> running the exception handler:
Sounds good, will add that and some regression tests and commit.
regards, tom lane