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
This commit is contained in:
parent
624e440a47
commit
3cb1b2a880
@ -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;
|
||||
|
||||
for (i = 0; i < len - 1; i++)
|
||||
list_arr[i]->next = list_arr[i + 1];
|
||||
/*
|
||||
* Copy over the data in the first cell; new_list() has already allocated
|
||||
* the head cell itself
|
||||
*/
|
||||
newlist->head->data = list_arr[0]->data;
|
||||
|
||||
list_arr[len - 1]->next = NULL;
|
||||
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;
|
||||
|
||||
newlist_prev = newlist_cur;
|
||||
}
|
||||
|
||||
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;
|
||||
}
|
||||
|
||||
/*
|
||||
|
Loading…
x
Reference in New Issue
Block a user