On Thu, Feb 20, 2020 at 5:14 PM Andres Freund <andres@anarazel.de> wrote:
> 16 files changed, 569 insertions(+), 1053 deletions(-)
Nice!
Some comments on 0001, 0003, 0004:
> Subject: [PATCH v1 1/6] Add additional prev/next & detached node functions to
+extern void dlist_check(const dlist_head *head);
+extern void slist_check(const slist_head *head);
I approve of the incidental constification in this patch.
+/*
+ * Like dlist_delete(), but also sets next/prev to NULL to signal not being in
+ * 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;
+}
Instead of introducing this strange terminology, why not just have the
callers do ...
dlist_delete(node);
dlist_node_init(node);
..., or perhaps supply dlist_delete_and_reinit(node) that does exactly
that? That is, reuse the code and terminology.
+/*
+ * 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().
+ */
+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;
+}
How about dlist_node_is_linked()? I don't like introducing random new
verbs when we already have 'linked' in various places, and these
things are, y'know, linked lists.
> Subject: [PATCH v1 3/6] Use dlist for syncrep queue.
LGTM.
> Subject: [PATCH v1 4/6] Use dlists for predicate locking.
+ dlist_foreach(iter, &unconstify(SERIALIZABLEXACT *, reader)->outConflicts)
Yuck... I suppose you could do this:
- dlist_foreach(iter, &unconstify(SERIALIZABLEXACT *, reader)->outConflicts)
+ dlist_foreach_const(iter, &reader->outConflicts)
... given:
+/* Variant for when you have a pointer to const dlist_head. */
+#define dlist_foreach_const(iter, lhead) \
+ for (AssertVariableIsOfTypeMacro(iter, dlist_iter), \
+ AssertVariableIsOfTypeMacro(lhead, const dlist_head *), \
+ (iter).end = (dlist_node *) &(lhead)->head, \
+ (iter).cur = (iter).end->next ? (iter).end->next : (iter).end; \
+ (iter).cur != (iter).end; \
+ (iter).cur = (iter).cur->next)
+
... or find a way to make dlist_foreach() handle that itself, which
seems pretty reasonable given its remit to traverse lists without
modifying them, though perhaps that would require a different iterator
type...
Otherwise looks OK to me and passes various tests I threw at it.