From: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
---|---|
To: | Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> |
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:15:47 |
Message-ID: | 4201.1350591347@sss.pgh.pa.us |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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.
I think we should remove the head argument at least from dlist_delete,
and probably also dlist_insert_after and dlist_insert_before.
regards, tom lane
From | Date | Subject | |
---|---|---|---|
Next Message | Alvaro Herrera | 2012-10-18 20:22:06 | Re: Skip checkpoint on promoting from streaming replication |
Previous Message | Alvaro Herrera | 2012-10-18 19:58:20 | Re: foreign key locks |