Redesign the API for list sorting (list_qsort becomes list_sort).

In the wake of commit 1cff1b95, the obvious way to sort a List
is to apply qsort() directly to the array of ListCells.  list_qsort
was building an intermediate array of pointers-to-ListCells, which
we no longer need, but getting rid of it forces an API change:
the comparator functions need to do one less level of indirection.

Since we're having to touch the callers anyway, let's do two additional
changes: sort the given list in-place rather than making a copy (as
none of the existing callers have any use for the copying behavior),
and rename list_qsort to list_sort.  It was argued that the old name
exposes more about the implementation than it should, which I find
pretty questionable, but a better reason to rename it is to be sure
we get the attention of any external callers about the need to fix
their comparator functions.

While we're at it, change four existing callers of qsort() to use
list_sort instead; previously, they all had local reinventions
of list_qsort, ie build-an-array-from-a-List-and-qsort-it.
(There are some other places where changing to list_sort perhaps
would be worthwhile, but they're less obviously wins.)

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

src/backend/nodes/list.c

@@ -1419,45 +1419,28 @@ list_copy_deep(const List *oldlist)
 }
 
 /*
- * Sort a list as though by qsort.
+ * Sort a list according to the specified comparator function.
  *
- * A new list is built and returned.  Like list_copy, this doesn't make
- * fresh copies of any pointed-to data.
+ * The list is sorted in-place.
  *
- * The comparator function receives arguments of type ListCell **.
- * (XXX that's really inefficient now, but changing it seems like
- * material for a standalone patch.)
+ * The comparator function is declared to receive arguments of type
+ * const ListCell *; this allows it to use lfirst() and variants
+ * without casting its arguments.  Otherwise it behaves the same as
+ * the comparator function for standard qsort().
+ *
+ * Like qsort(), this provides no guarantees about sort stability
+ * for equal keys.
  */
-List *
-list_qsort(const List *list, list_qsort_comparator cmp)
+void
+list_sort(List *list, list_sort_comparator cmp)
 {
-	int			len = list_length(list);
-	ListCell  **list_arr;
-	List	   *newlist;
-	ListCell   *cell;
-	int			i;
-
-	/* Empty list is easy */
-	if (len == 0)
-		return NIL;
+	typedef int (*qsort_comparator) (const void *a, const void *b);
+	int			len;
 
-	/* We have to make an array of pointers to satisfy the API */
-	list_arr = (ListCell **) palloc(sizeof(ListCell *) * len);
-	i = 0;
-	foreach(cell, list)
-		list_arr[i++] = cell;
-
-	qsort(list_arr, len, sizeof(ListCell *), cmp);
-
-	/* Construct new list (this code is much like list_copy) */
-	newlist = new_list(list->type, len);
-
-	for (i = 0; i < len; i++)
-		newlist->elements[i] = *list_arr[i];
-
-	/* Might as well free the workspace array */
-	pfree(list_arr);
+	check_list_invariants(list);
 
-	check_list_invariants(newlist);
-	return newlist;
+	/* Nothing to do if there's less than two elements */
+	len = list_length(list);
+	if (len > 1)
+		qsort(list->elements, len, sizeof(ListCell), (qsort_comparator) cmp);
 }

src/backend/optimizer/util/pathnode.c

@@ -52,8 +52,8 @@ typedef enum
 #define STD_FUZZ_FACTOR 1.01
 
 static List *translate_sub_tlist(List *tlist, int relid);
-static int	append_total_cost_compare(const void *a, const void *b);
-static int	append_startup_cost_compare(const void *a, const void *b);
+static int	append_total_cost_compare(const ListCell *a, const ListCell *b);
+static int	append_startup_cost_compare(const ListCell *a, const ListCell *b);
 static List *reparameterize_pathlist_by_child(PlannerInfo *root,
 											  List *pathlist,
 											  RelOptInfo *child_rel);
@@ -1239,9 +1239,8 @@ create_append_path(PlannerInfo *root,
 		 */
 		Assert(pathkeys == NIL);
 
-		subpaths = list_qsort(subpaths, append_total_cost_compare);
-		partial_subpaths = list_qsort(partial_subpaths,
-									  append_startup_cost_compare);
+		list_sort(subpaths, append_total_cost_compare);
+		list_sort(partial_subpaths, append_startup_cost_compare);
 	}
 	pathnode->first_partial_path = list_length(subpaths);
 	pathnode->subpaths = list_concat(subpaths, partial_subpaths);
@@ -1296,17 +1295,18 @@ create_append_path(PlannerInfo *root,
 
 /*
  * append_total_cost_compare
- *	  qsort comparator for sorting append child paths by total_cost descending
+ *	  list_sort comparator for sorting append child paths
+ *	  by total_cost descending
  *
  * For equal total costs, we fall back to comparing startup costs; if those
  * are equal too, break ties using bms_compare on the paths' relids.
- * (This is to avoid getting unpredictable results from qsort.)
+ * (This is to avoid getting unpredictable results from list_sort.)
  */
 static int
-append_total_cost_compare(const void *a, const void *b)
+append_total_cost_compare(const ListCell *a, const ListCell *b)
 {
-	Path	   *path1 = (Path *) lfirst(*(ListCell **) a);
-	Path	   *path2 = (Path *) lfirst(*(ListCell **) b);
+	Path	   *path1 = (Path *) lfirst(a);
+	Path	   *path2 = (Path *) lfirst(b);
 	int			cmp;
 
 	cmp = compare_path_costs(path1, path2, TOTAL_COST);
@@ -1317,17 +1317,18 @@ append_total_cost_compare(const void *a, const void *b)
 
 /*
  * append_startup_cost_compare
- *	  qsort comparator for sorting append child paths by startup_cost descending
+ *	  list_sort comparator for sorting append child paths
+ *	  by startup_cost descending
  *
  * For equal startup costs, we fall back to comparing total costs; if those
  * are equal too, break ties using bms_compare on the paths' relids.
- * (This is to avoid getting unpredictable results from qsort.)
+ * (This is to avoid getting unpredictable results from list_sort.)
  */
 static int
-append_startup_cost_compare(const void *a, const void *b)
+append_startup_cost_compare(const ListCell *a, const ListCell *b)
 {
-	Path	   *path1 = (Path *) lfirst(*(ListCell **) a);
-	Path	   *path2 = (Path *) lfirst(*(ListCell **) b);
+	Path	   *path1 = (Path *) lfirst(a);
+	Path	   *path2 = (Path *) lfirst(b);
 	int			cmp;
 
 	cmp = compare_path_costs(path1, path2, STARTUP_COST);

src/backend/parser/parse_agg.c

@@ -1722,11 +1722,12 @@ expand_groupingset_node(GroupingSet *gs)
 	return result;
 }
 
+/* list_sort comparator to sort sub-lists by length */
 static int
-cmp_list_len_asc(const void *a, const void *b)
+cmp_list_len_asc(const ListCell *a, const ListCell *b)
 {
-	int			la = list_length(*(List *const *) a);
-	int			lb = list_length(*(List *const *) b);
+	int			la = list_length((const List *) lfirst(a));
+	int			lb = list_length((const List *) lfirst(b));
 
 	return (la > lb) ? 1 : (la == lb) ? 0 : -1;
 }
@@ -1797,27 +1798,8 @@ expand_grouping_sets(List *groupingSets, int limit)
 		result = new_result;
 	}
 
-	if (list_length(result) > 1)
-	{
-		int			result_len = list_length(result);
-		List	  **buf = palloc(sizeof(List *) * result_len);
-		List	  **ptr = buf;
-
-		foreach(lc, result)
-		{
-			*ptr++ = lfirst(lc);
-		}
-
-		qsort(buf, result_len, sizeof(List *), cmp_list_len_asc);
-
-		result = NIL;
-		ptr = buf;
-
-		while (result_len-- > 0)
-			result = lappend(result, *ptr++);
-
-		pfree(buf);
-	}
+	/* Now sort the lists by length */
+	list_sort(result, cmp_list_len_asc);
 
 	return result;
 }

src/backend/replication/basebackup.c

@@ -70,7 +70,7 @@ static void base_backup_cleanup(int code, Datum arg);
 static void perform_base_backup(basebackup_options *opt);
 static void parse_basebackup_options(List *options, basebackup_options *opt);
 static void SendXlogRecPtrResult(XLogRecPtr ptr, TimeLineID tli);
-static int	compareWalFileNames(const void *a, const void *b);
+static int	compareWalFileNames(const ListCell *a, const ListCell *b);
 static void throttle(size_t increment);
 static bool is_checksummed_file(const char *fullpath, const char *filename);
 
@@ -379,13 +379,10 @@ perform_base_backup(basebackup_options *opt)
 		struct stat statbuf;
 		List	   *historyFileList = NIL;
 		List	   *walFileList = NIL;
-		char	  **walFiles;
-		int			nWalFiles;
 		char		firstoff[MAXFNAMELEN];
 		char		lastoff[MAXFNAMELEN];
 		DIR		   *dir;
 		struct dirent *de;
-		int			i;
 		ListCell   *lc;
 		TimeLineID	tli;
 
@@ -428,24 +425,17 @@ perform_base_backup(basebackup_options *opt)
 		CheckXLogRemoved(startsegno, ThisTimeLineID);
 
 		/*
-		 * Put the WAL filenames into an array, and sort. We send the files in
-		 * order from oldest to newest, to reduce the chance that a file is
-		 * recycled before we get a chance to send it over.
+		 * Sort the WAL filenames.  We want to send the files in order from
+		 * oldest to newest, to reduce the chance that a file is recycled
+		 * before we get a chance to send it over.
 		 */
-		nWalFiles = list_length(walFileList);
-		walFiles = palloc(nWalFiles * sizeof(char *));
-		i = 0;
-		foreach(lc, walFileList)
-		{
-			walFiles[i++] = lfirst(lc);
-		}
-		qsort(walFiles, nWalFiles, sizeof(char *), compareWalFileNames);
+		list_sort(walFileList, compareWalFileNames);
 
 		/*
 		 * There must be at least one xlog file in the pg_wal directory, since
 		 * we are doing backup-including-xlog.
 		 */
-		if (nWalFiles < 1)
+		if (walFileList == NIL)
 			ereport(ERROR,
 					(errmsg("could not find any WAL files")));
 
@@ -453,7 +443,8 @@ perform_base_backup(basebackup_options *opt)
 		 * Sanity check: the first and last segment should cover startptr and
 		 * endptr, with no gaps in between.
 		 */
-		XLogFromFileName(walFiles[0], &tli, &segno, wal_segment_size);
+		XLogFromFileName((char *) linitial(walFileList),
+						 &tli, &segno, wal_segment_size);
 		if (segno != startsegno)
 		{
 			char		startfname[MAXFNAMELEN];
@@ -463,12 +454,13 @@ perform_base_backup(basebackup_options *opt)
 			ereport(ERROR,
 					(errmsg("could not find WAL file \"%s\"", startfname)));
 		}
-		for (i = 0; i < nWalFiles; i++)
+		foreach(lc, walFileList)
 		{
+			char	   *walFileName = (char *) lfirst(lc);
 			XLogSegNo	currsegno = segno;
 			XLogSegNo	nextsegno = segno + 1;
 
-			XLogFromFileName(walFiles[i], &tli, &segno, wal_segment_size);
+			XLogFromFileName(walFileName, &tli, &segno, wal_segment_size);
 			if (!(nextsegno == segno || currsegno == segno))
 			{
 				char		nextfname[MAXFNAMELEN];
@@ -489,15 +481,16 @@ perform_base_backup(basebackup_options *opt)
 		}
 
 		/* Ok, we have everything we need. Send the WAL files. */
-		for (i = 0; i < nWalFiles; i++)
+		foreach(lc, walFileList)
 		{
+			char	   *walFileName = (char *) lfirst(lc);
 			FILE	   *fp;
 			char		buf[TAR_SEND_SIZE];
 			size_t		cnt;
 			pgoff_t		len = 0;
 
-			snprintf(pathbuf, MAXPGPATH, XLOGDIR "/%s", walFiles[i]);
-			XLogFromFileName(walFiles[i], &tli, &segno, wal_segment_size);
+			snprintf(pathbuf, MAXPGPATH, XLOGDIR "/%s", walFileName);
+			XLogFromFileName(walFileName, &tli, &segno, wal_segment_size);
 
 			fp = AllocateFile(pathbuf, "rb");
 			if (fp == NULL)
@@ -527,7 +520,7 @@ perform_base_backup(basebackup_options *opt)
 				CheckXLogRemoved(segno, tli);
 				ereport(ERROR,
 						(errcode_for_file_access(),
-						 errmsg("unexpected WAL file size \"%s\"", walFiles[i])));
+						 errmsg("unexpected WAL file size \"%s\"", walFileName)));
 			}
 
 			/* send the WAL file itself */
@@ -555,7 +548,7 @@ perform_base_backup(basebackup_options *opt)
 				CheckXLogRemoved(segno, tli);
 				ereport(ERROR,
 						(errcode_for_file_access(),
-						 errmsg("unexpected WAL file size \"%s\"", walFiles[i])));
+						 errmsg("unexpected WAL file size \"%s\"", walFileName)));
 			}
 
 			/* wal_segment_size is a multiple of 512, so no need for padding */
@@ -568,7 +561,7 @@ perform_base_backup(basebackup_options *opt)
 			 * walreceiver.c always doing an XLogArchiveForceDone() after a
 			 * complete segment.
 			 */
-			StatusFilePath(pathbuf, walFiles[i], ".done");
+			StatusFilePath(pathbuf, walFileName, ".done");
 			sendFileWithContent(pathbuf, "");
 		}
 
@@ -618,14 +611,14 @@ perform_base_backup(basebackup_options *opt)
 }
 
 /*
- * qsort comparison function, to compare log/seg portion of WAL segment
+ * list_sort comparison function, to compare log/seg portion of WAL segment
  * filenames, ignoring the timeline portion.
  */
 static int
-compareWalFileNames(const void *a, const void *b)
+compareWalFileNames(const ListCell *a, const ListCell *b)
 {
-	char	   *fna = *((char **) a);
-	char	   *fnb = *((char **) b);
+	char	   *fna = (char *) lfirst(a);
+	char	   *fnb = (char *) lfirst(b);
 
 	return strcmp(fna + 8, fnb + 8);
 }

src/backend/replication/logical/reorderbuffer.c

@@ -3236,7 +3236,7 @@ ReorderBufferToastReset(ReorderBuffer *rb, ReorderBufferTXN *txn)
  * -------------------------------------------------------------------------
  */
 
-/* struct for qsort()ing mapping files by lsn somewhat efficiently */
+/* struct for sorting mapping files by LSN efficiently */
 typedef struct RewriteMappingFile
 {
 	XLogRecPtr	lsn;
@@ -3378,13 +3378,13 @@ TransactionIdInArray(TransactionId xid, TransactionId *xip, Size num)
 }
 
 /*
- * qsort() comparator for sorting RewriteMappingFiles in LSN order.
+ * list_sort() comparator for sorting RewriteMappingFiles in LSN order.
  */
 static int
-file_sort_by_lsn(const void *a_p, const void *b_p)
+file_sort_by_lsn(const ListCell *a_p, const ListCell *b_p)
 {
-	RewriteMappingFile *a = *(RewriteMappingFile **) a_p;
-	RewriteMappingFile *b = *(RewriteMappingFile **) b_p;
+	RewriteMappingFile *a = (RewriteMappingFile *) lfirst(a_p);
+	RewriteMappingFile *b = (RewriteMappingFile *) lfirst(b_p);
 
 	if (a->lsn < b->lsn)
 		return -1;
@@ -3404,8 +3404,6 @@ UpdateLogicalMappings(HTAB *tuplecid_data, Oid relid, Snapshot snapshot)
 	struct dirent *mapping_de;
 	List	   *files = NIL;
 	ListCell   *file;
-	RewriteMappingFile **files_a;
-	size_t		off;
 	Oid			dboid = IsSharedRelation(relid) ? InvalidOid : MyDatabaseId;
 
 	mapping_dir = AllocateDir("pg_logical/mappings");
@@ -3459,21 +3457,12 @@ UpdateLogicalMappings(HTAB *tuplecid_data, Oid relid, Snapshot snapshot)
 	}
 	FreeDir(mapping_dir);
 
-	/* build array we can easily sort */
-	files_a = palloc(list_length(files) * sizeof(RewriteMappingFile *));
-	off = 0;
-	foreach(file, files)
-	{
-		files_a[off++] = lfirst(file);
-	}
-
 	/* sort files so we apply them in LSN order */
-	qsort(files_a, list_length(files), sizeof(RewriteMappingFile *),
-		  file_sort_by_lsn);
+	list_sort(files, file_sort_by_lsn);
 
-	for (off = 0; off < list_length(files); off++)
+	foreach(file, files)
 	{
-		RewriteMappingFile *f = files_a[off];
+		RewriteMappingFile *f = (RewriteMappingFile *) lfirst(file);
 
 		elog(DEBUG1, "applying mapping: \"%s\" in %u", f->fname,
 			 snapshot->subxip[0]);

src/backend/rewrite/rowsecurity.c

@@ -63,9 +63,9 @@ static void get_policies_for_relation(Relation relation,
 									  List **permissive_policies,
 									  List **restrictive_policies);
 
-static List *sort_policies_by_name(List *policies);
+static void sort_policies_by_name(List *policies);
 
-static int	row_security_policy_cmp(const void *a, const void *b);
+static int	row_security_policy_cmp(const ListCell *a, const ListCell *b);
 
 static void add_security_quals(int rt_index,
 							   List *permissive_policies,
@@ -470,7 +470,7 @@ get_policies_for_relation(Relation relation, CmdType cmd, Oid user_id,
 	 * We sort restrictive policies by name so that any WCOs they generate are
 	 * checked in a well-defined order.
 	 */
-	*restrictive_policies = sort_policies_by_name(*restrictive_policies);
+	sort_policies_by_name(*restrictive_policies);
 
 	/*
 	 * Then add any permissive or restrictive policies defined by extensions.
@@ -488,7 +488,7 @@ get_policies_for_relation(Relation relation, CmdType cmd, Oid user_id,
 		 * always check all built-in restrictive policies, in name order,
 		 * before checking restrictive policies added by hooks, in name order.
 		 */
-		hook_policies = sort_policies_by_name(hook_policies);
+		sort_policies_by_name(hook_policies);
 
 		foreach(item, hook_policies)
 		{
@@ -522,43 +522,20 @@ get_policies_for_relation(Relation relation, CmdType cmd, Oid user_id,
  * This is not necessary for permissive policies, since they are all combined
  * together using OR into a single WithCheckOption check.
  */
-static List *
+static void
 sort_policies_by_name(List *policies)
 {
-	int			npol = list_length(policies);
-	RowSecurityPolicy *pols;
-	ListCell   *item;
-	int			ii = 0;
-
-	if (npol <= 1)
-		return policies;
-
-	pols = (RowSecurityPolicy *) palloc(sizeof(RowSecurityPolicy) * npol);
-
-	foreach(item, policies)
-	{
-		RowSecurityPolicy *policy = (RowSecurityPolicy *) lfirst(item);
-
-		pols[ii++] = *policy;
-	}
-
-	qsort(pols, npol, sizeof(RowSecurityPolicy), row_security_policy_cmp);
-
-	policies = NIL;
-	for (ii = 0; ii < npol; ii++)
-		policies = lappend(policies, &pols[ii]);
-
-	return policies;
+	list_sort(policies, row_security_policy_cmp);
 }
 
 /*
- * qsort comparator to sort RowSecurityPolicy entries by name
+ * list_sort comparator to sort RowSecurityPolicy entries by name
  */
 static int
-row_security_policy_cmp(const void *a, const void *b)
+row_security_policy_cmp(const ListCell *a, const ListCell *b)
 {
-	const RowSecurityPolicy *pa = (const RowSecurityPolicy *) a;
-	const RowSecurityPolicy *pb = (const RowSecurityPolicy *) b;
+	const RowSecurityPolicy *pa = (const RowSecurityPolicy *) lfirst(a);
+	const RowSecurityPolicy *pb = (const RowSecurityPolicy *) lfirst(b);
 
 	/* Guard against NULL policy names from extensions */
 	if (pa->policy_name == NULL)

src/include/nodes/pg_list.h

@@ -570,7 +570,7 @@ extern List *list_copy(const List *list);
 extern List *list_copy_tail(const List *list, int nskip);
 extern List *list_copy_deep(const List *oldlist);
 
-typedef int (*list_qsort_comparator) (const void *a, const void *b);
-extern List *list_qsort(const List *list, list_qsort_comparator cmp);
+typedef int (*list_sort_comparator) (const ListCell *a, const ListCell *b);
+extern void list_sort(List *list, list_sort_comparator cmp);
 
 #endif							/* PG_LIST_H */