Tidy up code in get_cheapest_group_keys_order()

david-rowley · david-rowley · commit 4cc832f94a58 · 2022-07-13T14:02:20.000+12:00
There are a few things that we could do a little better within get_cheapest_group_keys_order(): 1. We should be using list_free() rather than pfree() on a List. 2. We should use for_each_from() instead of manually coding a for loop to skip the first n elements of a List 3. list_truncate(list_copy(...), n) is not a great way to copy the first n elements of a list. Let's invent list_copy_head() for that. That way we don't need to copy the entire list just to truncate it directly afterwards. 4. We can simplify finding the cheapest cost by setting the cheapest cost variable to DBL_MAX. That allows us to skip special-casing the initial iteration of the loop. Author: David Rowley Discussion: https://postgr.es/m/CAApHDvrGyL3ft8waEkncG9y5HDMu5TFFJB1paoTC8zi9YK97Nw@mail.gmail.com Backpatch-through: 15, where get_cheapest_group_keys_order was added.
diff --git 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
@@ -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
@@ -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);
 
