From: | David Rowley <dgrowleyml(at)gmail(dot)com> |
---|---|
To: | Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> |
Cc: | PostgreSQL Developers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
Subject: | Re: Adding doubly linked list type which stores the number of items in the list |
Date: | 2022-10-31 02:56:28 |
Message-ID: | CAApHDvoevXLsGGuYWTX0AAcWf=7X0nanSxYHe_thc0z=_WziVw@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Thank you for having another look at this
On Sat, 29 Oct 2022 at 18:32, Bharath Rupireddy
<bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
> 1. I guess we need to cast the 'node' parameter too, something like
> below? I'm reading the comment there talking about compilers
> complaning about the unused function arguments.
> dlist_member_check(head, node) ((void) (head); (void) (node);)
I looked at dlist_check() and I didn't quite manage to figure out why
the cast is needed. As far as I can see, there are no calls where we
only pass dlist_head solely for the dlist_check(). For
dlist_member_check(), dlist_delete_from() does not use the 'head'
parameter for anything apart from dlist_member_check(), so I believe
the cast is required for 'head'. I think I'd rather only add the cast
for 'node' unless we really require it. Cargo-culting it in there just
because that's what the other macros do does not seem like a good idea
to me.
> Can we put max limit, at least in assert, something like below, on
> 'count' instead of saying above? I'm not sure if there's someone
> storing 4 billion items, but it will be a good-to-have safety from the
> data structure perspective if others think it's not an overkill.
> Assert(head->count > 0 && head->count <= PG_UINT32_MAX);
'count' is a uint32. It's always going to be <= PG_UINT32_MAX.
My original thoughts were that it seems unlikely we'd ever give an
assert build a workload that would ever have 2^32 dlist_nodes in
dclist. Having said that, perhaps it would do no harm to add some
overflow checks to 'count'. I've gone and added some
Assert(head->count > 0) after we do count++.
> + * As dlist_delete but performs checks in ILIST_DEBUG to ensure that 'node'
> + * belongs to 'head'.
> I think 'Same as dlist_delete' instead of just 'As dlist_delete'
I don't really see what's wrong with this. We use "As above" when we
mean "Same as above" in many locations. Anyway, I don't feel strongly
about not adding the word, so I've adjusted the wording in that
comment which includes adding the word "Same" at the start.
> 5.
> + * Caution: 'node' must be a member of 'head'.
> + * Caller must ensure that 'before' is a member of 'head'.
> Can we have the same comments around something like below?
I've adjusted dclist_insert_after() and dclist_insert_before(). Each
dclist function that uses dlist_member_check() now has the same text.
> 8. Wondering if we need dlist_delete_from() at all. Can't we just add
> dlist_member_check() dclist_delete_from() and call dlist_delete()
> directly?
Certainly, but I made it that way on purpose. I wanted dclist to have
a superset of the functions that dlist has. I just see no reason why
dlist shouldn't have dlist_delete_from() when dclist has it.
I've attached the v3 version of the patch which includes some
additional polishing work.
David
Attachment | Content-Type | Size |
---|---|---|
v3_add_doubly_linked_count_lists.patch | text/plain | 32.0 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | shiy.fnst@fujitsu.com | 2022-10-31 03:20:25 | RE: Segfault on logical replication to partitioned table with foreign children |
Previous Message | Japin Li | 2022-10-31 02:48:01 | Lock on ShmemVariableCache fields? |