Rewrite list_qsort() to avoid trashing its input list.

The initial implementation of list_qsort(), from commit ab7271677,
re-used the ListCells of the input list while not touching the List
header.  This meant that anybody who still had a pointer to the
original header would now be in possession of a corrupted list,
a problem that seems sure to bite us eventually.

One possible solution is to re-use the original List header as well,
giving the function the semantics of update-in-place.  However, that
doesn't seem like a very good idea either given the way that the
function is used in the planner: create_path functions aren't normally
supposed to modify their input lists.  It doesn't look like there would
be a problem today, but it's not hard to foresee a time when modifying
a list of Paths in-place could have side-effects on some other append
path.

On the whole, and in view of the likelihood that this function might
be used in other contexts in the future, it seems best to get rid of
the micro-optimization of re-using the input list cells.  Just build
a new list.

Discussion: https://postgr.es/m/16912.1515449066@sss.pgh.pa.us

diff --git a/src/backend/nodes/list.c b/src/backend/nodes/list.c
index 083538f70a13f5aa9d8ce09f5bad488f355ca350..f3e18007086bc4b27c9afd7516ba8cfc520708df 100644 (file)
--- a/src/backend/nodes/list.c
+++ b/src/backend/nodes/list.c
@@ -1250,41 +1250,65 @@ list_copy_tail(const List *oldlist, int nskip)
 }
 
 /*
- * Sort a list using qsort. A sorted list is built but the cells of the
- * original list are re-used.  The comparator function receives arguments of
- * type ListCell **
+ * Sort a list as though by qsort.
+ *
+ * A new list is built and returned.  Like list_copy, this doesn't make
+ * fresh copies of any pointed-to data.
+ *
+ * The comparator function receives arguments of type ListCell **.
  */
 List *
 list_qsort(const List *list, list_qsort_comparator cmp)
 {
-       ListCell   *cell;
-       int                     i;
        int                     len = list_length(list);
        ListCell  **list_arr;
-       List       *new_list;
+       List       *newlist;
+       ListCell   *newlist_prev;
+       ListCell   *cell;
+       int                     i;
 
+       /* Empty list is easy */
        if (len == 0)
                return NIL;
 
+       /* Flatten list cells into an array, so we can use qsort */
+       list_arr = (ListCell **) palloc(sizeof(ListCell *) * len);
        i = 0;
-       list_arr = palloc(sizeof(ListCell *) * len);
        foreach(cell, list)
                list_arr[i++] = cell;
 
        qsort(list_arr, len, sizeof(ListCell *), cmp);
 
-       new_list = (List *) palloc(sizeof(List));
-       new_list->type = list->type;
-       new_list->length = len;
-       new_list->head = list_arr[0];
-       new_list->tail = list_arr[len - 1];
+       /* Construct new list (this code is much like list_copy) */
+       newlist = new_list(list->type);
+       newlist->length = len;
+
+       /*
+        * Copy over the data in the first cell; new_list() has already allocated
+        * the head cell itself
+        */
+       newlist->head->data = list_arr[0]->data;
+
+       newlist_prev = newlist->head;
+       for (i = 1; i < len; i++)
+       {
+               ListCell   *newlist_cur;
+
+               newlist_cur = (ListCell *) palloc(sizeof(*newlist_cur));
+               newlist_cur->data = list_arr[i]->data;
+               newlist_prev->next = newlist_cur;
 
-       for (i = 0; i < len - 1; i++)
-               list_arr[i]->next = list_arr[i + 1];
+               newlist_prev = newlist_cur;
+       }
 
-       list_arr[len - 1]->next = NULL;
+       newlist_prev->next = NULL;
+       newlist->tail = newlist_prev;
+
+       /* Might as well free the workspace array */
        pfree(list_arr);
-       return new_list;
+
+       check_list_invariants(newlist);
+       return newlist;
 }
 
 /*