RE: Use list_delete_xxxcell O(1) instead of list_delete_ptr O(N) in some places

> > I found some code places call list_delete_ptr can be replaced by
> list_delete_xxxcell which can be faster.
> >
> > diff --git a/src/backend/optimizer/path/joinpath.c
> > b/src/backend/optimizer/path/joinpath.c
> > index db54a6b..61ef7c8 100644
> > --- a/src/backend/optimizer/path/joinpath.c
> > +++ b/src/backend/optimizer/path/joinpath.c
> > @@ -1005,8 +1005,8 @@ sort_inner_and_outer(PlannerInfo *root,
> > /* Make a pathkey list with this guy first */
> > if (l != list_head(all_pathkeys))
> > outerkeys = lcons(front_pathkey,
> > -
> list_delete_ptr(list_copy(all_pathkeys),
> > -
> front_pathkey));
> > +
> list_delete_nth_cell(list_copy(all_pathkeys),
> > +
> > + foreach_current_index(l)));
> > else
> > outerkeys = all_pathkeys; /* no work at
> first one... */
>
> That looks ok to me. It would be more optimal if we had a method to move
> an element to the front of a list, or to any specified position, but I can't
> imagine it's worth making such a function just for that.
> So what you have there seems fine.
>
> > diff --git a/src/backend/rewrite/rewriteHandler.c
> > b/src/backend/rewrite/rewriteHandler.c
> > index fe777c3..d0f15b8 100644
> > --- a/src/backend/rewrite/rewriteHandler.c
> > +++ b/src/backend/rewrite/rewriteHandler.c
> > @@ -650,7 +650,7 @@ adjustJoinTreeList(Query *parsetree, bool removert,
> int rt_index)
> > if (IsA(rtr, RangeTblRef) &&
> > rtr->rtindex == rt_index)
> > {
> > - newjointree =
> list_delete_ptr(newjointree, rtr);
> > + newjointree =
> > + list_delete_cell(newjointree, l);
>
> I think you may as well just use newjointree =
> foreach_delete_current(newjointree, l);. The comment about why the
> list_delete is ok inside a foreach is then irrelevant since
> foreach_delete_current() is designed for deleting the current foreach cell.
>
> Looking around for other places I found two more in equivclass.c.
> These two do require an additional moving part to keep track of the index
> we want to delete, so they're not quite as clear cut a win to do. However,
> I don't think tracking the index makes the code overly complex, so I'm
> thinking they're both fine to do. Does anyone think differently?
>
> Updated patch attached.
>
Thanks for reviewing the patch!
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.

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.

Best regards,
houzj