From 2f5b8eb5a28b4e6de9d20cc7d2c6028c6c7a8aa8 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Tue, 16 Jul 2019 12:04:06 -0400 Subject: [PATCH] Clean up some ad-hoc code for sorting and de-duplicating Lists. heap.c and relcache.c contained nearly identical copies of logic to insert OIDs into an OID list while preserving the list's OID ordering (and rejecting duplicates, in one case but not the other). The comments argue that this is faster than qsort for small numbers of OIDs, which is at best unproven, and seems even less likely to be true now that lappend_cell_oid has to move data around. In any case it's ugly and hard-to-follow code, and if we do have a lot of OIDs to consider, it's O(N^2). Hence, replace with simply lappend'ing OIDs to a List, then list_sort the completed List, then remove adjacent duplicates if necessary. This is demonstrably O(N log N) and it's much simpler for the callers. It's possible that this would be somewhat inefficient if there were a very large number of duplicates, but that seems unlikely in the existing usage. This adds list_deduplicate_oid and list_oid_cmp infrastructure to list.c. I didn't bother with equivalent functionality for integer or pointer Lists, but such could always be added later if we find a use for it. Discussion: https://postgr.es/m/26193.1563228600@sss.pgh.pa.us --- src/backend/catalog/heap.c | 49 ++++-------------------------- src/backend/nodes/list.c | 44 +++++++++++++++++++++++++++ src/backend/utils/cache/relcache.c | 46 ++++++---------------------- src/include/nodes/pg_list.h | 4 +++ 4 files changed, 63 insertions(+), 80 deletions(-) diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c index ab8a475923..6c3ff762df 100644 --- a/src/backend/catalog/heap.c +++ b/src/backend/catalog/heap.c @@ -125,7 +125,6 @@ static void SetRelationNumChecks(Relation rel, int numchecks); static Node *cookConstraint(ParseState *pstate, Node *raw_constraint, char *relname); -static List *insert_ordered_unique_oid(List *list, Oid datum); /* ---------------------------------------------------------------- @@ -3384,57 +3383,21 @@ heap_truncate_find_FKs(List *relationIds) if (!list_member_oid(relationIds, con->confrelid)) continue; - /* Add referencer unless already in input or result list */ + /* Add referencer to result, unless present in input list */ if (!list_member_oid(relationIds, con->conrelid)) - result = insert_ordered_unique_oid(result, con->conrelid); + result = lappend_oid(result, con->conrelid); } systable_endscan(fkeyScan); table_close(fkeyRel, AccessShareLock); + /* Now sort and de-duplicate the result list */ + list_sort(result, list_oid_cmp); + list_deduplicate_oid(result); + return result; } -/* - * insert_ordered_unique_oid - * Insert a new Oid into a sorted list of Oids, preserving ordering, - * and eliminating duplicates - * - * Building the ordered list this way is O(N^2), but with a pretty small - * constant, so for the number of entries we expect it will probably be - * faster than trying to apply qsort(). It seems unlikely someone would be - * trying to truncate a table with thousands of dependent tables ... - */ -static List * -insert_ordered_unique_oid(List *list, Oid datum) -{ - ListCell *prev; - - /* Does the datum belong at the front? */ - if (list == NIL || datum < linitial_oid(list)) - return lcons_oid(datum, list); - /* Does it match the first entry? */ - if (datum == linitial_oid(list)) - return list; /* duplicate, so don't insert */ - /* No, so find the entry it belongs after */ - prev = list_head(list); - for (;;) - { - ListCell *curr = lnext(list, prev); - - if (curr == NULL || datum < lfirst_oid(curr)) - break; /* it belongs after 'prev', before 'curr' */ - - if (datum == lfirst_oid(curr)) - return list; /* duplicate, so don't insert */ - - prev = curr; - } - /* Insert datum into list after 'prev' */ - lappend_cell_oid(list, prev, datum); - return list; -} - /* * StorePartitionKey * Store information about the partition key rel into the catalog diff --git a/src/backend/nodes/list.c b/src/backend/nodes/list.c index 279d7a91c4..0b8686d262 100644 --- a/src/backend/nodes/list.c +++ b/src/backend/nodes/list.c @@ -1297,6 +1297,34 @@ list_concat_unique_oid(List *list1, const List *list2) return list1; } +/* + * Remove adjacent duplicates in a list of OIDs. + * + * It is caller's responsibility to have sorted the list to bring duplicates + * together, perhaps via list_sort(list, list_oid_cmp). + */ +void +list_deduplicate_oid(List *list) +{ + int len; + + Assert(IsOidList(list)); + len = list_length(list); + if (len > 1) + { + ListCell *elements = list->elements; + int i = 0; + + for (int j = 1; j < len; j++) + { + if (elements[i].oid_value != elements[j].oid_value) + elements[++i].oid_value = elements[j].oid_value; + } + list->length = i + 1; + } + check_list_invariants(list); +} + /* * Free all storage in a list, and optionally the pointed-to elements */ @@ -1444,3 +1472,19 @@ list_sort(List *list, list_sort_comparator cmp) if (len > 1) qsort(list->elements, len, sizeof(ListCell), (qsort_comparator) cmp); } + +/* + * list_sort comparator for sorting a list into ascending OID order. + */ +int +list_oid_cmp(const ListCell *p1, const ListCell *p2) +{ + Oid v1 = lfirst_oid(p1); + Oid v2 = lfirst_oid(p2); + + if (v1 < v2) + return -1; + if (v1 > v2) + return 1; + return 0; +} diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c index 3ca9a9f358..7aa5d7c7fa 100644 --- a/src/backend/utils/cache/relcache.c +++ b/src/backend/utils/cache/relcache.c @@ -285,7 +285,6 @@ static TupleDesc GetPgIndexDescriptor(void); static void AttrDefaultFetch(Relation relation); static void CheckConstraintFetch(Relation relation); static int CheckConstraintCmp(const void *a, const void *b); -static List *insert_ordered_oid(List *list, Oid datum); static void InitIndexAmRoutine(Relation relation); static void IndexSupportInitialize(oidvector *indclass, RegProcedure *indexSupport, @@ -4387,8 +4386,8 @@ RelationGetIndexList(Relation relation) if (!index->indislive) continue; - /* Add index's OID to result list in the proper order */ - result = insert_ordered_oid(result, index->indexrelid); + /* add index's OID to result list */ + result = lappend_oid(result, index->indexrelid); /* * Invalid, non-unique, non-immediate or predicate indexes aren't @@ -4413,6 +4412,9 @@ RelationGetIndexList(Relation relation) table_close(indrel, AccessShareLock); + /* Sort the result list into OID order, per API spec. */ + list_sort(result, list_oid_cmp); + /* Now save a copy of the completed list in the relcache entry. */ oldcxt = MemoryContextSwitchTo(CacheMemoryContext); oldlist = relation->rd_indexlist; @@ -4494,13 +4496,16 @@ RelationGetStatExtList(Relation relation) { Oid oid = ((Form_pg_statistic_ext) GETSTRUCT(htup))->oid; - result = insert_ordered_oid(result, oid); + result = lappend_oid(result, oid); } systable_endscan(indscan); table_close(indrel, AccessShareLock); + /* Sort the result list into OID order, per API spec. */ + list_sort(result, list_oid_cmp); + /* Now save a copy of the completed list in the relcache entry. */ oldcxt = MemoryContextSwitchTo(CacheMemoryContext); oldlist = relation->rd_statlist; @@ -4515,39 +4520,6 @@ RelationGetStatExtList(Relation relation) return result; } -/* - * insert_ordered_oid - * Insert a new Oid into a sorted list of Oids, preserving ordering - * - * Building the ordered list this way is O(N^2), but with a pretty small - * constant, so for the number of entries we expect it will probably be - * faster than trying to apply qsort(). Most tables don't have very many - * indexes... - */ -static List * -insert_ordered_oid(List *list, Oid datum) -{ - ListCell *prev; - - /* Does the datum belong at the front? */ - if (list == NIL || datum < linitial_oid(list)) - return lcons_oid(datum, list); - /* No, so find the entry it belongs after */ - prev = list_head(list); - for (;;) - { - ListCell *curr = lnext(list, prev); - - if (curr == NULL || datum < lfirst_oid(curr)) - break; /* it belongs after 'prev', before 'curr' */ - - prev = curr; - } - /* Insert datum into list after 'prev' */ - lappend_cell_oid(list, prev, datum); - return list; -} - /* * RelationGetPrimaryKeyIndex -- get OID of the relation's primary key index * diff --git a/src/include/nodes/pg_list.h b/src/include/nodes/pg_list.h index 6be26a2074..418f000629 100644 --- a/src/include/nodes/pg_list.h +++ b/src/include/nodes/pg_list.h @@ -563,6 +563,8 @@ extern List *list_concat_unique_ptr(List *list1, const List *list2); extern List *list_concat_unique_int(List *list1, const List *list2); extern List *list_concat_unique_oid(List *list1, const List *list2); +extern void list_deduplicate_oid(List *list); + extern void list_free(List *list); extern void list_free_deep(List *list); @@ -573,4 +575,6 @@ extern List *list_copy_deep(const List *oldlist); typedef int (*list_sort_comparator) (const ListCell *a, const ListCell *b); extern void list_sort(List *list, list_sort_comparator cmp); +extern int list_oid_cmp(const ListCell *p1, const ListCell *p2); + #endif /* PG_LIST_H */