On Sat, 10 Oct 2020 at 15:45, Hou, Zhijie <[email protected]> wrote:
> 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.
David
diff --git a/src/backend/optimizer/path/equivclass.c
b/src/backend/optimizer/path/equivclass.c
index 823422edad..690b753369 100644
--- a/src/backend/optimizer/path/equivclass.c
+++ b/src/backend/optimizer/path/equivclass.c
@@ -137,6 +137,7 @@ process_equivalence(PlannerInfo *root,
EquivalenceMember *em1,
*em2;
ListCell *lc1;
+ int ec2_idx;
/* Should not already be marked as having generated an eclass */
Assert(restrictinfo->left_ec == NULL);
@@ -258,6 +259,7 @@ process_equivalence(PlannerInfo *root,
*/
ec1 = ec2 = NULL;
em1 = em2 = NULL;
+ ec2_idx = -1;
foreach(lc1, root->eq_classes)
{
EquivalenceClass *cur_ec = (EquivalenceClass *) lfirst(lc1);
@@ -311,6 +313,7 @@ process_equivalence(PlannerInfo *root,
equal(item2, cur_em->em_expr))
{
ec2 = cur_ec;
+ ec2_idx = foreach_current_index(lc1);
em2 = cur_em;
if (ec1)
break;
@@ -371,7 +374,7 @@ process_equivalence(PlannerInfo *root,
ec1->ec_max_security = Max(ec1->ec_max_security,
ec2->ec_max_security);
ec2->ec_merged = ec1;
- root->eq_classes = list_delete_ptr(root->eq_classes, ec2);
+ root->eq_classes = list_delete_nth_cell(root->eq_classes,
ec2_idx);
/* just to avoid debugging confusion w/ dangling pointers: */
ec2->ec_members = NIL;
ec2->ec_sources = NIL;
@@ -1964,6 +1967,7 @@ reconsider_full_join_clause(PlannerInfo *root,
RestrictInfo *rinfo)
bool matchleft;
bool matchright;
ListCell *lc2;
+ int coal_idx = -1;
/* Ignore EC unless it contains pseudoconstants */
if (!cur_ec->ec_has_const)
@@ -2008,6 +2012,7 @@ reconsider_full_join_clause(PlannerInfo *root,
RestrictInfo *rinfo)
if (equal(leftvar, cfirst) && equal(rightvar,
csecond))
{
+ coal_idx = foreach_current_index(lc2);
match = true;
break;
}
@@ -2072,7 +2077,7 @@ reconsider_full_join_clause(PlannerInfo *root,
RestrictInfo *rinfo)
*/
if (matchleft && matchright)
{
- cur_ec->ec_members =
list_delete_ptr(cur_ec->ec_members, coal_em);
+ cur_ec->ec_members =
list_delete_nth_cell(cur_ec->ec_members, coal_idx);
return true;
}
diff --git a/src/backend/optimizer/path/joinpath.c
b/src/backend/optimizer/path/joinpath.c
index db54a6ba2e..4a35903b29 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... */
diff --git a/src/backend/rewrite/rewriteHandler.c
b/src/backend/rewrite/rewriteHandler.c
index fe777c3103..1faaafab08 100644
--- a/src/backend/rewrite/rewriteHandler.c
+++ b/src/backend/rewrite/rewriteHandler.c
@@ -650,11 +650,7 @@ adjustJoinTreeList(Query *parsetree, bool removert, int
rt_index)
if (IsA(rtr, RangeTblRef) &&
rtr->rtindex == rt_index)
{
- newjointree = list_delete_ptr(newjointree, rtr);
-
- /*
- * foreach is safe because we exit loop after
list_delete...
- */
+ newjointree =
foreach_delete_current(newjointree, l);
break;
}
}
