Re: Improve heavyweight locks instead of building new lock managers? - Mailing list pgsql-hackers

From Thomas Munro
Subject Re: Improve heavyweight locks instead of building new lock managers?
Date
Msg-id CA+hUKGL8kAotpMdTGEcZUUt+ZsfevStK5TLLtQTiW839LdXq2Q@mail.gmail.com
Whole thread Raw
In response to Re: Improve heavyweight locks instead of building new lock managers?  (Andres Freund <andres@anarazel.de>)
Responses Re: Improve heavyweight locks instead of building new lock managers?
List pgsql-hackers
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.



pgsql-hackers by date:

Previous
From: Alvaro Herrera
Date:
Subject: Re: PATCH: Add uri percent-encoding for binary data
Next
From: Tom Lane
Date:
Subject: Re: pgsql: Add kqueue(2) support to the WaitEventSet API.