On Fri, 16 Oct 2020 at 16:42, Hou, Zhijie <houzj.f...@cn.fujitsu.com> wrote: > And after checking the code again and I found two more places which can be > improved. > > 1. > --- a/src/backend/parser/parse_expr.c > +++ b/src/backend/parser/parse_expr.c > @@ -1702,7 +1702,7 @@ transformMultiAssignRef(ParseState *pstate, > MultiAssignRef *maref) > */ > if (maref->colno == maref->ncolumns) > pstate->p_multiassign_exprs = > - list_delete_ptr(pstate->p_multiassign_exprs, > tle); > + list_delete_last(pstate->p_multiassign_exprs); > > Based on the logic above in function transformMultiAssignRef, > I found 'tle' is always the last one in list ' pstate->p_multiassign_exprs ' , > So list_delete_last seems can be used here.
Yeah. After a bit of looking I agree. There's a similar assumption there already with: /* * Second or later column in a multiassignment. Re-fetch the * transformed SubLink or RowExpr, which we assume is still the last * entry in p_multiassign_exprs. */ Assert(pstate->p_multiassign_exprs != NIL); tle = (TargetEntry *) llast(pstate->p_multiassign_exprs); > 2. > > + nameEl_idx = foreach_current_index(option); > } > } > > @@ -405,7 +407,7 @@ generateSerialExtraStmts(CreateStmtContext *cxt, > ColumnDef *column, > } > sname = rv->relname; > /* Remove the SEQUENCE NAME item from seqoptions */ > - seqoptions = list_delete_ptr(seqoptions, nameEl); > + seqoptions = list_delete_nth_cell(seqoptions, nameEl_idx); > > Add a new var ' nameEl_idx ' to catch the index. Yeah. That looks fine too. Pushed. David
