Re: Assertion failure with assignment to array elem - Mailing list pgsql-bugs

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,