]> git.ipfire.org Git - thirdparty/postgresql.git/commitdiff
Avoid O(N^2) behavior in SyncPostCheckpoint().
authorTom Lane <tgl@sss.pgh.pa.us>
Tue, 2 Nov 2021 15:31:54 +0000 (11:31 -0400)
committerTom Lane <tgl@sss.pgh.pa.us>
Tue, 2 Nov 2021 15:31:54 +0000 (11:31 -0400)
As in commits 6301c3ada and e9d9ba2a4, 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

src/backend/nodes/list.c
src/backend/optimizer/util/clauses.c
src/backend/parser/parse_func.c
src/backend/storage/sync/sync.c
src/include/nodes/pg_list.h

index 80fa8c84e49ae83ca936fdcd1a01fc9188c7d9fa..bc2f2fcc025f161de9e0b6f5543dae1a4b59b0e1 100644 (file)
@@ -891,6 +891,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
index 97346e87fe799de5327b84e34dfa0088a96c3e9a..dc908eae43695d211e83387293866f213809bd0d 100644 (file)
@@ -4165,7 +4165,7 @@ add_function_defaults(List *args, 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);
index 9c3b6ad9166254d37d35c56d3a35c7ab3c1058a3..c28fc7d0bdb876c5c9ebd279414a83efc87c4ae1 100644 (file)
@@ -1679,7 +1679,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;
                        }
                }
index 3ded2cdd716bc2cdc49604c9f5f61e682e2b34ec..4ce2e687d3c8983a50862ea71146e3a348a665ed 100644 (file)
@@ -66,6 +66,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;
@@ -174,13 +175,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.
@@ -210,15 +216,13 @@ 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)
                {
@@ -226,6 +230,26 @@ SyncPostCheckpoint(void)
                        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);
+       }
 }
 
 /*
@@ -465,17 +489,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)
@@ -487,6 +508,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);
 
index 54447bb5eb63626b302d1e968467886a90ddab1a..b30596812b7fd294cb2c8e6947e209cf5734e1c5 100644 (file)
@@ -560,6 +560,7 @@ extern List *list_delete_int(List *list, int datum);
 extern List *list_delete_oid(List *list, Oid datum);
 extern List *list_delete_first(List *list);
 extern List *list_delete_last(List *list);
+extern List *list_delete_first_n(List *list, int n);
 extern List *list_delete_nth_cell(List *list, int n);
 extern List *list_delete_cell(List *list, ListCell *cell);