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

Hi,

On 2020-02-21 12:40:06 +1300, Thomas Munro wrote:
> On Thu, Feb 20, 2020 at 5:14 PM Andres Freund <andres@anarazel.de> wrote:
> >  16 files changed, 569 insertions(+), 1053 deletions(-)
> 
> Nice!

Thanks!


> 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.

It was just necessary fallout :)


> +/*
> + * 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);

There's quite a few callers in predicate.c - I actually did that first.


> ..., or perhaps supply dlist_delete_and_reinit(node) that does exactly
> that?  That is, reuse the code and terminology.

Yea, that's might be better, but see paragraph below.  I quite dislike
adding any "empty node" state.


> +/*
> + * 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.

Well, but that doesn't signal that you can't just delete and have
dlist_node_is_linked() work. I *want* it to sound "different".  We could
of course make delete always do this, but I don't want to add that
overhead unnecessarily.


> > Subject: [PATCH v1 4/6] Use dlists for predicate locking.
> 
> +    dlist_foreach(iter, &unconstify(SERIALIZABLEXACT *, reader)->outConflicts)
> 
> Yuck...

It doesn't seem *that* bad to me, at least signals properly what we're
doing, and only does so in one place.


> I suppose you could do this:
> 
> -    dlist_foreach(iter, &unconstify(SERIALIZABLEXACT *, reader)->outConflicts)
> +    dlist_foreach_const(iter, &reader->outConflicts)

We'd need a different iterator type too, I think? Because the iterator
itself can't be constant, but we'd want the elements themselves be
pointers to constants.

const just isn't a very granular thing in C :(.


> ... 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...

I was trying that first, but I don't easily see how we can do
so. Iterating over a non-constant list with dlist_foreach obviously
still allows to to manipulate the list members. Thus dlist_iter.cur
can't be a 'pointer to const'.  Whereas that's arguably what'd be needed
for a correct dlist_foreach() of a constant list?

We could just accept const pointers for dlist_foreach(), but then we'd
*always* accept them, and we'd thus unconditionally have iter.cur as
non-const.  Would that be better?


> Otherwise looks OK to me and passes various tests I threw at it

Thanks!

Greetings,

Andres Freund