Tidy up code in get_cheapest_group_keys_order()

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
index 9d8f4fd5c7cab5ce75d91dfb386f8577659901e8..b969a52dd6775b9e4626dbefa95d83b62a190290 100644 (file)
--- 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 9775c4a722592c35b5559ad21522357f3dd117fb..985b5d8de9bd97a41b701a2a9b3d64a579ee88b7 100644 (file)
--- 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 66e70263b2f4b84d16d9d7e815a851d683e44278..a6e04d04a3d1578872cd8cf9d81fc90756d2b913 100644 (file)
--- 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);