Re: [HACKERS] [PATCH] Generic type subscripting

Hi,

On 2018-01-04 16:47:50 +0100, Dmitry Dolgov wrote:
> diff --git a/src/backend/executor/execExpr.c b/src/backend/executor/execExpr.c
> index 16f908037c..ee50fda4ef 100644
> --- a/src/backend/executor/execExpr.c
> +++ b/src/backend/executor/execExpr.c
> @@ -64,7 +64,8 @@ static void ExecInitExprSlots(ExprState *state, Node *node);
>  static bool get_last_attnums_walker(Node *node, LastAttnumInfo *info);
>  static void ExecInitWholeRowVar(ExprEvalStep *scratch, Var *variable,
>  					ExprState *state);
> -static void ExecInitArrayRef(ExprEvalStep *scratch, ArrayRef *aref,
> +static void ExecInitSubscriptingRef(ExprEvalStep *scratch,
> +				 SubscriptingRef *sbsref,
>  				 ExprState *state,
>  				 Datum *resv, bool *resnull);
>  static bool isAssignmentIndirectionExpr(Expr *expr);
> @@ -857,11 +858,11 @@ ExecInitExprRec(Expr *node, ExprState *state,
>  				break;
>  			}
>  
> -		case T_ArrayRef:
> +		case T_SubscriptingRef:
>  			{
> -				ArrayRef   *aref = (ArrayRef *) node;
> +				SubscriptingRef   *sbsref = (SubscriptingRef *) node;
>  
> -				ExecInitArrayRef(&scratch, aref, state, resv, resnull);
> +				ExecInitSubscriptingRef(&scratch, sbsref, state, resv, resnull);
>  				break;
>  			}
>  
> @@ -1176,7 +1177,7 @@ ExecInitExprRec(Expr *node, ExprState *state,
>  					/*
>  					 * Use the CaseTestExpr mechanism to pass down the old
>  					 * value of the field being replaced; this is needed in
> -					 * case the newval is itself a FieldStore or ArrayRef that
> +					 * case the newval is itself a FieldStore or SubscriptingRef 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
> @@ -2401,34 +2402,40 @@ ExecInitWholeRowVar(ExprEvalStep *scratch, Var *variable, ExprState *state)
>  }
>  
>  /*
> - * Prepare evaluation of an ArrayRef expression.
> + * Prepare evaluation of a SubscriptingRef expression.
>   */
>  static void
> -ExecInitArrayRef(ExprEvalStep *scratch, ArrayRef *aref,
> +ExecInitSubscriptingRef(ExprEvalStep *scratch, SubscriptingRef *sbsref,
>  				 ExprState *state, Datum *resv, bool *resnull)
>  {
> -	bool		isAssignment = (aref->refassgnexpr != NULL);
> -	ArrayRefState *arefstate = palloc0(sizeof(ArrayRefState));
> -	List	   *adjust_jumps = NIL;
> -	ListCell   *lc;
> -	int			i;
> +	bool				 isAssignment = (sbsref->refassgnexpr != NULL);
> +	SubscriptingRefState *sbsrefstate = palloc0(sizeof(SubscriptingRefState));
> +	List				 *adjust_jumps = NIL;
> +	ListCell   			 *lc;
> +	int		   			  i;
> +	FmgrInfo   			 *eval_finfo, *nested_finfo;
> +
> +	eval_finfo = palloc0(sizeof(FmgrInfo));
> +	nested_finfo = palloc0(sizeof(FmgrInfo));
> +
> +	fmgr_info(sbsref->refevalfunc, eval_finfo);
> +	if (OidIsValid(sbsref->refnestedfunc))
> +	{
> +		fmgr_info(sbsref->refnestedfunc, nested_finfo);
> +	}
>  
> -	/* Fill constant fields of ArrayRefState */
> -	arefstate->isassignment = isAssignment;
> -	arefstate->refelemtype = aref->refelemtype;
> -	arefstate->refattrlength = get_typlen(aref->refarraytype);
> -	get_typlenbyvalalign(aref->refelemtype,
> -						 &arefstate->refelemlength,
> -						 &arefstate->refelembyval,
> -						 &arefstate->refelemalign);
> +	/* Fill constant fields of SubscriptingRefState */
> +	sbsrefstate->isassignment = isAssignment;
> +	sbsrefstate->refelemtype = sbsref->refelemtype;
> +	sbsrefstate->refattrlength = get_typlen(sbsref->refcontainertype);
>  
>  	/*
>  	 * Evaluate array input.  It's safe to do so into resv/resnull, because we
>  	 * won't use that as target for any of the other subexpressions, and it'll
> -	 * be overwritten by the final EEOP_ARRAYREF_FETCH/ASSIGN step, which is
> +	 * be overwritten by the final EEOP_SBSREF_FETCH/ASSIGN step, which is
>  	 * pushed last.
>  	 */
> -	ExecInitExprRec(aref->refexpr, state, resv, resnull);
> +	ExecInitExprRec(sbsref->refexpr, state, resv, resnull);
>  
>  	/*
>  	 * If refexpr yields NULL, and it's a fetch, then result is NULL.  We can
> @@ -2440,92 +2447,95 @@ ExecInitArrayRef(ExprEvalStep *scratch, ArrayRef *aref,
>  		scratch->opcode = EEOP_JUMP_IF_NULL;
>  		scratch->d.jump.jumpdone = -1;	/* adjust later */
>  		ExprEvalPushStep(state, scratch);
> +
>  		adjust_jumps = lappend_int(adjust_jumps,
>  								   state->steps_len - 1);
>  	}
>  
>  	/* Verify subscript list lengths are within limit */
> -	if (list_length(aref->refupperindexpr) > MAXDIM)
> +	if (list_length(sbsref->refupperindexpr) > MAX_SUBSCRIPT_DEPTH)
>  		ereport(ERROR,
>  				(errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
>  				 errmsg("number of array dimensions (%d) exceeds the maximum allowed (%d)",
> -						list_length(aref->refupperindexpr), MAXDIM)));
> +						list_length(sbsref->refupperindexpr), MAX_SUBSCRIPT_DEPTH)));
>  
> -	if (list_length(aref->reflowerindexpr) > MAXDIM)
> +	if (list_length(sbsref->reflowerindexpr) > MAX_SUBSCRIPT_DEPTH)
>  		ereport(ERROR,
>  				(errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
>  				 errmsg("number of array dimensions (%d) exceeds the maximum allowed (%d)",
> -						list_length(aref->reflowerindexpr), MAXDIM)));
> +						list_length(sbsref->reflowerindexpr), MAX_SUBSCRIPT_DEPTH)));
>  
>  	/* Evaluate upper subscripts */
>  	i = 0;
> -	foreach(lc, aref->refupperindexpr)
> +	foreach(lc, sbsref->refupperindexpr)
>  	{
>  		Expr	   *e = (Expr *) lfirst(lc);
>  
>  		/* When slicing, individual subscript bounds can be omitted */
>  		if (!e)
>  		{
> -			arefstate->upperprovided[i] = false;
> +			sbsrefstate->upperprovided[i] = false;
>  			i++;
>  			continue;
>  		}
>  
> -		arefstate->upperprovided[i] = true;
> +		sbsrefstate->upperprovided[i] = true;
>  
>  		/* Each subscript is evaluated into subscriptvalue/subscriptnull */
>  		ExecInitExprRec(e, state,
> -						&arefstate->subscriptvalue, &arefstate->subscriptnull);
> -
> -		/* ... and then ARRAYREF_SUBSCRIPT saves it into step's workspace */
> -		scratch->opcode = EEOP_ARRAYREF_SUBSCRIPT;
> -		scratch->d.arrayref_subscript.state = arefstate;
> -		scratch->d.arrayref_subscript.off = i;
> -		scratch->d.arrayref_subscript.isupper = true;
> -		scratch->d.arrayref_subscript.jumpdone = -1;	/* adjust later */
> +						&sbsrefstate->subscriptvalue, &sbsrefstate->subscriptnull);
> +
> +		/* ... and then SBSREF_SUBSCRIPT saves it into step's workspace */
> +		scratch->opcode = EEOP_SBSREF_SUBSCRIPT;
> +		scratch->d.sbsref_subscript.state = sbsrefstate;
> +		scratch->d.sbsref_subscript.off = i;
> +		scratch->d.sbsref_subscript.isupper = true;
> +		scratch->d.sbsref_subscript.jumpdone = -1;	/* adjust later */
>  		ExprEvalPushStep(state, scratch);
> +
>  		adjust_jumps = lappend_int(adjust_jumps,
>  								   state->steps_len - 1);
>  		i++;
>  	}
> -	arefstate->numupper = i;
> +	sbsrefstate->numupper = i;
>  
>  	/* Evaluate lower subscripts similarly */
>  	i = 0;
> -	foreach(lc, aref->reflowerindexpr)
> +	foreach(lc, sbsref->reflowerindexpr)
>  	{
>  		Expr	   *e = (Expr *) lfirst(lc);
>  
>  		/* When slicing, individual subscript bounds can be omitted */
>  		if (!e)
>  		{
> -			arefstate->lowerprovided[i] = false;
> +			sbsrefstate->lowerprovided[i] = false;
>  			i++;
>  			continue;
>  		}
>  
> -		arefstate->lowerprovided[i] = true;
> +		sbsrefstate->lowerprovided[i] = true;
>  
>  		/* Each subscript is evaluated into subscriptvalue/subscriptnull */
>  		ExecInitExprRec(e, state,
> -						&arefstate->subscriptvalue, &arefstate->subscriptnull);
> -
> -		/* ... and then ARRAYREF_SUBSCRIPT saves it into step's workspace */
> -		scratch->opcode = EEOP_ARRAYREF_SUBSCRIPT;
> -		scratch->d.arrayref_subscript.state = arefstate;
> -		scratch->d.arrayref_subscript.off = i;
> -		scratch->d.arrayref_subscript.isupper = false;
> -		scratch->d.arrayref_subscript.jumpdone = -1;	/* adjust later */
> +						&sbsrefstate->subscriptvalue, &sbsrefstate->subscriptnull);
> +
> +		/* ... and then SBSREF_SUBSCRIPT saves it into step's workspace */
> +		scratch->opcode = EEOP_SBSREF_SUBSCRIPT;
> +		scratch->d.sbsref_subscript.state = sbsrefstate;
> +		scratch->d.sbsref_subscript.off = i;
> +		scratch->d.sbsref_subscript.isupper = false;
> +		scratch->d.sbsref_subscript.jumpdone = -1;	/* adjust later */
>  		ExprEvalPushStep(state, scratch);
> +
>  		adjust_jumps = lappend_int(adjust_jumps,
>  								   state->steps_len - 1);
>  		i++;
>  	}
> -	arefstate->numlower = i;
> +	sbsrefstate->numlower = i;
>  
>  	/* Should be impossible if parser is sane, but check anyway: */
> -	if (arefstate->numlower != 0 &&
> -		arefstate->numupper != arefstate->numlower)
> +	if (sbsrefstate->numlower != 0 &&
> +		sbsrefstate->numupper != sbsrefstate->numlower)
>  		elog(ERROR, "upper and lower index lists are not same length");
>  
>  	if (isAssignment)
> @@ -2535,7 +2545,7 @@ ExecInitArrayRef(ExprEvalStep *scratch, ArrayRef *aref,
>  
>  		/*
>  		 * We might have a nested-assignment situation, in which the
> -		 * refassgnexpr is itself a FieldStore or ArrayRef that needs to
> +		 * refassgnexpr is itself a FieldStore or SubscriptingRef 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 CaseTestExpr mechanism.  It's safe
> @@ -2547,37 +2557,45 @@ ExecInitArrayRef(ExprEvalStep *scratch, ArrayRef *aref,
>  		 * Since fetching the old element might be a nontrivial expense, do it
>  		 * only if the argument actually needs it.
>  		 */
> -		if (isAssignmentIndirectionExpr(aref->refassgnexpr))
> +		if (isAssignmentIndirectionExpr(sbsref->refassgnexpr))
>  		{
> -			scratch->opcode = EEOP_ARRAYREF_OLD;
> -			scratch->d.arrayref.state = arefstate;
> +			scratch->opcode = EEOP_SBSREF_OLD;
> +			scratch->d.sbsref.state = sbsrefstate;
> +			scratch->d.sbsref.eval_finfo = eval_finfo;
> +			scratch->d.sbsref.nested_finfo = nested_finfo;
>  			ExprEvalPushStep(state, scratch);
>  		}
>  
> -		/* ARRAYREF_OLD puts extracted value into prevvalue/prevnull */
> +		/* SBSREF_OLD puts extracted value into prevvalue/prevnull */
>  		save_innermost_caseval = state->innermost_caseval;
>  		save_innermost_casenull = state->innermost_casenull;
> -		state->innermost_caseval = &arefstate->prevvalue;
> -		state->innermost_casenull = &arefstate->prevnull;
> +		state->innermost_caseval = &sbsrefstate->prevvalue;
> +		state->innermost_casenull = &sbsrefstate->prevnull;
>  
>  		/* evaluate replacement value into replacevalue/replacenull */
> -		ExecInitExprRec(aref->refassgnexpr, state,
> -						&arefstate->replacevalue, &arefstate->replacenull);
> +		ExecInitExprRec(sbsref->refassgnexpr, state,
> +						&sbsrefstate->replacevalue, &sbsrefstate->replacenull);
>  
>  		state->innermost_caseval = save_innermost_caseval;
>  		state->innermost_casenull = save_innermost_casenull;
>  
>  		/* and perform the assignment */
> -		scratch->opcode = EEOP_ARRAYREF_ASSIGN;
> -		scratch->d.arrayref.state = arefstate;
> +		scratch->opcode = EEOP_SBSREF_ASSIGN;
> +		scratch->d.sbsref.state = sbsrefstate;
> +		scratch->d.sbsref.eval_finfo = eval_finfo;
> +		scratch->d.sbsref.nested_finfo = nested_finfo;
>  		ExprEvalPushStep(state, scratch);
> +
>  	}
>  	else
>  	{
>  		/* array fetch is much simpler */
> -		scratch->opcode = EEOP_ARRAYREF_FETCH;
> -		scratch->d.arrayref.state = arefstate;
> +		scratch->opcode = EEOP_SBSREF_FETCH;
> +		scratch->d.sbsref.state = sbsrefstate;
> +		scratch->d.sbsref.eval_finfo = eval_finfo;
> +		scratch->d.sbsref.nested_finfo = nested_finfo;
>  		ExprEvalPushStep(state, scratch);
> +
>  	}
>  
>  	/* adjust jump targets */
> @@ -2585,10 +2603,10 @@ ExecInitArrayRef(ExprEvalStep *scratch, ArrayRef *aref,
>  	{
>  		ExprEvalStep *as = &state->steps[lfirst_int(lc)];
>  
> -		if (as->opcode == EEOP_ARRAYREF_SUBSCRIPT)
> +		if (as->opcode == EEOP_SBSREF_SUBSCRIPT)
>  		{
> -			Assert(as->d.arrayref_subscript.jumpdone == -1);
> -			as->d.arrayref_subscript.jumpdone = state->steps_len;
> +			Assert(as->d.sbsref_subscript.jumpdone == -1);
> +			as->d.sbsref_subscript.jumpdone = state->steps_len;
>  		}
>  		else
>  		{
> @@ -2600,8 +2618,8 @@ ExecInitArrayRef(ExprEvalStep *scratch, ArrayRef *aref,
>  }
>  
>  /*
> - * Helper for preparing ArrayRef expressions for evaluation: is expr a nested
> - * FieldStore or ArrayRef that needs the old element value passed down?
> + * Helper for preparing SubscriptingRef expressions for evaluation: is expr a nested
> + * FieldStore or SubscriptingRef that needs the old element value passed down?
>   *
>   * (We could use this in FieldStore too, but in that case passing the old
>   * value is so cheap there's no need.)
> @@ -2624,11 +2642,11 @@ isAssignmentIndirectionExpr(Expr *expr)
>  		if (fstore->arg && IsA(fstore->arg, CaseTestExpr))
>  			return true;
>  	}
> -	else if (IsA(expr, ArrayRef))
> +	else if (IsA(expr, SubscriptingRef))
>  	{
> -		ArrayRef   *arrayRef = (ArrayRef *) expr;
> +		SubscriptingRef   *sbsRef = (SubscriptingRef *) expr;
>  
> -		if (arrayRef->refexpr && IsA(arrayRef->refexpr, CaseTestExpr))
> +		if (sbsRef->refexpr && IsA(sbsRef->refexpr, CaseTestExpr))
>  			return true;
>  	}
>  	return false;
> diff --git a/src/backend/executor/execExprInterp.c b/src/backend/executor/execExprInterp.c
> index 2e88417265..0c72e80b58 100644
> --- a/src/backend/executor/execExprInterp.c
> +++ b/src/backend/executor/execExprInterp.c
> @@ -364,10 +364,10 @@ ExecInterpExpr(ExprState *state, ExprContext *econtext, bool *isnull)
>  		&&CASE_EEOP_FIELDSELECT,
>  		&&CASE_EEOP_FIELDSTORE_DEFORM,
>  		&&CASE_EEOP_FIELDSTORE_FORM,
> -		&&CASE_EEOP_ARRAYREF_SUBSCRIPT,
> -		&&CASE_EEOP_ARRAYREF_OLD,
> -		&&CASE_EEOP_ARRAYREF_ASSIGN,
> -		&&CASE_EEOP_ARRAYREF_FETCH,
> +		&&CASE_EEOP_SBSREF_SUBSCRIPT,
> +		&&CASE_EEOP_SBSREF_OLD,
> +		&&CASE_EEOP_SBSREF_ASSIGN,
> +		&&CASE_EEOP_SBSREF_FETCH,
>  		&&CASE_EEOP_DOMAIN_TESTVAL,
>  		&&CASE_EEOP_DOMAIN_NOTNULL,
>  		&&CASE_EEOP_DOMAIN_CHECK,
> @@ -1367,43 +1367,43 @@ ExecInterpExpr(ExprState *state, ExprContext *econtext, bool *isnull)
>  			EEO_NEXT();
>  		}
>  
> -		EEO_CASE(EEOP_ARRAYREF_SUBSCRIPT)
> +		EEO_CASE(EEOP_SBSREF_SUBSCRIPT)
>  		{
>  			/* Process an array subscript */
>  
>  			/* too complex for an inline implementation */
> -			if (ExecEvalArrayRefSubscript(state, op))
> +			if (ExecEvalSubscriptingRef(state, op))
>  			{
>  				EEO_NEXT();
>  			}
>  			else
>  			{
> -				/* Subscript is null, short-circuit ArrayRef to NULL */
> -				EEO_JUMP(op->d.arrayref_subscript.jumpdone);
> +				/* Subscript is null, short-circuit SubscriptingRef to NULL */
> +				EEO_JUMP(op->d.sbsref_subscript.jumpdone);
>  			}
>  		}
>  
> -		EEO_CASE(EEOP_ARRAYREF_OLD)
> +		EEO_CASE(EEOP_SBSREF_OLD)
>  		{
>  			/*
> -			 * Fetch the old value in an arrayref assignment, in case it's
> +			 * Fetch the old value in an sbsref assignment, in case it's
>  			 * referenced (via a CaseTestExpr) inside the assignment
>  			 * expression.
>  			 */
>  
>  			/* too complex for an inline implementation */
> -			ExecEvalArrayRefOld(state, op);
> +			ExecEvalSubscriptingRefOld(state, op);
>  
>  			EEO_NEXT();
>  		}
>  
>  		/*
> -		 * Perform ArrayRef assignment
> +		 * Perform SubscriptingRef assignment
>  		 */
> -		EEO_CASE(EEOP_ARRAYREF_ASSIGN)
> +		EEO_CASE(EEOP_SBSREF_ASSIGN)
>  		{
>  			/* too complex for an inline implementation */
> -			ExecEvalArrayRefAssign(state, op);
> +			ExecEvalSubscriptingRefAssign(state, op);
>  
>  			EEO_NEXT();
>  		}
> @@ -1411,10 +1411,10 @@ ExecInterpExpr(ExprState *state, ExprContext *econtext, bool *isnull)
>  		/*
>  		 * Fetch subset of an array.
>  		 */
> -		EEO_CASE(EEOP_ARRAYREF_FETCH)
> +		EEO_CASE(EEOP_SBSREF_FETCH)
>  		{
>  			/* too complex for an inline implementation */
> -			ExecEvalArrayRefFetch(state, op);
> +			ExecEvalSubscriptingRefFetch(state, op);
>  
>  			EEO_NEXT();
>  		}
> @@ -2702,197 +2702,115 @@ ExecEvalFieldStoreForm(ExprState *state, ExprEvalStep *op, ExprContext *econtext
>  }
>  
>  /*
> - * Process a subscript in an ArrayRef expression.
> + * Process a subscript in a SubscriptingRef expression.
>   *
>   * If subscript is NULL, throw error in assignment case, or in fetch case
>   * set result to NULL and return false (instructing caller to skip the rest
> - * of the ArrayRef sequence).
> + * of the SubscriptingRef sequence).
>   *
>   * Subscript expression result is in subscriptvalue/subscriptnull.
>   * On success, integer subscript value has been saved in upperindex[] or
>   * lowerindex[] for use later.
>   */
>  bool
> -ExecEvalArrayRefSubscript(ExprState *state, ExprEvalStep *op)
> +ExecEvalSubscriptingRef(ExprState *state, ExprEvalStep *op)
>  {
> -	ArrayRefState *arefstate = op->d.arrayref_subscript.state;
> -	int		   *indexes;
> -	int			off;
> +	SubscriptingRefState *sbsrefstate = op->d.sbsref_subscript.state;
> +	Datum				 *indexes;
> +	int					 off;
>  
>  	/* If any index expr yields NULL, result is NULL or error */
> -	if (arefstate->subscriptnull)
> +	if (sbsrefstate->subscriptnull)
>  	{
> -		if (arefstate->isassignment)
> +		if (sbsrefstate->isassignment)
>  			ereport(ERROR,
>  					(errcode(ERRCODE_NULL_VALUE_NOT_ALLOWED),
> -					 errmsg("array subscript in assignment must not be null")));
> +					 errmsg("subscript in assignment must not be null")));
>  		*op->resnull = true;
>  		return false;
>  	}
>  
>  	/* Convert datum to int, save in appropriate place */
> -	if (op->d.arrayref_subscript.isupper)
> -		indexes = arefstate->upperindex;
> +	if (op->d.sbsref_subscript.isupper)
> +		indexes = sbsrefstate->upper;
>  	else
> -		indexes = arefstate->lowerindex;
> -	off = op->d.arrayref_subscript.off;
> +		indexes = sbsrefstate->lower;
> +	off = op->d.sbsref_subscript.off;
>  
> -	indexes[off] = DatumGetInt32(arefstate->subscriptvalue);
> +	indexes[off] = sbsrefstate->subscriptvalue;
>  
>  	return true;
>  }
>  
>  /*
> - * Evaluate ArrayRef fetch.
> + * Evaluate SubscriptingRef fetch.
>   *
> - * Source array is in step's result variable.
> + * Source container is in step's result variable.
>   */
>  void
> -ExecEvalArrayRefFetch(ExprState *state, ExprEvalStep *op)
> +ExecEvalSubscriptingRefFetch(ExprState *state, ExprEvalStep *op)
>  {
> -	ArrayRefState *arefstate = op->d.arrayref.state;
> -
> -	/* Should not get here if source array (or any subscript) is null */
> +	/* Should not get here if source container (or any subscript) is null */
>  	Assert(!(*op->resnull));
>  
> -	if (arefstate->numlower == 0)
> -	{
> -		/* Scalar case */
> -		*op->resvalue = array_get_element(*op->resvalue,
> -										  arefstate->numupper,
> -										  arefstate->upperindex,
> -										  arefstate->refattrlength,
> -										  arefstate->refelemlength,
> -										  arefstate->refelembyval,
> -										  arefstate->refelemalign,
> -										  op->resnull);
> -	}
> -	else
> -	{
> -		/* Slice case */
> -		*op->resvalue = array_get_slice(*op->resvalue,
> -										arefstate->numupper,
> -										arefstate->upperindex,
> -										arefstate->lowerindex,
> -										arefstate->upperprovided,
> -										arefstate->lowerprovided,
> -										arefstate->refattrlength,
> -										arefstate->refelemlength,
> -										arefstate->refelembyval,
> -										arefstate->refelemalign);
> -	}
> +	*op->resvalue = FunctionCall2(op->d.sbsref.eval_finfo,
> +				  PointerGetDatum(*op->resvalue),
> +				  PointerGetDatum(op));
>  }
>  
>  /*
> - * Compute old array element/slice value for an ArrayRef assignment
> - * expression.  Will only be generated if the new-value subexpression
> - * contains ArrayRef or FieldStore.  The value is stored into the
> - * ArrayRefState's prevvalue/prevnull fields.
> + * Compute old container element/slice value for a SubscriptingRef assignment
> + * expression. Will only be generated if the new-value subexpression
> + * contains SubscriptingRef or FieldStore. The value is stored into the
> + * SubscriptingRefState's prevvalue/prevnull fields.
>   */
>  void
> -ExecEvalArrayRefOld(ExprState *state, ExprEvalStep *op)
> +ExecEvalSubscriptingRefOld(ExprState *state, ExprEvalStep *op)
>  {
> -	ArrayRefState *arefstate = op->d.arrayref.state;
> +	SubscriptingRefState *sbsrefstate = op->d.sbsref.state;
>  
>  	if (*op->resnull)
>  	{
> -		/* whole array is null, so any element or slice is too */
> -		arefstate->prevvalue = (Datum) 0;
> -		arefstate->prevnull = true;
> -	}
> -	else if (arefstate->numlower == 0)
> -	{
> -		/* Scalar case */
> -		arefstate->prevvalue = array_get_element(*op->resvalue,
> -												 arefstate->numupper,
> -												 arefstate->upperindex,
> -												 arefstate->refattrlength,
> -												 arefstate->refelemlength,
> -												 arefstate->refelembyval,
> -												 arefstate->refelemalign,
> -												 &arefstate->prevnull);
> +		/* whole container is null, so any element or slice is too */
> +		sbsrefstate->prevvalue = (Datum) 0;
> +		sbsrefstate->prevnull = true;
>  	}
>  	else
>  	{
> -		/* Slice case */
> -		/* this is currently unreachable */
> -		arefstate->prevvalue = array_get_slice(*op->resvalue,
> -											   arefstate->numupper,
> -											   arefstate->upperindex,
> -											   arefstate->lowerindex,
> -											   arefstate->upperprovided,
> -											   arefstate->lowerprovided,
> -											   arefstate->refattrlength,
> -											   arefstate->refelemlength,
> -											   arefstate->refelembyval,
> -											   arefstate->refelemalign);
> -		arefstate->prevnull = false;
> +		sbsrefstate->prevvalue = FunctionCall2(op->d.sbsref.nested_finfo,
> +					  PointerGetDatum(*op->resvalue),
> +					  PointerGetDatum(op));
> +
> +		if (sbsrefstate->numlower != 0)
> +			sbsrefstate->prevnull = false;
> +
>  	}
>  }
>  
>  /*
> - * Evaluate ArrayRef assignment.
> + * Evaluate SubscriptingRef assignment.
>   *
> - * Input array (possibly null) is in result area, replacement value is in
> - * ArrayRefState's replacevalue/replacenull.
> + * Input container (possibly null) is in result area, replacement value is in
> + * SubscriptingRefState's replacevalue/replacenull.
>   */
>  void
> -ExecEvalArrayRefAssign(ExprState *state, ExprEvalStep *op)
> +ExecEvalSubscriptingRefAssign(ExprState *state, ExprEvalStep *op)
>  {
> -	ArrayRefState *arefstate = op->d.arrayref.state;
> -
> +	SubscriptingRefState *sbsrefstate = op->d.sbsref.state;
>  	/*
> -	 * 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 we punt and
> -	 * return the original array.
> +	 * For an assignment to a fixed-length container type, both the original
> +	 * container and the value to be assigned into it must be non-NULL, else we
> +	 * punt and return the original container.
>  	 */
> -	if (arefstate->refattrlength > 0)	/* fixed-length array? */
> +	if (sbsrefstate->refattrlength > 0)
>  	{
> -		if (*op->resnull || arefstate->replacenull)
> +		if (*op->resnull || sbsrefstate->replacenull)
>  			return;
>  	}
>  
> -	/*
> -	 * For assignment to varlena arrays, we handle a NULL original array by
> -	 * substituting an empty (zero-dimensional) array; insertion of the new
> -	 * element will result in a singleton array value.  It does not matter
> -	 * whether the new element is NULL.
> -	 */
> -	if (*op->resnull)
> -	{
> -		*op->resvalue = PointerGetDatum(construct_empty_array(arefstate->refelemtype));
> -		*op->resnull = false;
> -	}
> -
> -	if (arefstate->numlower == 0)
> -	{
> -		/* Scalar case */
> -		*op->resvalue = array_set_element(*op->resvalue,
> -										  arefstate->numupper,
> -										  arefstate->upperindex,
> -										  arefstate->replacevalue,
> -										  arefstate->replacenull,
> -										  arefstate->refattrlength,
> -										  arefstate->refelemlength,
> -										  arefstate->refelembyval,
> -										  arefstate->refelemalign);
> -	}
> -	else
> -	{
> -		/* Slice case */
> -		*op->resvalue = array_set_slice(*op->resvalue,
> -										arefstate->numupper,
> -										arefstate->upperindex,
> -										arefstate->lowerindex,
> -										arefstate->upperprovided,
> -										arefstate->lowerprovided,
> -										arefstate->replacevalue,
> -										arefstate->replacenull,
> -										arefstate->refattrlength,
> -										arefstate->refelemlength,
> -										arefstate->refelembyval,
> -										arefstate->refelemalign);
> -	}
> +	*op->resvalue = FunctionCall2(op->d.sbsref.eval_finfo,
> +								  PointerGetDatum(*op->resvalue),
> +								  PointerGetDatum(op));
>  }


You might not love me for this suggestion, but I'd like to see the
renaming here split from the rest of the patch. There's a lot of diff
that's just more or less automatic changes, making it hard to see the
actual meaningful changes.

- Andres