Hi,
In [1] Robert justifiably complained about the use of PROC_QUEUE. I've
previously been bothered by this in [2], but didn't get around to finishing
the patches.
One thing that made me hesitate was the naming of the function for a) checking
whether a dlist_node is a list, b) initializing and deleting nodes in a way to
allow for a). My patch adds dlist_node_init(), dlist_delete_thoroughly() /
dlist_delete_from_thoroughly() / dclist_delete_from_thoroughly() and
dlist_node_is_detached(). Thomas proposed dlist_delete_and_reinit() and
dlist_node_is_linked() instead.
Attached is a revised version of the patches from [2].
I left the naming of the detached / thoroughly as it was before, for
now. Another alternative could be to try to just get rid of needing the
detached state at all, although that likely would make the code changes
bigger.
I've switched the PROC_QUEUE uses to dclist, which we only got recently. The
prior version of the patchset contained a patch to remove the use of the size
field of PROC_QUEUE, as it's only needed in a few places. But it seems easier
to just replace it with dclist for now.
Robert had previously complained about the ilist.h patch constifying some
functions. I don't really understand the complaint in this case - none of the
cases should require constifying outside code. It just allows to replace
things like SHMQueueEmpty() which were const, because there's a few places
that get passed a const PGPROC. There's more that could be constified
(removing the need for one unconstify() in predicate.c - but that seems like a
lot more work with less clear benefit. Either way, I split the constification
into a separate patch.
Comments?
Greetings,
Andres Freund
[1]
https://www.postgresql.org/message-id/20221117201304.vemjfsxaizmt3zbb%40awork3.anarazel.de
[2]
https://www.postgresql.org/message-id/20200211042229.msv23badgqljrdg2%40alap3.anarazel.de
>From e5efdebf82e8e857a21e675bcdccd3c129d16ab2 Mon Sep 17 00:00:00 2001
From: Andres Freund
Date: Sat, 19 Nov 2022 14:38:02 -0800
Subject: [PATCH v2 1/9] Add detached node functions to ilist
These allow to test whether an element is in a list by checking whether
prev/next are NULL. This is needed to replace SHM_QUEUE uses with ilist.h
---
src/include/lib/ilist.h | 63 +
1 file changed, 63 insertions(+)
diff --git a/src/include/lib/ilist.h b/src/include/lib/ilist.h
index 3c543e7c365..a1a4abf0609 100644
--- a/src/include/lib/ilist.h
+++ b/src/include/lib/ilist.h
@@ -316,6 +316,17 @@ dlist_init(dlist_head *head)
head->head.next = head->head.prev = >head;
}
+/*
+ * Initialize a doubly linked list element.
+ *
+ * This is only needed when dlist_node_is_detached() may be needed.
+ */
+static inline void
+dlist_node_init(dlist_node *node)
+{
+ node->next = node->prev = NULL;
+}
+
/*
* Is the list empty?
*
@@ -397,6 +408,19 @@ dlist_delete(dlist_node *node)
node->next->prev = node->prev;
}
+/*
+ * Like dlist_delete(), but also sets next/prev to NULL to signal not being in
+ * a list.
+ */
+static inline void
+dlist_delete_thoroughly(dlist_node *node)
+{
+ node->prev->next = node->next;
+ node->next->prev = node->prev;
+ node->next = NULL;
+ node->prev = NULL;
+}
+
/*
* Same as dlist_delete, but performs checks in ILIST_DEBUG builds to ensure
* that 'node' belongs to 'head'.
@@ -408,6 +432,17 @@ dlist_delete_from(dlist_head *head, dlist_node *node)
dlist_delete(node);
}
+/*
+ * Like dlist_delete_from, but also sets next/prev to NULL to signal not
+ * being in a list.
+ */
+static inline void
+dlist_delete_from_thoroughly(dlist_head *head, dlist_node *node)
+{
+ dlist_member_check(head, node);
+ dlist_delete_thoroughly(node);
+}
+
/*
* Remove and return the first node from a list (there must be one).
*/
@@ -480,6 +515,21 @@ dlist_has_prev(dlist_head *head, dlist_node *node)
return node->prev != >head;
}
+/*
+ * Check if node is detached. A node is only detached if it either has been
+ * initialized with dlist_init_node(), or deleted with
+ * dlist_delete_thoroughly() / dlist_delete_from_thoroughly() /
+ * dclist_delete_from_thoroughly().
+ */
+static inline bool
+dlist_node_is_detached(const dlist_node *node)
+{
+ Assert((node->next == NULL && node->prev == NULL) ||
+ (node->next != NULL && node->prev != NULL));
+
+ return node->next == NULL;
+}
+
/*
* Return the next node in the list (there must be one).
*/
@@ -718,6 +768,19 @@ dclist_delete_from(dclist_head *head, dlist_node *node)
head->count--;
}
+/*
+ * Like dclist_delete_from(), but also sets next/prev to NULL to signal not
+ * being in a list.
+ */
+static inline void
+dclist_delete_from_thoroughly(dclist_head *head, dlist_node *node)
+{
+ Assert(head->count > 0);
+
+ dlist_delete_from_thoroughly(>dlist, node);
+ head->count--;
+}
+
/*
* dclist_pop_head_node
* Remove and return the first node from a list (there must be one).
--
2.38.0
>From 5266ce81a47c75e5c8d42c54ef60bf0afcaed755 Mon
