Fix ExecEvalArrayRef to pass down the old value of the array element or slice

being assigned to, in case the expression to be assigned is a FieldStore that
would need to modify that value.  The need for this was foreseen some time
ago, but not implemented then because we did not have arrays of composites.
Now we do, but the point evidently got overlooked in that patch.  Net result
is that updating a field of an array element doesn't work right, as
illustrated if you try the new regression test on an unpatched backend.
Noted while experimenting with EXPLAIN VERBOSE, which has also got some issues
in this area.

Backpatch to 8.3, where arrays of composites were introduced.

diff --git a/src/backend/executor/execQual.c b/src/backend/executor/execQual.c
index f26a4437da3d8e82368b5ccb1a2d22e016fc87a1..6c287f92d0a99aff694a1f6dcd9ebba00cbbd976 100644 (file)
--- a/src/backend/executor/execQual.c
+++ b/src/backend/executor/execQual.c
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *       $PostgreSQL: pgsql/src/backend/executor/execQual.c,v 1.226.2.2 2009/12/29 17:41:18 heikki Exp $
+ *       $PostgreSQL: pgsql/src/backend/executor/execQual.c,v 1.226.2.3 2010/02/18 18:42:04 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -59,6 +59,7 @@
 static Datum ExecEvalArrayRef(ArrayRefExprState *astate,
                                 ExprContext *econtext,
                                 bool *isNull, ExprDoneCond *isDone);
+static bool isAssignmentIndirectionExpr(ExprState *exprstate);
 static Datum ExecEvalAggref(AggrefExprState *aggref,
                           ExprContext *econtext,
                           bool *isNull, ExprDoneCond *isDone);
@@ -333,22 +334,74 @@ ExecEvalArrayRef(ArrayRefExprState *astate,
        if (isAssignment)
        {
                Datum           sourceData;
+               Datum           save_datum;
+               bool            save_isNull;
 
                /*
-                * Evaluate the value to be assigned into the array.
+                * We might have a nested-assignment situation, in which the
+                * refassgnexpr is itself a FieldStore or ArrayRef that needs to
+                * obtain and modify the previous value of the array element or slice
+                * being replaced.  If so, we have to extract that value from the
+                * array and pass it down via the econtext's caseValue.  It's safe to
+                * reuse the CASE mechanism because there cannot be a CASE between
+                * here and where the value would be needed, and an array assignment
+                * can't be within a CASE either.  (So saving and restoring the
+                * caseValue is just paranoia, but let's do it anyway.)
                 *
-                * XXX At some point we'll need to look into making the old value of
-                * the array element available via CaseTestExpr, as is done by
-                * ExecEvalFieldStore.  This is not needed now but will be needed to
-                * support arrays of composite types; in an assignment to a field of
-                * an array member, the parser would generate a FieldStore that
-                * expects to fetch its input tuple via CaseTestExpr.
+                * Since fetching the old element might be a nontrivial expense, do it
+                * only if the argument appears to actually need it.
+                */
+               save_datum = econtext->caseValue_datum;
+               save_isNull = econtext->caseValue_isNull;
+
+               if (isAssignmentIndirectionExpr(astate->refassgnexpr))
+               {
+                       if (*isNull)
+                       {
+                               /* whole array is null, so any element or slice is too */
+                               econtext->caseValue_datum = (Datum) 0;
+                               econtext->caseValue_isNull = true;
+                       }
+                       else if (lIndex == NULL)
+                       {
+                               econtext->caseValue_datum = array_ref(array_source, i,
+                                                                                                         upper.indx,
+                                                                                                         astate->refattrlength,
+                                                                                                         astate->refelemlength,
+                                                                                                         astate->refelembyval,
+                                                                                                         astate->refelemalign,
+                                                                                                         &econtext->caseValue_isNull);
+                       }
+                       else
+                       {
+                               resultArray = array_get_slice(array_source, i,
+                                                                                         upper.indx, lower.indx,
+                                                                                         astate->refattrlength,
+                                                                                         astate->refelemlength,
+                                                                                         astate->refelembyval,
+                                                                                         astate->refelemalign);
+                               econtext->caseValue_datum = PointerGetDatum(resultArray);
+                               econtext->caseValue_isNull = false;
+                       }
+               }
+               else
+               {
+                       /* argument shouldn't need caseValue, but for safety set it null */
+                       econtext->caseValue_datum = (Datum) 0;
+                       econtext->caseValue_isNull = true;
+               }
+
+               /*
+                * Evaluate the value to be assigned into the array.
                 */
                sourceData = ExecEvalExpr(astate->refassgnexpr,
                                                                  econtext,
                                                                  &eisnull,
                                                                  NULL);
 
+               econtext->caseValue_datum = save_datum;
+               econtext->caseValue_isNull = save_isNull;
+
                /*
                 * For an assignment to a fixed-length array type, both the original
                 * array and the value to be assigned into it must be non-NULL, else
@@ -410,6 +463,34 @@ ExecEvalArrayRef(ArrayRefExprState *astate,
        }
 }
 
+/*
+ * Helper for ExecEvalArrayRef: is expr a nested FieldStore or ArrayRef
+ * that might need the old element value passed down?
+ *
+ * (We could use this in ExecEvalFieldStore too, but in that case passing
+ * the old value is so cheap there's no need.)
+ */
+static bool
+isAssignmentIndirectionExpr(ExprState *exprstate)
+{
+       if (exprstate == NULL)
+               return false;                   /* just paranoia */
+       if (IsA(exprstate, FieldStoreState))
+       {
+               FieldStore *fstore = (FieldStore *) exprstate->expr;
+
+               if (fstore->arg && IsA(fstore->arg, CaseTestExpr))
+                       return true;
+       }
+       else if (IsA(exprstate, ArrayRefExprState))
+       {
+               ArrayRef   *arrayRef = (ArrayRef *) exprstate->expr;
+
+               if (arrayRef->refexpr && IsA(arrayRef->refexpr, CaseTestExpr))
+                       return true;
+       }
+       return false;
+}
 
 /* ----------------------------------------------------------------
  *             ExecEvalAggref
@@ -3549,10 +3630,12 @@ ExecEvalFieldStore(FieldStoreState *fstate,
 
                /*
                 * Use the CaseTestExpr mechanism to pass down the old value of the
-                * field being replaced; this is useful in case we have a nested field
-                * update situation.  It's safe to reuse the CASE mechanism because
-                * there cannot be a CASE between here and where the value would be
-                * needed.
+                * field being replaced; this is needed in case the newval is itself a
+                * FieldStore or ArrayRef that has to obtain and modify the old value.
+                * It's safe to reuse the CASE mechanism because there cannot be a
+                * CASE between here and where the value would be needed, and a field
+                * assignment can't be within a CASE either.  (So saving and restoring
+                * the caseValue is just paranoia, but let's do it anyway.)
                 */
                econtext->caseValue_datum = values[fieldnum - 1];
                econtext->caseValue_isNull = isnull[fieldnum - 1];

diff --git a/src/test/regress/expected/arrays.out b/src/test/regress/expected/arrays.out
index 0f611bf7d7821488209fa2568e59eba7da674ab7..9a9017c29cc7c369257e4bc025d1a6a5af7e8915 100644 (file)
--- a/src/test/regress/expected/arrays.out
+++ b/src/test/regress/expected/arrays.out
@@ -894,3 +894,19 @@ select c2[2].f2 from comptable;
 drop type _comptype;
 drop table comptable;
 drop type comptype;
+-- Insert/update on a column that is array of composite
+create temp table t1 (f1 int8_tbl[]);
+insert into t1 (f1[5].q1) values(42);
+select * from t1;
+       f1        
+-----------------
+ [5:5]={"(42,)"}
+(1 row)
+
+update t1 set f1[5].q2 = 43;
+select * from t1;
+        f1         
+-------------------
+ [5:5]={"(42,43)"}
+(1 row)
+

diff --git a/src/test/regress/sql/arrays.sql b/src/test/regress/sql/arrays.sql
index a60bf560fa88b9a13aaa1cb913e485faa301aadb..c929248cffc39ae14bdc03e26cbb5f2000d578a4 100644 (file)
--- a/src/test/regress/sql/arrays.sql
+++ b/src/test/regress/sql/arrays.sql
@@ -338,3 +338,11 @@ select c2[2].f2 from comptable;
 drop type _comptype;
 drop table comptable;
 drop type comptype;
+
+-- Insert/update on a column that is array of composite
+
+create temp table t1 (f1 int8_tbl[]);
+insert into t1 (f1[5].q1) values(42);
+select * from t1;
+update t1 set f1[5].q2 = 43;
+select * from t1;