Thread: Adding doubly linked list type which stores the number of items in the list

As part of the AIO work [1], there are quite a number of dlist_heads
which a counter is used to keep track on how many items are in the
list.  We also have a few places in master which do the same thing.

In order to tidy this up and to help ensure that the count variable
does not get out of sync with the items which are stored in the list,
how about we introduce "dclist" which maintains the count for us?

I've attached a patch which does this.  The majority of the functions
for the new type are just wrappers around the equivalent dlist
function.

dclist provides all of the functionality that dlist does except
there's no dclist_delete() function.  dlist_delete() can be done by
just knowing the element to delete and not the list that the element
belongs to.  With dclist, that's not possible as we must also subtract
1 from the count variable and obviously we need the dclist_head for
that.

I'll add this to the November commitfest.

David

[1] https://www.postgresql.org/message-id/flat/20210223100344.llw5an2aklengrmn@alap3.anarazel.de

On Thu, Oct 27, 2022 at 9:05 AM David Rowley <dgrowleyml@gmail.com> wrote:
>
> As part of the AIO work [1], there are quite a number of dlist_heads
> which a counter is used to keep track on how many items are in the
> list.  We also have a few places in master which do the same thing.
>
> In order to tidy this up and to help ensure that the count variable
> does not get out of sync with the items which are stored in the list,
> how about we introduce "dclist" which maintains the count for us?
>
> I've attached a patch which does this.  The majority of the functions
> for the new type are just wrappers around the equivalent dlist
> function.
>
> dclist provides all of the functionality that dlist does except
> there's no dclist_delete() function.  dlist_delete() can be done by
> just knowing the element to delete and not the list that the element
> belongs to.  With dclist, that's not possible as we must also subtract
> 1 from the count variable and obviously we need the dclist_head for
> that.
>
> [1] https://www.postgresql.org/message-id/flat/20210223100344.llw5an2aklengrmn@alap3.anarazel.de

+1. Using dlist_head in dclist_head enables us to reuse dlist_* functions.

Some comments on the patch:
1. I think it's better to just return dlist_is_empty(&head->dlist) &&
(head->count == 0); from dclist_is_empty() and remove the assert for
better readability and safety against count being zero.

2. Missing dlist_is_memberof() in dclist_delete_from()?

3. Just thinking if we need to move dlist_is_memberof() check from
dclist_* functions to dlist_* functions, because they also need such
insurance against callers passing spurious nodes.

4. More opportunities to use dclist_* in below places, no?
    dlist_push_tail(&src->mappings, &pmap->node);
    src->num_mappings++;

    dlist_push_head(&MXactCache, &entry->node);
    if (MXactCacheMembers++ >= MAX_CACHE_ENTRIES)

5. dlist_is_memberof() - do we need this at all? We trust the callers
of dlist_* today that the passed in node belongs to the list, no?

6. If we decide to have dlist_is_memberof() and the asserts around it,
can we design it on the similar lines as dlist_check() to avoid many
#ifdef ILIST_DEBUG #endif blocks spreading around the code?

7. Do we need Assert(head->count > 0); in more places like dclist_delete_from()?

8. Don't we need dlist_container(), dlist_head_element(),
dlist_tail_element() for dclist_*? Even though, we might not use them
immediately, just for the sake for completeness of dclist data
structure.

> I'll add this to the November commitfest.

Yes, please.

-- 
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

Thank you for having a look at this.

On Thu, 27 Oct 2022 at 19:32, Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
> Some comments on the patch:
> 1. I think it's better to just return dlist_is_empty(&head->dlist) &&
> (head->count == 0); from dclist_is_empty() and remove the assert for
> better readability and safety against count being zero.

I don't think that's a good change. For 1) it adds unnecessary
overhead due to the redundant checks and 2) it removes the Assert
which is our early warning that the dclist's count is getting out of
sync somewhere.

> 2. Missing dlist_is_memberof() in dclist_delete_from()?

I put that in dlist_delete_from() which is called from dclist_delete_from().

> 3. Just thinking if we need to move dlist_is_memberof() check from
> dclist_* functions to dlist_* functions, because they also need such
> insurance against callers passing spurious nodes.

I think the affected functions there would be; dlist_move_head(),
dlist_move_tail(), dlist_has_next(), dlist_has_prev(),
dlist_next_node() and dlist_prev_node(). I believe if we did that then
it's effectively an API change. The comments only claim that it's
undefined if node is not a member of the list. It does not say 'node'
*must* be part of the list.  Now, perhaps doing this would just make
it more likely that we'd find bugs in our code and extension authors
would find bugs in their code, but it does move the bar.
dlist_move_head and dlist_move_tail look like they'd work perfectly
well to remove an item from 1 list and put it on the head or tail of
some completely different list. Should we really be changing that in a
patch that is meant to just add the dclist type?

> 4. More opportunities to use dclist_* in below places, no?
>     dlist_push_tail(&src->mappings, &pmap->node);
>     src->num_mappings++;
>
>     dlist_push_head(&MXactCache, &entry->node);
>     if (MXactCacheMembers++ >= MAX_CACHE_ENTRIES)

Thanks for finding those. I've adjusted them both to use dclists.

> 5. dlist_is_memberof() - do we need this at all? We trust the callers
> of dlist_* today that the passed in node belongs to the list, no?

hmm, this seems to contradict your #3?

If you look at something like dlist_move_head(), if someone calls that
and passes a 'node' that does not belong to 'head' then the result of
that is that we delete 'node' from whichever dlist that it's on and
push it onto 'head'. Nothing bad happens there.  If we do the same on
a dclist then the count gets out of sync. That's bad as it could lead
to assert failures and bugs.

> 6. If we decide to have dlist_is_memberof() and the asserts around it,
> can we design it on the similar lines as dlist_check() to avoid many
> #ifdef ILIST_DEBUG #endif blocks spreading around the code?

OK, that likely is a better idea. I've done this in the attached by
way of dlist_member_check()

> 7. Do we need Assert(head->count > 0); in more places like dclist_delete_from()?

I guess it does no harm. I've added some additional ones in the attached.

> 8. Don't we need dlist_container(), dlist_head_element(),
> dlist_tail_element() for dclist_*? Even though, we might not use them
> immediately, just for the sake for completeness of dclist data
> structure.

OK, I think I'd left those because dclist_container() would just be
the same as dlist_container(), but that's not the case for the other
two, so I've added all 3.

One additional change is that I also ended up removing the use of
dclist that I had in the previous patch for ReorderBufferTXN.subtxns.
Looking more closely at the code in ReorderBufferAssignChild():

/*
* We already saw this transaction, but initially added it to the
* list of top-level txns.  Now that we know it's not top-level,
* remove it from there.
*/
dlist_delete(&subtxn->node);

The problem is that since ReorderBufferTXN is used for both
transactions and sub-transactions that it's not easy to determine if
the ReorderBufferTXN.node is part of the ReorderBuffer.toplevel_by_lsn
dlist or the ReorderBufferTXN.subtxns.  It seems safer just to leave
this one alone.

David

On Fri, Oct 28, 2022 at 11:01 AM David Rowley <dgrowleyml@gmail.com> wrote:
>
> > 3. Just thinking if we need to move dlist_is_memberof() check from
> > dclist_* functions to dlist_* functions, because they also need such
> > insurance against callers passing spurious nodes.
>
> I think the affected functions there would be; dlist_move_head(),
> dlist_move_tail(), dlist_has_next(), dlist_has_prev(),
> dlist_next_node() and dlist_prev_node(). I believe if we did that then
> it's effectively an API change. The comments only claim that it's
> undefined if node is not a member of the list. It does not say 'node'
> *must* be part of the list.  Now, perhaps doing this would just make
> it more likely that we'd find bugs in our code and extension authors
> would find bugs in their code, but it does move the bar.
> dlist_move_head and dlist_move_tail look like they'd work perfectly
> well to remove an item from 1 list and put it on the head or tail of
> some completely different list. Should we really be changing that in a
> patch that is meant to just add the dclist type?

Hm. Let's not touch that here.

Thanks for the patch. Few more comments on v2:
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);)

2.
+ * maximum of UINT32 elements.  It is up to the caller to ensure no more than
+ * this many items are added to a dclist.
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);

3. I guess, we can split up the patches for the ease of review, 0001
introducing dclist_* data structure and 0002 using it. It's not
mandatory though. The two patches can go separately if needed.

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

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?
+ * Caller must ensure that 'node' must be a member of 'head'.
+ * Caller must ensure that 'before' is a member of 'head'.
or
+ * Caution: 'node' must be a member of 'head'.
+ * Caution: 'before' must be a member of 'head'.
or
 * Caution: unreliable if 'node' is not in the list.
 * Caution: unreliable if 'before' is not in the list.

6.
+dclist_has_prev(dclist_head *head, dlist_node *node)
+{
+    dlist_member_check(&head->dlist, node);
+
+    Assert(head->count > 0);

+    Assert(head->count > 0);
+
+    return (dlist_node *) dlist_head_element_off(&head->dlist, 0);

+    Assert(head->count > 0);
+
+    return (dlist_node *) dlist_tail_element_off(&head->dlist, 0);

+    Assert(!dclist_is_empty(head));
+    return (char *) head->dlist.head.next - off;

+    dlist_member_check(&head->dlist, node);
+
+    Assert(head->count > 0);
+
+    return dlist_has_prev(&head->dlist, node);

+    dlist_member_check(&head->dlist, node);
+    Assert(head->count > 0);
+
+    return dlist_has_next(&head->dlist, node);

Remove extra lines in between and have them uniformly across,
something like below?

dlist_member_check();
Assert();

return XXX;

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?

--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

Thank you for having another look at this

On Sat, 29 Oct 2022 at 18:32, Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.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

On Mon, Oct 31, 2022 at 8:26 AM David Rowley <dgrowleyml@gmail.com> wrote:
>
> 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.

Hm, you're right, dlist_member_check() needs it. Also, slist_check()
needs it for slist_has_next(). dlist_check() doesn't need it, however,
keeping it intact doesn't harm, I guess.

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

So, when an overflow occurs, the head->count wraps around after
PG_UINT32_MAX, meaning, becomes 0 and we will catch it in an assert
build. This looks reasonable to me. However, the responsibility lies
with the developers to deal with such overflows.

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

Okay.

> I've attached the v3 version of the patch which includes some
> additional polishing work.

Thanks. The v3 patch looks good to me.

BTW, do we need sclist_* as well? AFAICS, no such use-case exists
needing slist and element count, maybe we don't need.

I'm wondering if adding count to dlist_head and maintaining it as part
of the existing dlist_* data structure and functions is any better
than introducing dclist_*? In that case, we need only one function,
dlist_count, no? Or do we choose to go dclist_* because we want to
avoid the extra cost of maintaining count within dlist_*? If yes, is
maintaining count in dlist_* really costly?

--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

On Mon, 31 Oct 2022 at 19:05, Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
> So, when an overflow occurs, the head->count wraps around after
> PG_UINT32_MAX, meaning, becomes 0 and we will catch it in an assert
> build. This looks reasonable to me. However, the responsibility lies
> with the developers to deal with such overflows.

I'm not sure what the alternatives are here.  One of the advantages of
dlist over List is that there are no memory allocations to add a node
which is already allocated onto a list.  lappend() might need to
enlarge the list, which means you can't do that in a critical section.
It's currently OK to add an item to a dlist in a critical section,
however, if we add an elog(ERROR) then it won't be.  The best I think
we can do is to just let the calling code ensure that it only uses
dlist when it's certain that there can't be more than 2^32 items to
store at once.

Additionally, everywhere I've replaced dlist with dclist in the patch
used either an int or uint32 for the counter.  There was no code which
checked if the existing counter had wrapped.

> Thanks. The v3 patch looks good to me.

Great. Thanks for having a look.

> BTW, do we need sclist_* as well? AFAICS, no such use-case exists
> needing slist and element count, maybe we don't need.

I don't see anywhere that requires it.

> I'm wondering if adding count to dlist_head and maintaining it as part
> of the existing dlist_* data structure and functions is any better
> than introducing dclist_*? In that case, we need only one function,
> dlist_count, no? Or do we choose to go dclist_* because we want to
> avoid the extra cost of maintaining count within dlist_*? If yes, is
> maintaining count in dlist_* really costly?

I have a few reasons for not wanting to do that:

1) I think dlist operations are very fast at the moment.  The fact
that the functions are static inline tells me the function call
overhead matters. Therefore, it's likely maintaining a count also
matters.
2) Code bloat.  The functions are static inline. That means all
compiled code that adds or removes an item from a dlist would end up
larger. That results in more instruction cache misses.
3) I've no reason to believe that all call sites that do
dlist_delete() have the ability to know which list the node is on.
Just look at ReorderBufferAssignChild().  I decided to not convert the
subtxns dlist into a dclist as the subtransaction sometimes seems to
go onto the top transaction list before it's moved to the sub-txn's
list.
4) There's very little or no scope for bugs in dclist relating to the
dlist implementation as all that stuff is done by just calling the
dlist_* functions.  The only scope is really that it could call the
wrong dlist_* function.  It does not seem terribly hard to ensure we
don't write any bugs like that.

David

On Mon, Oct 31, 2022 at 12:44 PM David Rowley <dgrowleyml@gmail.com> wrote:
>
> On Mon, 31 Oct 2022 at 19:05, Bharath Rupireddy
> <bharath.rupireddyforpostgres@gmail.com> wrote:
> > So, when an overflow occurs, the head->count wraps around after
> > PG_UINT32_MAX, meaning, becomes 0 and we will catch it in an assert
> > build. This looks reasonable to me. However, the responsibility lies
> > with the developers to deal with such overflows.
>
> I'm not sure what the alternatives are here.  One of the advantages of
> dlist over List is that there are no memory allocations to add a node
> which is already allocated onto a list.  lappend() might need to
> enlarge the list, which means you can't do that in a critical section.

Using uint64 is one option to allow many elements, however, I'm also
fine with removing the overflow assertions Assert(head->count > 0);
/* count overflow check */ altogether and let the callers take the
responsibility. dlist_* and dclist_* callers already have another
responsibility - Caution: 'foo' must be a member of 'head'.

> It's currently OK to add an item to a dlist in a critical section,
> however, if we add an elog(ERROR) then it won't be.

dlist_check() and dlist_member_check() have an elog(ERROR) and the
above statement isn't true in case of ILIST_DEBUG-defined builds.

> The best I think
> we can do is to just let the calling code ensure that it only uses
> dlist when it's certain that there can't be more than 2^32 items to
> store at once.

Right.

+ * able to store a maximum of PG_UINT32_MAX elements.  It is up to the caller
+ * to ensure no more than this many items are added to a dclist.

The above comment seems fine to me, if we really want to enforce any
overflow checks on non-debug, non-assert builds, it might add some
costs to dclist_* functions.

> > I'm wondering if adding count to dlist_head and maintaining it as part
> > of the existing dlist_* data structure and functions is any better
> > than introducing dclist_*? In that case, we need only one function,
> > dlist_count, no? Or do we choose to go dclist_* because we want to
> > avoid the extra cost of maintaining count within dlist_*? If yes, is
> > maintaining count in dlist_* really costly?
>
> I have a few reasons for not wanting to do that:
>
> 1) I think dlist operations are very fast at the moment.  The fact
> that the functions are static inline tells me the function call
> overhead matters. Therefore, it's likely maintaining a count also
> matters.
> 2) Code bloat.  The functions are static inline. That means all
> compiled code that adds or removes an item from a dlist would end up
> larger. That results in more instruction cache misses.

This seems a fair point to me.

> 3) I've no reason to believe that all call sites that do
> dlist_delete() have the ability to know which list the node is on.
> Just look at ReorderBufferAssignChild().  I decided to not convert the
> subtxns dlist into a dclist as the subtransaction sometimes seems to
> go onto the top transaction list before it's moved to the sub-txn's
> list.
> 4) There's very little or no scope for bugs in dclist relating to the
> dlist implementation as all that stuff is done by just calling the
> dlist_* functions.  The only scope is really that it could call the
> wrong dlist_* function.  It does not seem terribly hard to ensure we
> don't write any bugs like that.

Right.

> > Thanks. The v3 patch looks good to me.
>
> Great. Thanks for having a look.

I will take another look at v3 tomorrow and probably mark it RfC.

--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

Hi hackers,

> I will take another look at v3 tomorrow and probably mark it RfC.

I very much like the patch. While on it:

```
+static inline bool
+dclist_is_empty(dclist_head *head)
+{
+    Assert(dlist_is_empty(&head->dlist) == (head->count == 0));
+    return (head->count == 0);
+}
```

Should we consider const'ifying the arguments of the dlist_*/dclist_*
functions that don't change the arguments?

Additionally it doesn't seem that we have any unit tests for dlist /
dclist. Should we consider adding unit tests for them to
src/test/regress?

To clarify, IMO both questions are out of scope of this specific patch
and should be submitted separately.

-- 
Best regards,
Aleksander Alekseev

On Mon, Oct 31, 2022 at 6:58 PM Aleksander Alekseev
<aleksander@timescale.com> wrote:
>
> Hi hackers,
>
> > I will take another look at v3 tomorrow and probably mark it RfC.
>
> I very much like the patch. While on it:
>
> ```
> +static inline bool
> +dclist_is_empty(dclist_head *head)
> +{
> +    Assert(dlist_is_empty(&head->dlist) == (head->count == 0));
> +    return (head->count == 0);
> +}
> ```
>
> Should we consider const'ifying the arguments of the dlist_*/dclist_*
> functions that don't change the arguments?

+1, but as a separate discussion/thread/patch IMO.

> Additionally it doesn't seem that we have any unit tests for dlist /
> dclist. Should we consider adding unit tests for them to
> src/test/regress?

Most of the dlist_* functions are being covered I guess. AFAICS,
dclist_* functions that aren't covered are dclist_insert_after(),
dclist_insert_before(), dclist_pop_head_node(), dclist_move_head(),
dclist_move_tail(), dclist_has_next(), dclist_has_prev(),
dclist_next_node(), dclist_prev_node(), dclist_head_element_off(),
dclist_head_node(), dclist_tail_element_off(), dclist_head_element().

IMO, adding an extension under src/test/modules to cover missing or
all dlist_* and dclist_* functions makes sense. It improves the code
coverage. FWIW, test_lfind is one such recent test extension.

> To clarify, IMO both questions are out of scope of this specific patch
> and should be submitted separately.

You're right, both of them must be discussed separately.

--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

On Mon, Oct 31, 2022 at 8:28 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Mon, Oct 31, 2022 at 6:58 PM Aleksander Alekseev
> <aleksander@timescale.com> wrote:
> >
> > Hi hackers,
> >
> > > I will take another look at v3 tomorrow and probably mark it RfC.
> >
> > I very much like the patch. While on it:

I took another look at v3 patch today and it looked good to me, hence
marked it RfC - https://commitfest.postgresql.org/40/3967/

--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

On Tue, 1 Nov 2022 at 20:55, Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
> I took another look at v3 patch today and it looked good to me, hence
> marked it RfC - https://commitfest.postgresql.org/40/3967/

Many thanks for reviewing this.

If nobody has any objections, I plan to push this tomorrow morning New
Zealand time (around 10 hours from now).

David

On Tue, 1 Nov 2022 at 23:19, David Rowley <dgrowleyml@gmail.com> wrote:
> If nobody has any objections, I plan to push this tomorrow morning New
> Zealand time (around 10 hours from now).

Pushed.  Thank you both for reviewing this.

David

Hi David,

> Pushed.  Thank you both for reviewing this.

Thanks for applying the patch.

>> Should we consider const'ifying the arguments of the dlist_*/dclist_*
>> functions that don't change the arguments?
>> [...]
> You're right, both of them must be discussed separately.

I would like to piggyback on this thread to propose the const'ifying
patch, if that's OK. Here it is.

-- 
Best regards,
Aleksander Alekseev

On Wed, Nov 2, 2022 at 2:23 PM Aleksander Alekseev
<aleksander@timescale.com> wrote:
>
> Hi David,
>
> > Pushed.  Thank you both for reviewing this.
>
> Thanks for applying the patch.
>
> >> Should we consider const'ifying the arguments of the dlist_*/dclist_*
> >> functions that don't change the arguments?
> >> [...]
> > You're right, both of them must be discussed separately.
>
> I would like to piggyback on this thread to propose the const'ifying
> patch, if that's OK. Here it is.

Thanks for the patch. IMO, this can be discussed in a separate thread
to get more thoughts from the hackers.

BTW, there's proclist_* data structure which might need the similar
const'ifying.

-- 
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

Hi Bharath,

> Thanks for the patch. IMO, this can be discussed in a separate thread
> to get more thoughts from the hackers.

OK, I started a new thread [1], thanks.

Regarding the improvement of the code coverage I realized that this
may be a good patch for a newcomer. I know several people who may be
interested in starting to contribute to PostgreSQL. Maybe I'll be able
to find a volunteer.

[1]: https://www.postgresql.org/message-id/flat/CAJ7c6TM2=08mNKD9aJg8vEY9hd+G4L7+Nvh30UiNT3kShgRgNg@mail.gmail.com

-- 
Best regards,
Aleksander Alekseev

On Mon, Nov 7, 2022 at 2:37 PM Aleksander Alekseev
<aleksander@timescale.com> wrote:
>
> Regarding the improvement of the code coverage I realized that this
> may be a good patch for a newcomer. I know several people who may be
> interested in starting to contribute to PostgreSQL. Maybe I'll be able
> to find a volunteer.

Hm. Or adding a ToDo item here https://wiki.postgresql.org/wiki/Todo
might also help?

-- 
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

Hi Bharath,

> > Regarding the improvement of the code coverage I realized that this
> > may be a good patch for a newcomer. I know several people who may be
> > interested in starting to contribute to PostgreSQL. Maybe I'll be able
> > to find a volunteer.
>
> Hm. Or adding a ToDo item here https://wiki.postgresql.org/wiki/Todo
> might also help?

Good point. Will it be better to use the "Miscellaneous Other" section
for this or create a new "Code coverage" section?

-- 
Best regards,
Aleksander Alekseev