I was rebasing a patch which requires me to make some changes in
get_cheapest_group_keys_order(). I noticed a few things in there that
I think we could do a little better on:
* The code uses pfree() on a list and it should be using list_free()
* There's a manually coded for loop over a list which seems to be done
so we can skip the first n elements of the list. for_each_from()
should be used for that.
* I think list_truncate(list_copy(list), n) is a pretty bad way to
copy the first n elements of a list, especially when n is likely to be
0 most of the time. I think we should just add a function called
list_copy_head(). We already have list_copy_tail().
* We could reduce some of the branching in the while loop and just set
cheapest_sort_cost to DBL_MAX to save having to check if we're doing
the first loop.
I think the first 3 are worth fixing in PG15 since all that code is
new to that version. The 4th, I'm so sure about.
Does anyone else have any thoughts?
David
diff --git a/src/backend/nodes/list.c b/src/backend/nodes/list.c
index 9d8f4fd5c7..b969a52dd6 100644
--- a/src/backend/nodes/list.c
+++ b/src/backend/nodes/list.c
@@ -1584,6 +1584,27 @@ list_copy(const List *oldlist)
return newlist;
}
+/*
+ * Return a shallow copy of the specified list containing only the first 'len'
+ * elements. If oldlist is shorter than 'len' then we copy the entire list.
+ */
+List *
+list_copy_head(const List *oldlist, int len)
+{
+ List *newlist;
+
+ len = Min(oldlist->length, len);
+
+ if (len <= 0)
+ return NIL;
+
+ newlist = new_list(oldlist->type, len);
+ memcpy(newlist->elements, oldlist->elements, len * sizeof(ListCell));
+
+ check_list_invariants(newlist);
+ return newlist;
+}
+
/*
* Return a shallow copy of the specified list, without the first N elements.
*/
diff --git a/src/backend/optimizer/path/pathkeys.c
b/src/backend/optimizer/path/pathkeys.c
index 9775c4a722..849abbaaba 100644
--- a/src/backend/optimizer/path/pathkeys.c
+++ b/src/backend/optimizer/path/pathkeys.c
@@ -17,6 +17,8 @@
*/
#include "postgres.h"
+#include <float.h>
+
#include "miscadmin.h"
#include "access/stratnum.h"
#include "catalog/pg_opfamily.h"
@@ -591,7 +593,7 @@ get_cheapest_group_keys_order(PlannerInfo *root, double
nrows,
ListCell *cell;
PathkeyMutatorState mstate;
- double cheapest_sort_cost = -1.0;
+ double cheapest_sort_cost = DBL_MAX;
int nFreeKeys;
int nToPermute;
@@ -620,23 +622,23 @@ get_cheapest_group_keys_order(PlannerInfo *root, double
nrows,
nToPermute = 4;
if (nFreeKeys > nToPermute)
{
- int i;
PathkeySortCost *costs = palloc(sizeof(PathkeySortCost) *
nFreeKeys);
+ PathkeySortCost *cost = costs;
- /* skip the pre-ordered pathkeys */
- cell = list_nth_cell(*group_pathkeys, n_preordered);
-
- /* estimate cost for sorting individual pathkeys */
- for (i = 0; cell != NULL; i++, (cell = lnext(*group_pathkeys,
cell)))
+ /*
+ * Estimate cost for sorting individual pathkeys skipping the
+ * pre-ordered pathkeys.
+ */
+ for_each_from(cell, *group_pathkeys, n_preordered)
{
- List *to_cost = list_make1(lfirst(cell));
-
- Assert(i < nFreeKeys);
+ PathKey *pathkey = (PathKey *) lfirst(cell);
+ List *to_cost = list_make1(pathkey);
- costs[i].pathkey = lfirst(cell);
- costs[i].cost = cost_sort_estimate(root, to_cost, 0,
nrows);
+ cost->pathkey = pathkey;
+ cost->cost = cost_sort_estimate(root, to_cost, 0,
nrows);
+ cost++;
- pfree(to_cost);
+ list_free(to_cost);
}
/* sort the pathkeys by sort cost in ascending order */
@@ -646,9 +648,9 @@ get_cheapest_group_keys_order(PlannerInfo *root, double
nrows,
* Rebuild the list of pathkeys - first the preordered ones,
then the
* rest ordered by cost.
*/
- new_group_pathkeys = list_truncate(list_copy(*group_pathkeys),
n_preordered);
+ new_group_pathkeys = list_copy_head(*group_pathkeys,
n_preordered);
- for (i = 0; i < nFreeKeys; i++)
+ for (int i = 0; i < nFreeKeys; i++)
new_group_pathkeys = lappend(new_group_pathkeys,
costs[i].pathkey);
pfree(costs);
@@ -689,7 +691,7 @@ get_cheapest_group_keys_order(PlannerInfo *root, double
nrows,
cost = cost_sort_estimate(root, var_group_pathkeys,
n_preordered, nrows);
- if (cost < cheapest_sort_cost || cheapest_sort_cost < 0)
+ if (cost < cheapest_sort_cost)
{
list_free(new_group_pathkeys);
new_group_pathkeys = list_copy(var_group_pathkeys);
diff --git a/src/include/nodes/pg_list.h b/src/include/nodes/pg_list.h
index 66e70263b2..a6e04d04a3 100644
--- a/src/include/nodes/pg_list.h
+++ b/src/include/nodes/pg_list.h
@@ -620,6 +620,7 @@ extern void list_free(List *list);
extern void list_free_deep(List *list);
extern pg_nodiscard List *list_copy(const List *list);
+extern pg_nodiscard List *list_copy_head(const List *oldlist, int len);
extern pg_nodiscard List *list_copy_tail(const List *list, int nskip);
extern pg_nodiscard List *list_copy_deep(const List *oldlist);
