Improve ilist.h's support for deletion of slist elements during iteration.

Previously one had to use slist_delete(), implying an additional scan of
the list, making this infrastructure considerably less efficient than
traditional Lists when deletion of element(s) in a long list is needed.
Modify the slist_foreach_modify() macro to support deleting the current
element in O(1) time, by keeping a "prev" pointer in addition to "cur"
and "next".  Although this makes iteration with this macro a bit slower,
no real harm is done, since in any scenario where you're not going to
delete the current list element you might as well just use slist_foreach
instead.  Improve the comments about when to use each macro.

Back-patch to 9.3 so that we'll have consistent semantics in all branches
that provide ilist.h.  Note this is an ABI break for callers of
slist_foreach_modify().

Andres Freund and Tom Lane

6 files changed, 72 insertions, 20 deletions

diff --git a/src/backend/lib/ilist.c b/src/backend/lib/ilist.c
index 957a1181227..eb23fa1c687 100644
--- a/src/backend/lib/ilist.c
+++ b/src/backend/lib/ilist.c
@@ -28,7 +28,7 @@
  *
  * It is not allowed to delete a 'node' which is is not in the list 'head'
  *
- * Caution: this is O(n)
+ * Caution: this is O(n); consider using slist_delete_current() instead.
  */
 void
 slist_delete(slist_head *head, slist_node *node)
diff --git a/src/backend/postmaster/bgworker.c b/src/backend/postmaster/bgworker.c
index ada24e905e6..f807c9cfa19 100644
--- a/src/backend/postmaster/bgworker.c
+++ b/src/backend/postmaster/bgworker.c
@@ -267,15 +267,20 @@ BackgroundWorkerStateChange(void)
 /*
  * Forget about a background worker that's no longer needed.
  *
- * At present, this only happens when a background worker marked
- * BGW_NEVER_RESTART exits.  This function should only be invoked in
- * the postmaster.
+ * The worker must be identified by passing an slist_mutable_iter that
+ * points to it.  This convention allows deletion of workers during
+ * searches of the worker list, and saves having to search the list again.
+ *
+ * This function must be invoked only in the postmaster.
  */
 void
-ForgetBackgroundWorker(RegisteredBgWorker *rw)
+ForgetBackgroundWorker(slist_mutable_iter *cur)
 {
+	RegisteredBgWorker *rw;
 	BackgroundWorkerSlot *slot;
 
+	rw = slist_container(RegisteredBgWorker, rw_lnode, cur->cur);
+
 	Assert(rw->rw_shmem_slot < max_worker_processes);
 	slot = &BackgroundWorkerData->slot[rw->rw_shmem_slot];
 	slot->in_use = false;
@@ -284,7 +289,7 @@ ForgetBackgroundWorker(RegisteredBgWorker *rw)
 			(errmsg("unregistering background worker: %s",
 				rw->rw_worker.bgw_name)));
 
-	slist_delete(&BackgroundWorkerList, &rw->rw_lnode);
+	slist_delete_current(cur);
 	free(rw);
 }
 
diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c
index e539bac0c1d..a52377fc9db 100644
--- a/src/backend/postmaster/pgstat.c
+++ b/src/backend/postmaster/pgstat.c
@@ -3602,6 +3602,13 @@ pgstat_write_statsfiles(bool permanent, bool allDbs)
 	{
 		slist_mutable_iter iter;
 
+		/*
+		 * Strictly speaking we should do slist_delete_current() before
+		 * freeing each request struct.  We skip that and instead
+		 * re-initialize the list header at the end.  Nonetheless, we must use
+		 * slist_foreach_modify, not just slist_foreach, since we will free
+		 * the node's storage before advancing.
+		 */
 		slist_foreach_modify(iter, &last_statrequests)
 		{
 			DBWriteRequest *req;
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index 7894217ed52..e6d750d4d38 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -1452,7 +1452,7 @@ DetermineSleepTime(struct timeval * timeout)
 
 			if (rw->rw_worker.bgw_restart_time == BGW_NEVER_RESTART)
 			{
-				ForgetBackgroundWorker(rw);
+				ForgetBackgroundWorker(&siter);
 				continue;
 			}
 
@@ -5641,7 +5641,7 @@ StartOneBackgroundWorker(void)
 		{
 			if (rw->rw_worker.bgw_restart_time == BGW_NEVER_RESTART)
 			{
-				ForgetBackgroundWorker(rw);
+				ForgetBackgroundWorker(&iter);
 				continue;
 			}
 
diff --git a/src/include/lib/ilist.h b/src/include/lib/ilist.h
index 73f41159a6c..734ef70aa1c 100644
--- a/src/include/lib/ilist.h
+++ b/src/include/lib/ilist.h
@@ -211,9 +211,14 @@ typedef struct slist_head
  * Used as state in slist_foreach(). To get the current element of the
  * iteration use the 'cur' member.
  *
- * Do *not* manipulate the list while iterating!
- *
- * NB: this wouldn't really need to be an extra struct, we could use a
+ * It's allowed to modify the list while iterating, with the exception of
+ * deleting the iterator's current node; deletion of that node requires
+ * care if the iteration is to be continued afterward.	(Doing so and also
+ * deleting or inserting adjacent list elements might misbehave; also, if
+ * the user frees the current node's storage, continuing the iteration is
+ * not safe.)
+ *
+ * NB: this wouldn't really need to be an extra struct, we could use an
  * slist_node * directly. We prefer a separate type for consistency.
  */
 typedef struct slist_iter
@@ -224,15 +229,18 @@ typedef struct slist_iter
 /*
  * Singly linked list iterator allowing some modifications while iterating.
  *
- * Used as state in slist_foreach_modify().
+ * Used as state in slist_foreach_modify(). To get the current element of the
+ * iteration use the 'cur' member.
  *
- * Iterations using this are allowed to remove the current node and to add
- * more nodes ahead of the current node.
+ * The only list modification allowed while iterating is to remove the current
+ * node via slist_delete_current() (*not* slist_delete()).	Insertion or
+ * deletion of nodes adjacent to the current node would misbehave.
  */
 typedef struct slist_mutable_iter
 {
 	slist_node *cur;			/* current element */
 	slist_node *next;			/* next node we'll iterate to */
+	slist_node *prev;			/* prev node, for deletions */
 } slist_mutable_iter;
 
 
@@ -243,7 +251,7 @@ typedef struct slist_mutable_iter
 
 /* Prototypes for functions too big to be inline */
 
-/* Caution: this is O(n) */
+/* Caution: this is O(n); consider using slist_delete_current() instead */
 extern void slist_delete(slist_head *head, slist_node *node);
 
 #ifdef ILIST_DEBUG
@@ -578,6 +586,7 @@ extern slist_node *slist_pop_head_node(slist_head *head);
 extern bool slist_has_next(slist_head *head, slist_node *node);
 extern slist_node *slist_next_node(slist_head *head, slist_node *node);
 extern slist_node *slist_head_node(slist_head *head);
+extern void slist_delete_current(slist_mutable_iter *iter);
 
 /* slist macro support function */
 extern void *slist_head_element_off(slist_head *head, size_t off);
@@ -679,6 +688,29 @@ slist_head_node(slist_head *head)
 {
 	return (slist_node *) slist_head_element_off(head, 0);
 }
+
+/*
+ * Delete the list element the iterator currently points to.
+ *
+ * Caution: this modifies iter->cur, so don't use that again in the current
+ * loop iteration.
+ */
+STATIC_IF_INLINE void
+slist_delete_current(slist_mutable_iter *iter)
+{
+	/*
+	 * Update previous element's forward link.  If the iteration is at the
+	 * first list element, iter->prev will point to the list header's "head"
+	 * field, so we don't need a special case for that.
+	 */
+	iter->prev->next = iter->next;
+
+	/*
+	 * Reset cur to prev, so that prev will continue to point to the prior
+	 * valid list element after slist_foreach_modify() advances to the next.
+	 */
+	iter->cur = iter->prev;
+}
 #endif   /* PG_USE_INLINE || ILIST_INCLUDE_DEFINITIONS */
 
 /*
@@ -706,7 +738,12 @@ slist_head_node(slist_head *head)
  *
  * Access the current element with iter.cur.
  *
- * It is *not* allowed to manipulate the list during iteration.
+ * It's allowed to modify the list while iterating, with the exception of
+ * deleting the iterator's current node; deletion of that node requires
+ * care if the iteration is to be continued afterward.	(Doing so and also
+ * deleting or inserting adjacent list elements might misbehave; also, if
+ * the user frees the current node's storage, continuing the iteration is
+ * not safe.)
  */
 #define slist_foreach(iter, lhead)											\
 	for (AssertVariableIsOfTypeMacro(iter, slist_iter),						\
@@ -720,15 +757,18 @@ slist_head_node(slist_head *head)
  *
  * Access the current element with iter.cur.
  *
- * Iterations using this are allowed to remove the current node and to add
- * more nodes ahead of the current node.
+ * The only list modification allowed while iterating is to remove the current
+ * node via slist_delete_current() (*not* slist_delete()).	Insertion or
+ * deletion of nodes adjacent to the current node would misbehave.
  */
 #define slist_foreach_modify(iter, lhead)									\
 	for (AssertVariableIsOfTypeMacro(iter, slist_mutable_iter),				\
 		 AssertVariableIsOfTypeMacro(lhead, slist_head *),					\
-		 (iter).cur = (lhead)->head.next,									\
+		 (iter).prev = &(lhead)->head,										\
+		 (iter).cur = (iter).prev->next,									\
 		 (iter).next = (iter).cur ? (iter).cur->next : NULL;				\
 		 (iter).cur != NULL;												\
+		 (iter).prev = (iter).cur,											\
 		 (iter).cur = (iter).next,											\
 		 (iter).next = (iter).next ? (iter).next->next : NULL)
 
diff --git a/src/include/postmaster/bgworker_internals.h b/src/include/postmaster/bgworker_internals.h
index 6484cfb7a6b..478a1fff0db 100644
--- a/src/include/postmaster/bgworker_internals.h
+++ b/src/include/postmaster/bgworker_internals.h
@@ -39,7 +39,7 @@ extern slist_head BackgroundWorkerList;
 extern Size BackgroundWorkerShmemSize(void);
 extern void BackgroundWorkerShmemInit(void);
 extern void BackgroundWorkerStateChange(void);
-extern void ForgetBackgroundWorker(RegisteredBgWorker *);
+extern void ForgetBackgroundWorker(slist_mutable_iter *cur);
 
 #ifdef EXEC_BACKEND
 extern BackgroundWorker *BackgroundWorkerEntry(int slotno);