Improve list manipulation in several places
There was discussion in [1]/messages/by-id/CAMbWs49aakL=PP7NcTajCtDyaVUE-NMVMGpaLEKreYbQknkQWA@mail.gmail.com about improvements to list manipulation in
several places. But since the discussion is not related to the topic in
that thread, fork a new thread here and attach a patch to show my
thoughts.
Some are just cosmetic changes by using macros. The others should have
performance gain from the avoidance of moving list entries. But I doubt
the performance gain can be noticed or measured, as currently there are
only a few places affected by the change. I still think the changes are
worthwhile though, because it is very likely that future usage of the
same scenario can benefit from these changes.
(Copying in David and Ranier. Ranier provided a patch about the changes
in list.c, but I'm not using that one.)
[1]: /messages/by-id/CAMbWs49aakL=PP7NcTajCtDyaVUE-NMVMGpaLEKreYbQknkQWA@mail.gmail.com
/messages/by-id/CAMbWs49aakL=PP7NcTajCtDyaVUE-NMVMGpaLEKreYbQknkQWA@mail.gmail.com
Thanks
Richard
Attachments:
v1-0001-Improve-list-manipulation-in-several-places.patchapplication/octet-stream; name=v1-0001-Improve-list-manipulation-in-several-places.patchDownload
From e382a3a1f789e2b799e0e6fdfdd49c7657d736ed Mon Sep 17 00:00:00 2001
From: Richard Guo <guofenglinux@gmail.com>
Date: Fri, 21 Apr 2023 14:36:59 +0800
Subject: [PATCH v1] Improve list manipulation in several places
---
src/backend/commands/extension.c | 2 +-
src/backend/nodes/list.c | 86 +++++++++++++++++++
src/backend/optimizer/path/joinpath.c | 6 +-
.../replication/logical/applyparallelworker.c | 2 +-
src/backend/rewrite/rewriteSearchCycle.c | 2 +-
src/backend/utils/adt/ruleutils.c | 2 +-
src/include/nodes/pg_list.h | 14 +++
7 files changed, 106 insertions(+), 8 deletions(-)
diff --git a/src/backend/commands/extension.c b/src/backend/commands/extension.c
index 0eabe18335..1471725f24 100644
--- a/src/backend/commands/extension.c
+++ b/src/backend/commands/extension.c
@@ -1710,7 +1710,7 @@ get_required_extension(char *reqExtensionName,
reqExtensionName)));
/* Add current extension to list of parents to pass down. */
- cascade_parents = lappend(list_copy(parents), extensionName);
+ cascade_parents = lappend_copy(parents, extensionName);
/*
* Create the required extension. We propagate the SCHEMA option
diff --git a/src/backend/nodes/list.c b/src/backend/nodes/list.c
index 750ee5a7e5..7ca3dc7aa6 100644
--- a/src/backend/nodes/list.c
+++ b/src/backend/nodes/list.c
@@ -349,6 +349,36 @@ lappend(List *list, void *datum)
return list;
}
+/*
+ * Form a new list by appending a new element to a shallow copy of the
+ * specified list.
+ *
+ * The input list is not modified.
+ *
+ * This is equivalent to, but more efficient than,
+ * lappend(list_copy(list), datum).
+ */
+List *
+lappend_copy(List *list, void *datum)
+{
+ List *result;
+
+ Assert(IsPointerList(list));
+
+ if (list == NIL)
+ result = new_list(T_List, 1);
+ else
+ {
+ result = new_list(list->type, list->length + 1);
+ memcpy(result->elements, list->elements,
+ list->length * sizeof(ListCell));
+ }
+
+ llast(result) = datum;
+ check_list_invariants(result);
+ return result;
+}
+
/*
* Append an integer to the specified list. See lappend()
*/
@@ -505,6 +535,36 @@ lcons(void *datum, List *list)
return list;
}
+/*
+ * Form a new list by prepending a new element to a shallow copy of the
+ * specified list.
+ *
+ * The input list is not modified.
+ *
+ * This is equivalent to, but more efficient than,
+ * lcons(datum, list_copy(list)).
+ */
+List *
+lcons_copy(void *datum, List *list)
+{
+ List *result;
+
+ Assert(IsPointerList(list));
+
+ if (list == NIL)
+ result = new_list(T_List, 1);
+ else
+ {
+ result = new_list(list->type, list->length + 1);
+ memcpy(result->elements + 1, list->elements,
+ list->length * sizeof(ListCell));
+ }
+
+ linitial(result) = datum;
+ check_list_invariants(result);
+ return result;
+}
+
/*
* Prepend an integer to the list. See lcons()
*/
@@ -1654,6 +1714,32 @@ list_copy_deep(const List *oldlist)
return newlist;
}
+/*
+ * Return a shallow copy of the specified list with nth element being moved to
+ * the head.
+ */
+List *
+list_copy_move_nth_to_head(const List *oldlist, int n)
+{
+ List *newlist;
+
+ if (oldlist == NIL)
+ return NIL;
+
+ Assert(n >= 0 && n < oldlist->length);
+
+ newlist = new_list(oldlist->type, oldlist->length);
+
+ newlist->elements[0] = oldlist->elements[n];
+ memcpy(newlist->elements + 1, oldlist->elements,
+ n * sizeof(ListCell));
+ memcpy(newlist->elements + 1 + n, oldlist->elements + 1 + n,
+ (newlist->length - 1 - n) * sizeof(ListCell));
+
+ check_list_invariants(newlist);
+ return newlist;
+}
+
/*
* Sort a list according to the specified comparator function.
*
diff --git a/src/backend/optimizer/path/joinpath.c b/src/backend/optimizer/path/joinpath.c
index cd80e61fd7..98ede13c99 100644
--- a/src/backend/optimizer/path/joinpath.c
+++ b/src/backend/optimizer/path/joinpath.c
@@ -1317,7 +1317,6 @@ sort_inner_and_outer(PlannerInfo *root,
foreach(l, all_pathkeys)
{
- PathKey *front_pathkey = (PathKey *) lfirst(l);
List *cur_mergeclauses;
List *outerkeys;
List *innerkeys;
@@ -1325,9 +1324,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_nth_cell(list_copy(all_pathkeys),
- foreach_current_index(l)));
+ outerkeys = list_copy_move_nth_to_head(all_pathkeys,
+ foreach_current_index(l));
else
outerkeys = all_pathkeys; /* no work at first one... */
diff --git a/src/backend/replication/logical/applyparallelworker.c b/src/backend/replication/logical/applyparallelworker.c
index 4518683779..45935070d4 100644
--- a/src/backend/replication/logical/applyparallelworker.c
+++ b/src/backend/replication/logical/applyparallelworker.c
@@ -1473,7 +1473,7 @@ pa_stream_abort(LogicalRepStreamAbortData *abort_data)
*/
for (i = list_length(subxactlist) - 1; i >= 0; i--)
{
- TransactionId xid_tmp = lfirst_xid(list_nth_cell(subxactlist, i));
+ TransactionId xid_tmp = list_nth_xid(subxactlist, i);
if (xid_tmp == subxid)
{
diff --git a/src/backend/rewrite/rewriteSearchCycle.c b/src/backend/rewrite/rewriteSearchCycle.c
index b7c8e06fa2..428a98ef2b 100644
--- a/src/backend/rewrite/rewriteSearchCycle.c
+++ b/src/backend/rewrite/rewriteSearchCycle.c
@@ -523,7 +523,7 @@ rewriteSearchAndCycle(CommonTableExpr *cte)
fexpr = makeFuncExpr(F_INT8INC, INT8OID, list_make1(fs), InvalidOid, InvalidOid, COERCE_EXPLICIT_CALL);
- lfirst(list_head(search_col_rowexpr->args)) = fexpr;
+ linitial(search_col_rowexpr->args) = fexpr;
texpr = (Expr *) search_col_rowexpr;
}
diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index 461735e84f..c1231db8c8 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -5417,7 +5417,7 @@ get_query_def(Query *query, StringInfo buf, List *parentnamespace,
AcquireRewriteLocks(query, false, false);
context.buf = buf;
- context.namespaces = lcons(&dpns, list_copy(parentnamespace));
+ context.namespaces = lcons_copy(&dpns, parentnamespace);
context.windowClause = NIL;
context.windowTList = NIL;
context.varprefix = (parentnamespace != NIL ||
diff --git a/src/include/nodes/pg_list.h b/src/include/nodes/pg_list.h
index 529a382d28..0cf31df508 100644
--- a/src/include/nodes/pg_list.h
+++ b/src/include/nodes/pg_list.h
@@ -324,6 +324,17 @@ list_nth_oid(const List *list, int n)
return lfirst_oid(list_nth_cell(list, n));
}
+/*
+ * Return the Xid value contained in the n'th element of the specified
+ * list.
+ */
+static inline TransactionId
+list_nth_xid(const List *list, int n)
+{
+ Assert(IsA(list, XidList));
+ return lfirst_xid(list_nth_cell(list, n));
+}
+
#define list_nth_node(type,list,n) castNode(type, list_nth(list, n))
/*
@@ -558,6 +569,7 @@ extern List *list_make5_impl(NodeTag t, ListCell datum1, ListCell datum2,
ListCell datum5);
extern pg_nodiscard List *lappend(List *list, void *datum);
+extern pg_nodiscard List *lappend_copy(List *list, void *datum);
extern pg_nodiscard List *lappend_int(List *list, int datum);
extern pg_nodiscard List *lappend_oid(List *list, Oid datum);
extern pg_nodiscard List *lappend_xid(List *list, TransactionId datum);
@@ -567,6 +579,7 @@ extern pg_nodiscard List *list_insert_nth_int(List *list, int pos, int datum);
extern pg_nodiscard List *list_insert_nth_oid(List *list, int pos, Oid datum);
extern pg_nodiscard List *lcons(void *datum, List *list);
+extern pg_nodiscard List *lcons_copy(void *datum, List *list);
extern pg_nodiscard List *lcons_int(int datum, List *list);
extern pg_nodiscard List *lcons_oid(Oid datum, List *list);
@@ -625,6 +638,7 @@ extern pg_nodiscard List *list_copy(const List *oldlist);
extern pg_nodiscard List *list_copy_head(const List *oldlist, int len);
extern pg_nodiscard List *list_copy_tail(const List *oldlist, int nskip);
extern pg_nodiscard List *list_copy_deep(const List *oldlist);
+extern pg_nodiscard List *list_copy_move_nth_to_head(const List *oldlist, int n);
typedef int (*list_sort_comparator) (const ListCell *a, const ListCell *b);
extern void list_sort(List *list, list_sort_comparator cmp);
--
2.31.0
Em sex., 21 de abr. de 2023 às 04:34, Richard Guo <guofenglinux@gmail.com>
escreveu:
There was discussion in [1] about improvements to list manipulation in
several places. But since the discussion is not related to the topic in
that thread, fork a new thread here and attach a patch to show my
thoughts.Some are just cosmetic changes by using macros. The others should have
performance gain from the avoidance of moving list entries. But I doubt
the performance gain can be noticed or measured, as currently there are
only a few places affected by the change. I still think the changes are
worthwhile though, because it is very likely that future usage of the
same scenario can benefit from these changes.
+1
Perhaps list_delete_nth_cell needs to check NIL too?
+ if (list == NIL)
+ return NIL;+lcons_copy(void *datum, const List *list)
+lappend_copy(const List *list, void *datum)
list param pointer can be const here not?regards,
Ranier Vilela
On Fri, 21 Apr 2023 at 23:16, Ranier Vilela <ranier.vf@gmail.com> wrote:
Perhaps list_delete_nth_cell needs to check NIL too? + if (list == NIL) + return NIL;
Which cell would you be deleting from an empty list?
David
Em sex, 21 de abr de 2023 9:10 AM, David Rowley <dgrowleyml@gmail.com>
escreveu:
On Fri, 21 Apr 2023 at 23:16, Ranier Vilela <ranier.vf@gmail.com> wrote:
Perhaps list_delete_nth_cell needs to check NIL too? + if (list == NIL) + return NIL;Which cell would you be deleting from an empty list?
None.
But list_delete_nth_cel can checks a length of NIL list.
Perhaps a assert?
regards,
Ranier Vilela
On 21.04.23 09:34, Richard Guo wrote:
There was discussion in [1] about improvements to list manipulation in
several places. But since the discussion is not related to the topic in
that thread, fork a new thread here and attach a patch to show my
thoughts.Some are just cosmetic changes by using macros. The others should have
performance gain from the avoidance of moving list entries. But I doubt
the performance gain can be noticed or measured, as currently there are
only a few places affected by the change. I still think the changes are
worthwhile though, because it is very likely that future usage of the
same scenario can benefit from these changes.
Can you explain the changes?
Maybe this patch should be split up. It seems some of the changes are
trivial simplifications using existing APIs, while others introduce new
functions.
On Sat, Apr 22, 2023 at 12:55 AM Peter Eisentraut <
peter.eisentraut@enterprisedb.com> wrote:
On 21.04.23 09:34, Richard Guo wrote:
There was discussion in [1] about improvements to list manipulation in
several places. But since the discussion is not related to the topic in
that thread, fork a new thread here and attach a patch to show my
thoughts.Some are just cosmetic changes by using macros. The others should have
performance gain from the avoidance of moving list entries. But I doubt
the performance gain can be noticed or measured, as currently there are
only a few places affected by the change. I still think the changes are
worthwhile though, because it is very likely that future usage of the
same scenario can benefit from these changes.Can you explain the changes?
Maybe this patch should be split up. It seems some of the changes are
trivial simplifications using existing APIs, while others introduce new
functions.
Thanks for the suggestion. I've split the patch into two as attached.
0001 is just a minor simplification by replacing lfirst(list_head(list))
with linitial(list). 0002 introduces new functions to reduce the
movement of list elements in several places so as to gain performance
improvement and benefit future callers.
Thanks
Richard
Attachments:
v2-0002-Improve-list-manipulation-in-several-places.patchapplication/octet-stream; name=v2-0002-Improve-list-manipulation-in-several-places.patchDownload
From 08fd0abbb9f4f44ea2a2bf06c9b1524c176ec6ca Mon Sep 17 00:00:00 2001
From: Richard Guo <guofenglinux@gmail.com>
Date: Sun, 23 Apr 2023 14:14:39 +0800
Subject: [PATCH v2 2/2] Improve list manipulation in several places
In several places where we manipulate List in such a way as
lappend(list_copy(list), datum), or lcons(datum, list_copy(list)), we can
improve their performance by reducing the movement of list elements. This
patch introduces new functions to do that, which may also benefit future
callers.
---
src/backend/commands/extension.c | 2 +-
src/backend/nodes/list.c | 86 +++++++++++++++++++
src/backend/optimizer/path/joinpath.c | 6 +-
.../replication/logical/applyparallelworker.c | 2 +-
src/backend/utils/adt/ruleutils.c | 2 +-
src/include/nodes/pg_list.h | 14 +++
6 files changed, 105 insertions(+), 7 deletions(-)
diff --git a/src/backend/commands/extension.c b/src/backend/commands/extension.c
index 0eabe18335..1471725f24 100644
--- a/src/backend/commands/extension.c
+++ b/src/backend/commands/extension.c
@@ -1710,7 +1710,7 @@ get_required_extension(char *reqExtensionName,
reqExtensionName)));
/* Add current extension to list of parents to pass down. */
- cascade_parents = lappend(list_copy(parents), extensionName);
+ cascade_parents = lappend_copy(parents, extensionName);
/*
* Create the required extension. We propagate the SCHEMA option
diff --git a/src/backend/nodes/list.c b/src/backend/nodes/list.c
index 750ee5a7e5..0714e34873 100644
--- a/src/backend/nodes/list.c
+++ b/src/backend/nodes/list.c
@@ -349,6 +349,36 @@ lappend(List *list, void *datum)
return list;
}
+/*
+ * Form a new list by appending a new element to a shallow copy of the
+ * specified list.
+ *
+ * The input list is not modified.
+ *
+ * This is equivalent to, but more efficient than,
+ * lappend(list_copy(list), datum).
+ */
+List *
+lappend_copy(const List *list, void *datum)
+{
+ List *result;
+
+ Assert(IsPointerList(list));
+
+ if (list == NIL)
+ result = new_list(T_List, 1);
+ else
+ {
+ result = new_list(list->type, list->length + 1);
+ memcpy(result->elements, list->elements,
+ list->length * sizeof(ListCell));
+ }
+
+ llast(result) = datum;
+ check_list_invariants(result);
+ return result;
+}
+
/*
* Append an integer to the specified list. See lappend()
*/
@@ -505,6 +535,36 @@ lcons(void *datum, List *list)
return list;
}
+/*
+ * Form a new list by prepending a new element to a shallow copy of the
+ * specified list.
+ *
+ * The input list is not modified.
+ *
+ * This is equivalent to, but more efficient than,
+ * lcons(datum, list_copy(list)).
+ */
+List *
+lcons_copy(void *datum, const List *list)
+{
+ List *result;
+
+ Assert(IsPointerList(list));
+
+ if (list == NIL)
+ result = new_list(T_List, 1);
+ else
+ {
+ result = new_list(list->type, list->length + 1);
+ memcpy(result->elements + 1, list->elements,
+ list->length * sizeof(ListCell));
+ }
+
+ linitial(result) = datum;
+ check_list_invariants(result);
+ return result;
+}
+
/*
* Prepend an integer to the list. See lcons()
*/
@@ -1654,6 +1714,32 @@ list_copy_deep(const List *oldlist)
return newlist;
}
+/*
+ * Return a shallow copy of the specified list with nth element being moved to
+ * the head.
+ */
+List *
+list_copy_move_nth_to_head(const List *oldlist, int n)
+{
+ List *newlist;
+
+ if (oldlist == NIL)
+ return NIL;
+
+ Assert(n >= 0 && n < oldlist->length);
+
+ newlist = new_list(oldlist->type, oldlist->length);
+
+ newlist->elements[0] = oldlist->elements[n];
+ memcpy(newlist->elements + 1, oldlist->elements,
+ n * sizeof(ListCell));
+ memcpy(newlist->elements + 1 + n, oldlist->elements + 1 + n,
+ (newlist->length - 1 - n) * sizeof(ListCell));
+
+ check_list_invariants(newlist);
+ return newlist;
+}
+
/*
* Sort a list according to the specified comparator function.
*
diff --git a/src/backend/optimizer/path/joinpath.c b/src/backend/optimizer/path/joinpath.c
index cd80e61fd7..98ede13c99 100644
--- a/src/backend/optimizer/path/joinpath.c
+++ b/src/backend/optimizer/path/joinpath.c
@@ -1317,7 +1317,6 @@ sort_inner_and_outer(PlannerInfo *root,
foreach(l, all_pathkeys)
{
- PathKey *front_pathkey = (PathKey *) lfirst(l);
List *cur_mergeclauses;
List *outerkeys;
List *innerkeys;
@@ -1325,9 +1324,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_nth_cell(list_copy(all_pathkeys),
- foreach_current_index(l)));
+ outerkeys = list_copy_move_nth_to_head(all_pathkeys,
+ foreach_current_index(l));
else
outerkeys = all_pathkeys; /* no work at first one... */
diff --git a/src/backend/replication/logical/applyparallelworker.c b/src/backend/replication/logical/applyparallelworker.c
index 4518683779..45935070d4 100644
--- a/src/backend/replication/logical/applyparallelworker.c
+++ b/src/backend/replication/logical/applyparallelworker.c
@@ -1473,7 +1473,7 @@ pa_stream_abort(LogicalRepStreamAbortData *abort_data)
*/
for (i = list_length(subxactlist) - 1; i >= 0; i--)
{
- TransactionId xid_tmp = lfirst_xid(list_nth_cell(subxactlist, i));
+ TransactionId xid_tmp = list_nth_xid(subxactlist, i);
if (xid_tmp == subxid)
{
diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index 461735e84f..c1231db8c8 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -5417,7 +5417,7 @@ get_query_def(Query *query, StringInfo buf, List *parentnamespace,
AcquireRewriteLocks(query, false, false);
context.buf = buf;
- context.namespaces = lcons(&dpns, list_copy(parentnamespace));
+ context.namespaces = lcons_copy(&dpns, parentnamespace);
context.windowClause = NIL;
context.windowTList = NIL;
context.varprefix = (parentnamespace != NIL ||
diff --git a/src/include/nodes/pg_list.h b/src/include/nodes/pg_list.h
index 529a382d28..b8bcdec2ce 100644
--- a/src/include/nodes/pg_list.h
+++ b/src/include/nodes/pg_list.h
@@ -324,6 +324,17 @@ list_nth_oid(const List *list, int n)
return lfirst_oid(list_nth_cell(list, n));
}
+/*
+ * Return the Xid value contained in the n'th element of the specified
+ * list.
+ */
+static inline TransactionId
+list_nth_xid(const List *list, int n)
+{
+ Assert(IsA(list, XidList));
+ return lfirst_xid(list_nth_cell(list, n));
+}
+
#define list_nth_node(type,list,n) castNode(type, list_nth(list, n))
/*
@@ -558,6 +569,7 @@ extern List *list_make5_impl(NodeTag t, ListCell datum1, ListCell datum2,
ListCell datum5);
extern pg_nodiscard List *lappend(List *list, void *datum);
+extern pg_nodiscard List *lappend_copy(const List *list, void *datum);
extern pg_nodiscard List *lappend_int(List *list, int datum);
extern pg_nodiscard List *lappend_oid(List *list, Oid datum);
extern pg_nodiscard List *lappend_xid(List *list, TransactionId datum);
@@ -567,6 +579,7 @@ extern pg_nodiscard List *list_insert_nth_int(List *list, int pos, int datum);
extern pg_nodiscard List *list_insert_nth_oid(List *list, int pos, Oid datum);
extern pg_nodiscard List *lcons(void *datum, List *list);
+extern pg_nodiscard List *lcons_copy(void *datum, const List *list);
extern pg_nodiscard List *lcons_int(int datum, List *list);
extern pg_nodiscard List *lcons_oid(Oid datum, List *list);
@@ -625,6 +638,7 @@ extern pg_nodiscard List *list_copy(const List *oldlist);
extern pg_nodiscard List *list_copy_head(const List *oldlist, int len);
extern pg_nodiscard List *list_copy_tail(const List *oldlist, int nskip);
extern pg_nodiscard List *list_copy_deep(const List *oldlist);
+extern pg_nodiscard List *list_copy_move_nth_to_head(const List *oldlist, int n);
typedef int (*list_sort_comparator) (const ListCell *a, const ListCell *b);
extern void list_sort(List *list, list_sort_comparator cmp);
--
2.31.0
v2-0001-A-minor-simplification-for-List-manipulation.patchapplication/octet-stream; name=v2-0001-A-minor-simplification-for-List-manipulation.patchDownload
From 37de0c900eae302cf9e20dc7bb8174a9c6363b0c Mon Sep 17 00:00:00 2001
From: Richard Guo <guofenglinux@gmail.com>
Date: Sun, 23 Apr 2023 13:45:45 +0800
Subject: [PATCH v2 1/2] A minor simplification for List manipulation
Fix one place that was using lfirst(list_head(list)) by using linitial(list)
instead. They are equivalent but the latter is simpler. We did the same in
9d299a49.
---
src/backend/rewrite/rewriteSearchCycle.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/backend/rewrite/rewriteSearchCycle.c b/src/backend/rewrite/rewriteSearchCycle.c
index b7c8e06fa2..428a98ef2b 100644
--- a/src/backend/rewrite/rewriteSearchCycle.c
+++ b/src/backend/rewrite/rewriteSearchCycle.c
@@ -523,7 +523,7 @@ rewriteSearchAndCycle(CommonTableExpr *cte)
fexpr = makeFuncExpr(F_INT8INC, INT8OID, list_make1(fs), InvalidOid, InvalidOid, COERCE_EXPLICIT_CALL);
- lfirst(list_head(search_col_rowexpr->args)) = fexpr;
+ linitial(search_col_rowexpr->args) = fexpr;
texpr = (Expr *) search_col_rowexpr;
}
--
2.31.0
On Fri, Apr 21, 2023 at 7:16 PM Ranier Vilela <ranier.vf@gmail.com> wrote:
+lcons_copy(void *datum, const List *list) +lappend_copy(const List *list, void *datum) list param pointer can be const here not?
Correct. Good point. V2 patch does that.
Thanks
Richard
Richard Guo <guofenglinux@gmail.com> 于2023年4月21日周五 15:35写道：
There was discussion in [1] about improvements to list manipulation in
several places. But since the discussion is not related to the topic in
that thread, fork a new thread here and attach a patch to show my
thoughts.Some are just cosmetic changes by using macros. The others should have
performance gain from the avoidance of moving list entries. But I doubt
the performance gain can be noticed or measured, as currently there are
only a few places affected by the change. I still think the changes are
worthwhile though, because it is very likely that future usage of the
same scenario can benefit from these changes.
I doubt the performance gain from lappend_copy func. new_tail_cell in
lappend may not enter enlarge_list in most cases, because we
may allocate extra cells in new_list(see the comment in new_list).
Show quoted text
(Copying in David and Ranier. Ranier provided a patch about the changes
in list.c, but I'm not using that one.)[1]
/messages/by-id/CAMbWs49aakL=PP7NcTajCtDyaVUE-NMVMGpaLEKreYbQknkQWA@mail.gmail.comThanks
Richard
On 23.04.23 08:42, Richard Guo wrote:
Thanks for the suggestion. I've split the patch into two as attached.
0001 is just a minor simplification by replacing lfirst(list_head(list))
with linitial(list). 0002 introduces new functions to reduce the
movement of list elements in several places so as to gain performance
improvement and benefit future callers.
These look sensible to me. If you could show some numbers that support
the claim that there is a performance advantage, it would be even more
convincing.
On 2023-May-08, Peter Eisentraut wrote:
On 23.04.23 08:42, Richard Guo wrote:
Thanks for the suggestion. I've split the patch into two as attached.
0001 is just a minor simplification by replacing lfirst(list_head(list))
with linitial(list). 0002 introduces new functions to reduce the
movement of list elements in several places so as to gain performance
improvement and benefit future callers.These look sensible to me. If you could show some numbers that support the
claim that there is a performance advantage, it would be even more
convincing.
0001 looks fine.
The problem I see is that each of these new functions has a single
caller, and the only one that looks like it could have a performance
advantage is list_copy_move_nth_to_head() (which is the weirdest of the
lot). I'm inclined not to have any of these single-use functions unless
a performance case can be made for them.
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
Em seg., 8 de mai. de 2023 às 14:26, Alvaro Herrera <alvherre@alvh.no-ip.org>
escreveu:
On 2023-May-08, Peter Eisentraut wrote:
On 23.04.23 08:42, Richard Guo wrote:
Thanks for the suggestion. I've split the patch into two as attached.
0001 is just a minor simplification by replacinglfirst(list_head(list))
with linitial(list). 0002 introduces new functions to reduce the
movement of list elements in several places so as to gain performance
improvement and benefit future callers.These look sensible to me. If you could show some numbers that support
the
claim that there is a performance advantage, it would be even more
convincing.0001 looks fine.
The problem I see is that each of these new functions has a single
caller, and the only one that looks like it could have a performance
advantage is list_copy_move_nth_to_head() (which is the weirdest of the
lot). I'm inclined not to have any of these single-use functions unless
a performance case can be made for them.
I think you missed list_nth_xid, It makes perfect sense to exist.
regards,
Ranier Vilela
On Mon, May 8, 2023 at 11:22 PM Peter Eisentraut <
peter.eisentraut@enterprisedb.com> wrote:
On 23.04.23 08:42, Richard Guo wrote:
Thanks for the suggestion. I've split the patch into two as attached.
0001 is just a minor simplification by replacing lfirst(list_head(list))
with linitial(list). 0002 introduces new functions to reduce the
movement of list elements in several places so as to gain performance
improvement and benefit future callers.These look sensible to me. If you could show some numbers that support
the claim that there is a performance advantage, it would be even more
convincing.
Thanks Peter for looking at those patches. I tried to devise a query to
show performance gain but did not succeed :-(. So I begin to wonder if
0002 is worthwhile to do, as it seems that it does not solve any real
problem.
Thanks
Richard
On Tue, May 9, 2023 at 1:26 AM Alvaro Herrera <alvherre@alvh.no-ip.org>
wrote:
The problem I see is that each of these new functions has a single
caller, and the only one that looks like it could have a performance
advantage is list_copy_move_nth_to_head() (which is the weirdest of the
lot). I'm inclined not to have any of these single-use functions unless
a performance case can be made for them.
Yeah, maybe this is the reason I failed to devise a query that shows any
performance gain. I tried with a query which makes the 'all_pathkeys'
in sort_inner_and_outer being length of 500 and still cannot see any
notable performance improvements gained by list_copy_move_nth_to_head.
Maybe the cost of other parts of planning swamps the performance gain
here? Now I agree that maybe 0002 is not worthwhile to do.
Thanks
Richard
On Tue, May 9, 2023 at 1:48 AM Ranier Vilela <ranier.vf@gmail.com> wrote:
I think you missed list_nth_xid, It makes perfect sense to exist.
It seems that list_nth_xid is more about simplification. So maybe we
should put it in 0001?
Thanks
Richard
On 2023-May-08, Ranier Vilela wrote:
Em seg., 8 de mai. de 2023 às 14:26, Alvaro Herrera <alvherre@alvh.no-ip.org>
escreveu:The problem I see is that each of these new functions has a single
caller, and the only one that looks like it could have a performance
advantage is list_copy_move_nth_to_head() (which is the weirdest of the
lot). I'm inclined not to have any of these single-use functions unless
a performance case can be made for them.I think you missed list_nth_xid, It makes perfect sense to exist.
I saw that one; it's just syntactic sugar, just like list_nth_int and
list_nth_oid, except it has only one possible callsite instead of a
dozen like those others. I see no harm in that function, but no
practical advantage to it either. Xid lists are a very fringe feature,
there being exactly one place in the whole server that uses them.
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
On 09.05.23 05:13, Richard Guo wrote:
On Tue, May 9, 2023 at 1:26 AM Alvaro Herrera <alvherre@alvh.no-ip.org
<mailto:alvherre@alvh.no-ip.org>> wrote:The problem I see is that each of these new functions has a single
caller, and the only one that looks like it could have a performance
advantage is list_copy_move_nth_to_head() (which is the weirdest of the
lot). I'm inclined not to have any of these single-use functions unless
a performance case can be made for them.Yeah, maybe this is the reason I failed to devise a query that shows any
performance gain. I tried with a query which makes the 'all_pathkeys'
in sort_inner_and_outer being length of 500 and still cannot see any
notable performance improvements gained by list_copy_move_nth_to_head.
Maybe the cost of other parts of planning swamps the performance gain
here? Now I agree that maybe 0002 is not worthwhile to do.
I have committed patch 0001. Since you have withdrawn 0002, this closes
the commit fest item.
On Mon, Jul 3, 2023 at 5:41 PM Peter Eisentraut <
peter.eisentraut@enterprisedb.com> wrote:
On 09.05.23 05:13, Richard Guo wrote:
Yeah, maybe this is the reason I failed to devise a query that shows any
performance gain. I tried with a query which makes the 'all_pathkeys'
in sort_inner_and_outer being length of 500 and still cannot see any
notable performance improvements gained by list_copy_move_nth_to_head.
Maybe the cost of other parts of planning swamps the performance gain
here? Now I agree that maybe 0002 is not worthwhile to do.I have committed patch 0001. Since you have withdrawn 0002, this closes
the commit fest item.
Thanks for pushing it and closing the item!
Thanks
Richard