Re: Improve heavyweight locks instead of building new lock managers?

On Thu, Feb 20, 2020 at 5:14 PM Andres Freund <andres(at)anarazel(dot)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.