Re: [HACKERS] [PATCH] Generic type subscripting - Mailing list pgsql-hackers

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