Avoid O(N^2) behavior in SyncPostCheckpoint().

tglsfdc · tglsfdc · commit 08cfa5981e17 · 2021-11-02T11:31:54.000-04:00
As in commits 6301c3a and e9d9ba2, avoid doing repetitive list_delete_first() operations, since that would be expensive when there are many files waiting to be unlinked. This is a slightly larger change than in those cases. We have to keep the list state valid for calls to AbsorbSyncRequests(), so it's necessary to invent a "canceled" field instead of immediately deleting PendingUnlinkEntry entries. Also, because we might not be able to process all the entries, we need a new list primitive list_delete_first_n(). list_delete_first_n() is almost list_copy_tail(), but it modifies the input List instead of making a new copy. I found a couple of existing uses of the latter that could profitably use the new function. (There might be more, but the other callers look like they probably shouldn't overwrite the input List.) As before, back-patch to v13. Discussion: https://postgr.es/m/CD2F0E7F-9822-45EC-A411-AE56F14DEA9F@amazon.com
diff --git a/src/backend/nodes/list.c b/src/backend/nodes/list.c
@@ -906,6 +906,72 @@ list_delete_last(List *list)
 	return list_truncate(list, list_length(list) - 1);
 }
 
+/*
+ * Delete the first N cells of the list.
+ *
+ * The List is pfree'd if the request causes all cells to be deleted.
+ */
+List *
+list_delete_first_n(List *list, int n)
+{
+	check_list_invariants(list);
+
+	/* No-op request? */
+	if (n <= 0)
+		return list;
+
+	/* Delete whole list? */
+	if (n >= list_length(list))
+	{
+		list_free(list);
+		return NIL;
+	}
+
+	/*
+	 * Otherwise, we normally just collapse out the removed elements.  But for
+	 * debugging purposes, move the whole list contents someplace else.
+	 *
+	 * (Note that we *must* keep the contents in the same memory context.)
+	 */
+#ifndef DEBUG_LIST_MEMORY_USAGE
+	memmove(&list->elements[0], &list->elements[n],
+			(list->length - n) * sizeof(ListCell));
+	list->length -= n;
+#else
+	{
+		ListCell   *newelems;
+		int			newmaxlen = list->length - n;
+
+		newelems = (ListCell *)
+			MemoryContextAlloc(GetMemoryChunkContext(list),
+							   newmaxlen * sizeof(ListCell));
+		memcpy(newelems, &list->elements[n], newmaxlen * sizeof(ListCell));
+		if (list->elements != list->initial_elements)
+			pfree(list->elements);
+		else
+		{
+			/*
+			 * As in enlarge_list(), clear the initial_elements[] space and/or
+			 * mark it inaccessible.
+			 */
+#ifdef CLOBBER_FREED_MEMORY
+			wipe_mem(list->initial_elements,
+					 list->max_length * sizeof(ListCell));
+#else
+			VALGRIND_MAKE_MEM_NOACCESS(list->initial_elements,
+									   list->max_length * sizeof(ListCell));
+#endif
+		}
+		list->elements = newelems;
+		list->max_length = newmaxlen;
+		list->length = newmaxlen;
+		check_list_invariants(list);
+	}
+#endif
+
+	return list;
+}
+
 /*
  * Generate the union of two lists. This is calculated by copying
  * list1 via list_copy(), then adding to it all the members of list2
diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c
@@ -4092,7 +4092,7 @@ add_function_defaults(List *args, int pronargs, HeapTuple func_tuple)
 	if (ndelete < 0)
 		elog(ERROR, "not enough default arguments");
 	if (ndelete > 0)
-		defaults = list_copy_tail(defaults, ndelete);
+		defaults = list_delete_first_n(defaults, ndelete);
 
 	/* And form the combined argument list, not modifying the input list */
 	return list_concat_copy(args, defaults);
diff --git a/src/backend/parser/parse_func.c b/src/backend/parser/parse_func.c
@@ -1691,7 +1691,7 @@ func_get_detail(List *funcname,
 
 				ndelete = list_length(defaults) - best_candidate->ndargs;
 				if (ndelete > 0)
-					defaults = list_copy_tail(defaults, ndelete);
+					defaults = list_delete_first_n(defaults, ndelete);
 				*argdefaults = defaults;
 			}
 		}
diff --git a/src/backend/storage/sync/sync.c b/src/backend/storage/sync/sync.c
@@ -69,6 +69,7 @@ typedef struct
 {
 	FileTag		tag;			/* identifies handler and file */
 	CycleCtr	cycle_ctr;		/* checkpoint_cycle_ctr when request was made */
+	bool		canceled;		/* true if request has been canceled */
 } PendingUnlinkEntry;
 
 static HTAB *pendingOps = NULL;
@@ -195,13 +196,18 @@ void
 SyncPostCheckpoint(void)
 {
 	int			absorb_counter;
+	ListCell   *lc;
 
 	absorb_counter = UNLINKS_PER_ABSORB;
-	while (pendingUnlinks != NIL)
+	foreach(lc, pendingUnlinks)
 	{
-		PendingUnlinkEntry *entry = (PendingUnlinkEntry *) linitial(pendingUnlinks);
+		PendingUnlinkEntry *entry = (PendingUnlinkEntry *) lfirst(lc);
 		char		path[MAXPGPATH];
 
+		/* Skip over any canceled entries */
+		if (entry->canceled)
+			continue;
+
 		/*
 		 * New entries are appended to the end, so if the entry is new we've
 		 * reached the end of old entries.
@@ -231,22 +237,40 @@ SyncPostCheckpoint(void)
 						 errmsg("could not remove file \"%s\": %m", path)));
 		}
 
-		/* And remove the list entry */
-		pendingUnlinks = list_delete_first(pendingUnlinks);
-		pfree(entry);
+		/* Mark the list entry as canceled, just in case */
+		entry->canceled = true;
 
 		/*
 		 * As in ProcessSyncRequests, we don't want to stop absorbing fsync
 		 * requests for a long time when there are many deletions to be done.
-		 * We can safely call AbsorbSyncRequests() at this point in the loop
-		 * (note it might try to delete list entries).
+		 * We can safely call AbsorbSyncRequests() at this point in the loop.
 		 */
 		if (--absorb_counter <= 0)
 		{
 			AbsorbSyncRequests();
 			absorb_counter = UNLINKS_PER_ABSORB;
 		}
 	}
+
+	/*
+	 * If we reached the end of the list, we can just remove the whole list
+	 * (remembering to pfree all the PendingUnlinkEntry objects).  Otherwise,
+	 * we must keep the entries at or after "lc".
+	 */
+	if (lc == NULL)
+	{
+		list_free_deep(pendingUnlinks);
+		pendingUnlinks = NIL;
+	}
+	else
+	{
+		int			ntodelete = list_cell_number(pendingUnlinks, lc);
+
+		for (int i = 0; i < ntodelete; i++)
+			pfree(list_nth(pendingUnlinks, i));
+
+		pendingUnlinks = list_delete_first_n(pendingUnlinks, ntodelete);
+	}
 }
 
 /*
@@ -486,17 +510,14 @@ RememberSyncRequest(const FileTag *ftag, SyncRequestType type)
 				entry->canceled = true;
 		}
 
-		/* Remove matching unlink requests */
+		/* Cancel matching unlink requests */
 		foreach(cell, pendingUnlinks)
 		{
 			PendingUnlinkEntry *entry = (PendingUnlinkEntry *) lfirst(cell);
 
 			if (entry->tag.handler == ftag->handler &&
 				syncsw[ftag->handler].sync_filetagmatches(ftag, &entry->tag))
-			{
-				pendingUnlinks = foreach_delete_current(pendingUnlinks, cell);
-				pfree(entry);
-			}
+				entry->canceled = true;
 		}
 	}
 	else if (type == SYNC_UNLINK_REQUEST)
@@ -508,6 +529,7 @@ RememberSyncRequest(const FileTag *ftag, SyncRequestType type)
 		entry = palloc(sizeof(PendingUnlinkEntry));
 		entry->tag = *ftag;
 		entry->cycle_ctr = checkpoint_cycle_ctr;
+		entry->canceled = false;
 
 		pendingUnlinks = lappend(pendingUnlinks, entry);
 
diff --git a/src/include/nodes/pg_list.h b/src/include/nodes/pg_list.h
@@ -564,6 +564,7 @@ extern pg_nodiscard List *list_delete_int(List *list, int datum);
 extern pg_nodiscard List *list_delete_oid(List *list, Oid datum);
 extern pg_nodiscard List *list_delete_first(List *list);
 extern pg_nodiscard List *list_delete_last(List *list);
+extern pg_nodiscard List *list_delete_first_n(List *list, int n);
 extern pg_nodiscard List *list_delete_nth_cell(List *list, int n);
 extern pg_nodiscard List *list_delete_cell(List *list, ListCell *cell);
 

Original file line number	Diff line number	Diff line change
`@@ -1691,7 +1691,7 @@ func_get_detail(List *funcname,`
`1691`	`1691`
`1692`	`1692`	`ndelete = list_length(defaults) - best_candidate->ndargs;`
`1693`	`1693`	`if (ndelete > 0)`
`1694`		`- defaults = list_copy_tail(defaults, ndelete);`
	`1694`	`+ defaults = list_delete_first_n(defaults, ndelete);`
`1695`	`1695`	`*argdefaults = defaults;`
`1696`	`1696`	`}`
`1697`	`1697`	`}`