From dfb3bc020dc2709aa78156fef63bfa60f6fc8b32 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Thu, 18 Feb 2010 18:41:55 +0000 Subject: [PATCH] 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. --- src/backend/executor/execQual.c | 107 ++++++++++++++++++++++++--- src/test/regress/expected/arrays.out | 16 ++++ src/test/regress/sql/arrays.sql | 8 ++ 3 files changed, 119 insertions(+), 12 deletions(-) diff --git a/src/backend/executor/execQual.c b/src/backend/executor/execQual.c index 702727bf7f..674421f5be 100644 --- 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.250.2.3 2010/01/11 15:31:12 tgl Exp $ + * $PostgreSQL: pgsql/src/backend/executor/execQual.c,v 1.250.2.4 2010/02/18 18:41:55 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -60,6 +60,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); @@ -348,22 +349,74 @@ ExecEvalArrayRef(ArrayRefExprState *astate, if (isAssignment) { Datum sourceData; + Datum save_datum; + bool save_isNull; + + /* + * 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.) + * + * 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. - * - * 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. */ 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 @@ -425,6 +478,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 @@ -3928,10 +4009,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 15f3f6082e..03c32a5cea 100644 --- a/src/test/regress/expected/arrays.out +++ b/src/test/regress/expected/arrays.out @@ -1191,3 +1191,19 @@ select unnest(array[1,2,3,null,4,null,null,5,6]::text[]); 6 (9 rows) +-- 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 5e4933c31d..f65bc452e2 100644 --- a/src/test/regress/sql/arrays.sql +++ b/src/test/regress/sql/arrays.sql @@ -404,3 +404,11 @@ select unnest(array[1,2,3,4.5]::float8[]); select unnest(array[1,2,3,4.5]::numeric[]); select unnest(array[1,2,3,null,4,null,null,5,6]); select unnest(array[1,2,3,null,4,null,null,5,6]::text[]); + +-- 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;