From e983c4d1aa42d613542cf222e222b034918374b1 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Mon, 16 Feb 2015 12:23:58 -0500 Subject: [PATCH] Rationalize the APIs of array element/slice access functions. The four functions array_ref, array_set, array_get_slice, array_set_slice have traditionally declared their array inputs and results as being of type "ArrayType *". This is a lie, and has been since Berkeley days, because they actually also support "fixed-length array" types such as "name" and "point"; not to mention that the inputs could be toasted. These values should be declared Datum instead to avoid confusion. The current coding already risks possible misoptimization by compilers, and it'll get worse when "expanded" array representations become a valid alternative. However, there's a fair amount of code using array_ref and array_set with arrays that *are* known to be ArrayType structures, and there might be more such places in third-party code. Rather than cluttering those call sites with PointerGetDatum/DatumGetArrayTypeP cruft, what I did was to rename the existing functions to array_get_element/array_set_element, fix their signatures, then reincarnate array_ref/array_set as backwards compatibility wrappers. array_get_slice/array_set_slice have no such constituency in the core code, and probably not in third-party code either, so I just changed their APIs. --- src/backend/executor/execQual.c | 112 +++++++++---------- src/backend/rewrite/rewriteHandler.c | 2 +- src/backend/utils/adt/arrayfuncs.c | 158 +++++++++++++++++---------- src/include/utils/array.h | 21 ++-- src/pl/plpgsql/src/pl_exec.c | 29 +++-- 5 files changed, 180 insertions(+), 142 deletions(-) diff --git a/src/backend/executor/execQual.c b/src/backend/executor/execQual.c index dd9d0883e4..fec76d4f1b 100644 --- a/src/backend/executor/execQual.c +++ b/src/backend/executor/execQual.c @@ -252,12 +252,6 @@ static Datum ExecEvalCurrentOfExpr(ExprState *exprstate, ExprContext *econtext, * * NOTE: if we get a NULL result from a subscript expression, we return NULL * when it's an array reference, or raise an error when it's an assignment. - * - * NOTE: we deliberately refrain from applying DatumGetArrayTypeP() here, - * even though that might seem natural, because this code needs to support - * both varlena arrays and fixed-length array types. DatumGetArrayTypeP() - * only works for the varlena kind. The routines we call in arrayfuncs.c - * have to know the difference (that's what they need refattrlength for). *---------- */ static Datum @@ -267,8 +261,7 @@ ExecEvalArrayRef(ArrayRefExprState *astate, ExprDoneCond *isDone) { ArrayRef *arrayRef = (ArrayRef *) astate->xprstate.expr; - ArrayType *array_source; - ArrayType *resultArray; + Datum array_source; bool isAssignment = (arrayRef->refassgnexpr != NULL); bool eisnull; ListCell *l; @@ -278,11 +271,10 @@ ExecEvalArrayRef(ArrayRefExprState *astate, lower; int *lIndex; - array_source = (ArrayType *) - DatumGetPointer(ExecEvalExpr(astate->refexpr, - econtext, - isNull, - isDone)); + array_source = ExecEvalExpr(astate->refexpr, + econtext, + isNull, + isDone); /* * If refexpr yields NULL, and it's a fetch, then result is NULL. In the @@ -390,23 +382,24 @@ ExecEvalArrayRef(ArrayRefExprState *astate, } 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); + econtext->caseValue_datum = + array_get_element(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_datum = + array_get_slice(array_source, i, + upper.indx, lower.indx, + astate->refattrlength, + astate->refelemlength, + astate->refelembyval, + astate->refelemalign); econtext->caseValue_isNull = false; } } @@ -435,7 +428,7 @@ ExecEvalArrayRef(ArrayRefExprState *astate, */ if (astate->refattrlength > 0) /* fixed-length array? */ if (eisnull || *isNull) - return PointerGetDatum(array_source); + return array_source; /* * For assignment to varlena arrays, we handle a NULL original array @@ -445,48 +438,45 @@ ExecEvalArrayRef(ArrayRefExprState *astate, */ if (*isNull) { - array_source = construct_empty_array(arrayRef->refelemtype); + array_source = PointerGetDatum(construct_empty_array(arrayRef->refelemtype)); *isNull = false; } if (lIndex == NULL) - resultArray = array_set(array_source, i, - upper.indx, - sourceData, - eisnull, - astate->refattrlength, - astate->refelemlength, - astate->refelembyval, - astate->refelemalign); + return array_set_element(array_source, i, + upper.indx, + sourceData, + eisnull, + astate->refattrlength, + astate->refelemlength, + astate->refelembyval, + astate->refelemalign); else - resultArray = array_set_slice(array_source, i, - upper.indx, lower.indx, - (ArrayType *) DatumGetPointer(sourceData), - eisnull, - astate->refattrlength, - astate->refelemlength, - astate->refelembyval, - astate->refelemalign); - return PointerGetDatum(resultArray); + return array_set_slice(array_source, i, + upper.indx, lower.indx, + sourceData, + eisnull, + astate->refattrlength, + astate->refelemlength, + astate->refelembyval, + astate->refelemalign); } if (lIndex == NULL) - return array_ref(array_source, i, upper.indx, - astate->refattrlength, - astate->refelemlength, - astate->refelembyval, - astate->refelemalign, - isNull); + return array_get_element(array_source, i, + upper.indx, + astate->refattrlength, + astate->refelemlength, + astate->refelembyval, + astate->refelemalign, + isNull); else - { - resultArray = array_get_slice(array_source, i, - upper.indx, lower.indx, - astate->refattrlength, - astate->refelemlength, - astate->refelembyval, - astate->refelemalign); - return PointerGetDatum(resultArray); - } + return array_get_slice(array_source, i, + upper.indx, lower.indx, + astate->refattrlength, + astate->refelemlength, + astate->refelembyval, + astate->refelemalign); } /* diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c index b8e6e7a605..9d2c280b32 100644 --- a/src/backend/rewrite/rewriteHandler.c +++ b/src/backend/rewrite/rewriteHandler.c @@ -664,7 +664,7 @@ adjustJoinTreeList(Query *parsetree, bool removert, int rt_index) * UPDATE table SET foo[2] = 42, foo[4] = 43; * We can merge such operations into a single assignment op. Essentially, * the expression we want to produce in this case is like - * foo = array_set(array_set(foo, 2, 42), 4, 43) + * foo = array_set_element(array_set_element(foo, 2, 42), 4, 43) * * 4. Sort the tlist into standard order: non-junk fields in order by resno, * then junk fields (these in no particular order). diff --git a/src/backend/utils/adt/arrayfuncs.c b/src/backend/utils/adt/arrayfuncs.c index 5591b46309..79aefaf6a4 100644 --- a/src/backend/utils/adt/arrayfuncs.c +++ b/src/backend/utils/adt/arrayfuncs.c @@ -1795,15 +1795,15 @@ array_cardinality(PG_FUNCTION_ARGS) /* - * array_ref : - * This routine takes an array pointer and a subscript array and returns + * array_get_element : + * This routine takes an array datum and a subscript array and returns * the referenced item as a Datum. Note that for a pass-by-reference * datatype, the returned Datum is a pointer into the array object. * * This handles both ordinary varlena arrays and fixed-length arrays. * * Inputs: - * array: the array object (mustn't be NULL) + * arraydatum: the array object (mustn't be NULL) * nSubscripts: number of subscripts supplied * indx[]: the subscript values * arraytyplen: pg_type.typlen for the array type @@ -1816,15 +1816,16 @@ array_cardinality(PG_FUNCTION_ARGS) * *isNull is set to indicate whether the element is NULL. */ Datum -array_ref(ArrayType *array, - int nSubscripts, - int *indx, - int arraytyplen, - int elmlen, - bool elmbyval, - char elmalign, - bool *isNull) +array_get_element(Datum arraydatum, + int nSubscripts, + int *indx, + int arraytyplen, + int elmlen, + bool elmbyval, + char elmalign, + bool *isNull) { + ArrayType *array; int i, ndim, *dim, @@ -1846,13 +1847,13 @@ array_ref(ArrayType *array, fixedLb[0] = 0; dim = fixedDim; lb = fixedLb; - arraydataptr = (char *) array; + arraydataptr = (char *) DatumGetPointer(arraydatum); arraynullsptr = NULL; } else { /* detoast input array if necessary */ - array = DatumGetArrayTypeP(PointerGetDatum(array)); + array = DatumGetArrayTypeP(arraydatum); ndim = ARR_NDIM(array); dim = ARR_DIMS(array); @@ -1910,7 +1911,7 @@ array_ref(ArrayType *array, * This handles both ordinary varlena arrays and fixed-length arrays. * * Inputs: - * array: the array object (mustn't be NULL) + * arraydatum: the array object (mustn't be NULL) * nSubscripts: number of subscripts supplied (must be same for upper/lower) * upperIndx[]: the upper subscript values * lowerIndx[]: the lower subscript values @@ -1925,8 +1926,8 @@ array_ref(ArrayType *array, * NOTE: we assume it is OK to scribble on the provided subscript arrays * lowerIndx[] and upperIndx[]. These are generally just temporaries. */ -ArrayType * -array_get_slice(ArrayType *array, +Datum +array_get_slice(Datum arraydatum, int nSubscripts, int *upperIndx, int *lowerIndx, @@ -1935,6 +1936,7 @@ array_get_slice(ArrayType *array, bool elmbyval, char elmalign) { + ArrayType *array; ArrayType *newarray; int i, ndim, @@ -1973,13 +1975,13 @@ array_get_slice(ArrayType *array, dim = fixedDim; lb = fixedLb; elemtype = InvalidOid; /* XXX */ - arraydataptr = (char *) array; + arraydataptr = (char *) DatumGetPointer(arraydatum); arraynullsptr = NULL; } else { /* detoast input array if necessary */ - array = DatumGetArrayTypeP(PointerGetDatum(array)); + array = DatumGetArrayTypeP(arraydatum); ndim = ARR_NDIM(array); dim = ARR_DIMS(array); @@ -1995,7 +1997,7 @@ array_get_slice(ArrayType *array, * slice, return an empty array. */ if (ndim < nSubscripts || ndim <= 0 || ndim > MAXDIM) - return construct_empty_array(elemtype); + return PointerGetDatum(construct_empty_array(elemtype)); for (i = 0; i < nSubscripts; i++) { @@ -2004,7 +2006,7 @@ array_get_slice(ArrayType *array, if (upperIndx[i] >= (dim[i] + lb[i])) upperIndx[i] = dim[i] + lb[i] - 1; if (lowerIndx[i] > upperIndx[i]) - return construct_empty_array(elemtype); + return PointerGetDatum(construct_empty_array(elemtype)); } /* fill any missing subscript positions with full array range */ for (; i < ndim; i++) @@ -2012,7 +2014,7 @@ array_get_slice(ArrayType *array, lowerIndx[i] = lb[i]; upperIndx[i] = dim[i] + lb[i] - 1; if (lowerIndx[i] > upperIndx[i]) - return construct_empty_array(elemtype); + return PointerGetDatum(construct_empty_array(elemtype)); } mda_get_range(ndim, span, lowerIndx, upperIndx); @@ -2058,18 +2060,18 @@ array_get_slice(ArrayType *array, lowerIndx, upperIndx, elmlen, elmbyval, elmalign); - return newarray; + return PointerGetDatum(newarray); } /* - * array_set : - * This routine sets the value of an array element (specified by + * array_set_element : + * This routine sets the value of one array element (specified by * a subscript array) to a new value specified by "dataValue". * * This handles both ordinary varlena arrays and fixed-length arrays. * * Inputs: - * array: the initial array object (mustn't be NULL) + * arraydatum: the initial array object (mustn't be NULL) * nSubscripts: number of subscripts supplied * indx[]: the subscript values * dataValue: the datum to be inserted at the given position @@ -2091,17 +2093,18 @@ array_get_slice(ArrayType *array, * NOTE: For assignments, we throw an error for invalid subscripts etc, * rather than returning a NULL as the fetch operations do. */ -ArrayType * -array_set(ArrayType *array, - int nSubscripts, - int *indx, - Datum dataValue, - bool isNull, - int arraytyplen, - int elmlen, - bool elmbyval, - char elmalign) +Datum +array_set_element(Datum arraydatum, + int nSubscripts, + int *indx, + Datum dataValue, + bool isNull, + int arraytyplen, + int elmlen, + bool elmbyval, + char elmalign) { + ArrayType *array; ArrayType *newarray; int i, ndim, @@ -2130,6 +2133,8 @@ array_set(ArrayType *array, * fixed-length arrays -- these are assumed to be 1-d, 0-based. We * cannot extend them, either. */ + char *resultarray; + if (nSubscripts != 1) ereport(ERROR, (errcode(ERRCODE_ARRAY_SUBSCRIPT_ERROR), @@ -2145,11 +2150,11 @@ array_set(ArrayType *array, (errcode(ERRCODE_NULL_VALUE_NOT_ALLOWED), errmsg("cannot assign null value to an element of a fixed-length array"))); - newarray = (ArrayType *) palloc(arraytyplen); - memcpy(newarray, array, arraytyplen); - elt_ptr = (char *) newarray + indx[0] * elmlen; + resultarray = (char *) palloc(arraytyplen); + memcpy(resultarray, DatumGetPointer(arraydatum), arraytyplen); + elt_ptr = (char *) resultarray + indx[0] * elmlen; ArrayCastAndSet(dataValue, elmlen, elmbyval, elmalign, elt_ptr); - return newarray; + return PointerGetDatum(resultarray); } if (nSubscripts <= 0 || nSubscripts > MAXDIM) @@ -2162,7 +2167,7 @@ array_set(ArrayType *array, dataValue = PointerGetDatum(PG_DETOAST_DATUM(dataValue)); /* detoast input array if necessary */ - array = DatumGetArrayTypeP(PointerGetDatum(array)); + array = DatumGetArrayTypeP(arraydatum); ndim = ARR_NDIM(array); @@ -2181,9 +2186,10 @@ array_set(ArrayType *array, lb[i] = indx[i]; } - return construct_md_array(&dataValue, &isNull, nSubscripts, - dim, lb, elmtype, - elmlen, elmbyval, elmalign); + return PointerGetDatum(construct_md_array(&dataValue, &isNull, + nSubscripts, dim, lb, + elmtype, + elmlen, elmbyval, elmalign)); } if (ndim != nSubscripts) @@ -2345,7 +2351,7 @@ array_set(ArrayType *array, } } - return newarray; + return PointerGetDatum(newarray); } /* @@ -2357,12 +2363,12 @@ array_set(ArrayType *array, * This handles both ordinary varlena arrays and fixed-length arrays. * * Inputs: - * array: the initial array object (mustn't be NULL) + * arraydatum: the initial array object (mustn't be NULL) * nSubscripts: number of subscripts supplied (must be same for upper/lower) * upperIndx[]: the upper subscript values * lowerIndx[]: the lower subscript values - * srcArray: the source for the inserted values - * isNull: indicates whether srcArray is NULL + * srcArrayDatum: the source for the inserted values + * isNull: indicates whether srcArrayDatum is NULL * arraytyplen: pg_type.typlen for the array type * elmlen: pg_type.typlen for the array's element type * elmbyval: pg_type.typbyval for the array's element type @@ -2383,18 +2389,20 @@ array_set(ArrayType *array, * NOTE: For assignments, we throw an error for silly subscripts etc, * rather than returning a NULL or empty array as the fetch operations do. */ -ArrayType * -array_set_slice(ArrayType *array, +Datum +array_set_slice(Datum arraydatum, int nSubscripts, int *upperIndx, int *lowerIndx, - ArrayType *srcArray, + Datum srcArrayDatum, bool isNull, int arraytyplen, int elmlen, bool elmbyval, char elmalign) { + ArrayType *array; + ArrayType *srcArray; ArrayType *newarray; int i, ndim, @@ -2420,7 +2428,7 @@ array_set_slice(ArrayType *array, /* Currently, assignment from a NULL source array is a no-op */ if (isNull) - return array; + return arraydatum; if (arraytyplen > 0) { @@ -2433,8 +2441,8 @@ array_set_slice(ArrayType *array, } /* detoast arrays if necessary */ - array = DatumGetArrayTypeP(PointerGetDatum(array)); - srcArray = DatumGetArrayTypeP(PointerGetDatum(srcArray)); + array = DatumGetArrayTypeP(arraydatum); + srcArray = DatumGetArrayTypeP(srcArrayDatum); /* note: we assume srcArray contains no toasted elements */ @@ -2467,9 +2475,9 @@ array_set_slice(ArrayType *array, (errcode(ERRCODE_ARRAY_SUBSCRIPT_ERROR), errmsg("source array too small"))); - return construct_md_array(dvalues, dnulls, nSubscripts, - dim, lb, elmtype, - elmlen, elmbyval, elmalign); + return PointerGetDatum(construct_md_array(dvalues, dnulls, nSubscripts, + dim, lb, elmtype, + elmlen, elmbyval, elmalign)); } if (ndim < nSubscripts || ndim <= 0 || ndim > MAXDIM) @@ -2671,7 +2679,43 @@ array_set_slice(ArrayType *array, } } - return newarray; + return PointerGetDatum(newarray); +} + +/* + * array_ref : backwards compatibility wrapper for array_get_element + * + * This only works for detoasted/flattened varlena arrays, since the array + * argument is declared as "ArrayType *". However there's enough code like + * that to justify preserving this API. + */ +Datum +array_ref(ArrayType *array, int nSubscripts, int *indx, + int arraytyplen, int elmlen, bool elmbyval, char elmalign, + bool *isNull) +{ + return array_get_element(PointerGetDatum(array), nSubscripts, indx, + arraytyplen, elmlen, elmbyval, elmalign, + isNull); +} + +/* + * array_set : backwards compatibility wrapper for array_set_element + * + * This only works for detoasted/flattened varlena arrays, since the array + * argument and result are declared as "ArrayType *". However there's enough + * code like that to justify preserving this API. + */ +ArrayType * +array_set(ArrayType *array, int nSubscripts, int *indx, + Datum dataValue, bool isNull, + int arraytyplen, int elmlen, bool elmbyval, char elmalign) +{ + return DatumGetArrayTypeP(array_set_element(PointerGetDatum(array), + nSubscripts, indx, + dataValue, isNull, + arraytyplen, + elmlen, elmbyval, elmalign)); } /* diff --git a/src/include/utils/array.h b/src/include/utils/array.h index 694bce7e16..dff69eba06 100644 --- a/src/include/utils/array.h +++ b/src/include/utils/array.h @@ -248,19 +248,26 @@ extern Datum array_remove(PG_FUNCTION_ARGS); extern Datum array_replace(PG_FUNCTION_ARGS); extern Datum width_bucket_array(PG_FUNCTION_ARGS); +extern Datum array_get_element(Datum arraydatum, int nSubscripts, int *indx, + int arraytyplen, int elmlen, bool elmbyval, char elmalign, + bool *isNull); +extern Datum array_set_element(Datum arraydatum, int nSubscripts, int *indx, + Datum dataValue, bool isNull, + int arraytyplen, int elmlen, bool elmbyval, char elmalign); +extern Datum array_get_slice(Datum arraydatum, int nSubscripts, + int *upperIndx, int *lowerIndx, + int arraytyplen, int elmlen, bool elmbyval, char elmalign); +extern Datum array_set_slice(Datum arraydatum, int nSubscripts, + int *upperIndx, int *lowerIndx, + Datum srcArrayDatum, bool isNull, + int arraytyplen, int elmlen, bool elmbyval, char elmalign); + extern Datum array_ref(ArrayType *array, int nSubscripts, int *indx, int arraytyplen, int elmlen, bool elmbyval, char elmalign, bool *isNull); extern ArrayType *array_set(ArrayType *array, int nSubscripts, int *indx, Datum dataValue, bool isNull, int arraytyplen, int elmlen, bool elmbyval, char elmalign); -extern ArrayType *array_get_slice(ArrayType *array, int nSubscripts, - int *upperIndx, int *lowerIndx, - int arraytyplen, int elmlen, bool elmbyval, char elmalign); -extern ArrayType *array_set_slice(ArrayType *array, int nSubscripts, - int *upperIndx, int *lowerIndx, - ArrayType *srcArray, bool isNull, - int arraytyplen, int elmlen, bool elmbyval, char elmalign); extern Datum array_map(FunctionCallInfo fcinfo, Oid inpType, Oid retType, ArrayMapState *amstate); diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c index ae5421fd97..f5ed02a048 100644 --- a/src/pl/plpgsql/src/pl_exec.c +++ b/src/pl/plpgsql/src/pl_exec.c @@ -4233,12 +4233,11 @@ exec_assign_value(PLpgSQL_execstate *estate, PLpgSQL_expr *subscripts[MAXDIM]; int subscriptvals[MAXDIM]; Datum oldarraydatum, + newarraydatum, coerced_value; bool oldarrayisnull; Oid parenttypoid; int32 parenttypmod; - ArrayType *oldarrayval; - ArrayType *newarrayval; SPITupleTable *save_eval_tuptable; MemoryContext oldcontext; @@ -4378,26 +4377,24 @@ exec_assign_value(PLpgSQL_execstate *estate, (oldarrayisnull || *isNull)) return; - /* oldarrayval and newarrayval should be short-lived */ + /* empty array, if any, and newarraydatum are short-lived */ oldcontext = MemoryContextSwitchTo(estate->eval_econtext->ecxt_per_tuple_memory); if (oldarrayisnull) - oldarrayval = construct_empty_array(arrayelem->elemtypoid); - else - oldarrayval = (ArrayType *) DatumGetPointer(oldarraydatum); + oldarraydatum = PointerGetDatum(construct_empty_array(arrayelem->elemtypoid)); /* * Build the modified array value. */ - newarrayval = array_set(oldarrayval, - nsubscripts, - subscriptvals, - coerced_value, - *isNull, - arrayelem->arraytyplen, - arrayelem->elemtyplen, - arrayelem->elemtypbyval, - arrayelem->elemtypalign); + newarraydatum = array_set_element(oldarraydatum, + nsubscripts, + subscriptvals, + coerced_value, + *isNull, + arrayelem->arraytyplen, + arrayelem->elemtyplen, + arrayelem->elemtypbyval, + arrayelem->elemtypalign); MemoryContextSwitchTo(oldcontext); @@ -4409,7 +4406,7 @@ exec_assign_value(PLpgSQL_execstate *estate, */ *isNull = false; exec_assign_value(estate, target, - PointerGetDatum(newarrayval), + newarraydatum, arrayelem->arraytypoid, isNull); break; } -- 2.40.0