From 569ed7f48312c70ed4a79daec1d7688fda4e74ac Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Tue, 16 Jul 2019 11:51:44 -0400 Subject: [PATCH] Redesign the API for list sorting (list_qsort becomes list_sort). In the wake of commit 1cff1b95a, 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 | 53 +++++++------------ src/backend/optimizer/util/pathnode.c | 31 +++++------ src/backend/parser/parse_agg.c | 30 +++-------- src/backend/replication/basebackup.c | 51 ++++++++---------- .../replication/logical/reorderbuffer.c | 29 ++++------ src/backend/rewrite/rowsecurity.c | 43 ++++----------- src/include/nodes/pg_list.h | 4 +- 7 files changed, 83 insertions(+), 158 deletions(-) diff --git a/src/backend/nodes/list.c b/src/backend/nodes/list.c index ecc2911496..279d7a91c4 100644 --- a/src/backend/nodes/list.c +++ b/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; + typedef int (*qsort_comparator) (const void *a, const void *b); + int len; - /* Empty list is easy */ - if (len == 0) - return NIL; + check_list_invariants(list); - /* 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(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); } diff --git a/src/backend/optimizer/util/pathnode.c b/src/backend/optimizer/util/pathnode.c index 67254c2fd9..0ac73984d2 100644 --- a/src/backend/optimizer/util/pathnode.c +++ b/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); diff --git a/src/backend/parser/parse_agg.c b/src/backend/parser/parse_agg.c index 1ae4fb5b21..19e3164afb 100644 --- a/src/backend/parser/parse_agg.c +++ b/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; } diff --git a/src/backend/replication/basebackup.c b/src/backend/replication/basebackup.c index d8c370efc7..d5f9b617c8 100644 --- a/src/backend/replication/basebackup.c +++ b/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); } diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c index 591377d2cd..e8ffa0492f 100644 --- a/src/backend/replication/logical/reorderbuffer.c +++ b/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; + /* sort files so we apply them in LSN order */ + list_sort(files, file_sort_by_lsn); + 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); - - for (off = 0; off < list_length(files); off++) - { - RewriteMappingFile *f = files_a[off]; + RewriteMappingFile *f = (RewriteMappingFile *) lfirst(file); elog(DEBUG1, "applying mapping: \"%s\" in %u", f->fname, snapshot->subxip[0]); diff --git a/src/backend/rewrite/rowsecurity.c b/src/backend/rewrite/rowsecurity.c index 300af6f06f..1c44714589 100644 --- a/src/backend/rewrite/rowsecurity.c +++ b/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) diff --git a/src/include/nodes/pg_list.h b/src/include/nodes/pg_list.h index 8b1e4fb69e..6be26a2074 100644 --- a/src/include/nodes/pg_list.h +++ b/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 */