Re: embedded list

From: Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: Andres Freund <andres(at)2ndquadrant(dot)com>, pgsql-hackers(at)postgresql(dot)org, Peter Geoghegan <peter(at)2ndquadrant(dot)com>, Robert Haas <robertmhaas(at)gmail(dot)com>
Subject: Re: embedded list
Date: 2012-10-18 20:39:29
Message-ID: 20121018203929.GS3763@alvh.no-ip.org
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Tom Lane escribió:
> Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> writes:
> > Here's the final version. I think this is ready to go in.
>
> I got around to reviewing this today. I'm pretty seriously annoyed at
> the definition of dlist_delete: it should *not* require the list header.
> The present coding simply throws away one of the primary advantages of
> a doubly-linked list over a singly-linked list, namely that you don't
> have to have your hands on the list header in order to unlink a node.
> This isn't merely academic either, as I see that the patch to catcache
> code actually added a field to struct catctup to support making the
> list header available. That's a complete waste of 8 bytes (on a 64-bit
> machine) per catalog cache entry. The only thing it buys for us is
> the ability to run dlist_check, which is something that isn't even
> compiled (not even in an Assert build), and which doesn't actually do
> that much useful even if it is compiled --- for instance, there's no way
> to verify that the nodes were actually in the list claimed.

Oops. I mentioned this explicitely somewhere in the discussion. I
assumed you had seen that, and that you would have complained had you
found it objectionable. (It's hard enough to figure out if people don't
respond because they don't have a problem with something, or just
because they didn't see it.)

On the other hand, it's convenient to remove them, because in
predicate.c there are plenty of SHM_QUEUE node removals which is clearly
easier to port over to dlist if we don't have to figure out exactly
which list each node is in. (Maybe in other SHM_QUEUE users as well,
but that's the most complex of the bunch.)

> I think we should remove the head argument at least from dlist_delete,
> and probably also dlist_insert_after and dlist_insert_before.

There are more functions that get the list head just to run the check.
Can I assume that you don't propose removing the argument from those?
(dlist_next_node, dlist_prev_node I think are the only ones).

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Boszormenyi Zoltan 2012-10-18 20:40:15 Re: [PATCH] lock_timeout and common SIGALRM framework
Previous Message Claudio Freire 2012-10-18 20:30:26 Prefetch index pages for B-Tree index scans