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

diff --git a/src/backend/lib/ilist.c b/src/backend/lib/ilist.c
index 957a11812277b9991e6a0f6a82912e7c33532c6d..eb23fa1c687218eb6f2ddfc6bccef10c9422331d 100644 (file)
--- 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/pgstat.c b/src/backend/postmaster/pgstat.c
index ac20dffd9881953135e45c00ff2d08a7cbeca7c0..dac5bca78aab80dc306fcd97da1864357576ef6b 100644 (file)
--- a/src/backend/postmaster/pgstat.c
+++ b/src/backend/postmaster/pgstat.c
@@ -3598,6 +3598,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/include/lib/ilist.h b/src/include/lib/ilist.h
index 73f41159a6cd2c0632bbdf910e4e59e01884c746..734ef70aa1c772cd761fae53cec3ffb86d455dab 100644 (file)
--- 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)