Subject: Add new for_each macros for iterating over a List that do not require ListCell pointer

From: Jelte Fennema <[email-masked]>
Date: Tue, 24 Oct 2023 18:03:48 +0200

Many usages of the foreach macro in the Postgres codebase only use the
ListCell variable to then get its value. This adds macros that
simplify iteration code for that very common use case. Instead of
passing a ListCell you can pass a variable of the type of its
contents. This IMHO improves readability of the code by reducing the
total amount of code while also essentially forcing the use of useful
variable names.

While this might seem like a small quality of life improvement, in
practice it turns out to be very nice to use. At Microsoft we have
been using macros very similar to these ones in the Citus codebase for
a long time now and we pretty much never use plain foreach anymore for
new code.

Finally, I guess there needs to be some bikeshedding on the naming. In
the Citus codebase we call them foreach_xyz instead of the
for_each_xyz naming pattern that is used in this patchset. I'm not
sure what the current stance is on if foreach should be written with
or without an underscore between for and each. Currently pg_list.h
uses both.

P.S. Similar macros for forboth/forthree are also possible, but
require an exponential macro count handle all different possibilities,
which might not be worth the effort since forboth/forthree are used
much less often than foreach. In Citus we do have 3 forboth macros
that don't require ListCell for the most common cases (foreach_ptr,
foreach_ptr_oid, foreach_int_oid). But I did not want to clutter this
patchset with that discussion.

---

From: Nathan Bossart <[email-masked]>
Date: Tue, 24 Oct 2023 11:47:15 -0500

> a long time now and we pretty much never use plain foreach anymore for

This seems reasonable to me.

> or without an underscore between for and each. Currently pg_list.h

I don't have a strong opinion on the matter, but if I had to choose, I
guess I'd pick foreach_*() because these macros are most closely related to
foreach().

BTW after applying your patches, initdb began failing with the following
for me:

	TRAP: failed Assert("n >= 0 && n < list->length"), File: "list.c", Line: 770, PID: 902807

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

---

From: Alvaro Herrera <[email-masked]>
Date: Wed, 25 Oct 2023 10:51:55 +0200

> total amount of code while also essentially forcing the use of useful

+1 for getting rid of useless "lc" variables.

Looking at for_each_ptr() I think it may be cleaner to follow
palloc_object()'s precedent and make it foreach_object() instead (I have
no love for the extra underscore, but I won't object to it either).  And
like foreach_node, have it receive a type name to add a cast to.

I'd imagine something like

  SubscriptionRelState *rstate;

  foreach_object(SubscriptionRelState *, rstate, table_states_not_ready)
  {

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/

---

From: Jelte Fennema <[email-masked]>
Date: Wed, 25 Oct 2023 14:35:45 +0200

>   foreach_object(SubscriptionRelState *, rstate, table_states_not_ready)

Could you clarify why you think it may be cleaner? I don't see much
benefit to passing the type in there if all we use it for is adding a
cast. It seems like extra things to type for little benefit.
palloc_object uses the passed in type to not only do the cast, but
also to determine the size of the the allocation.

If foreach_object would allow us to remove the declaration further up
in the function I do see a benefit though.

I attached a new patchset which includes a 3rd patch that does this
(the other patches are equivalent to v4). I quite like that it moves
the type declaration to the loop itself, limiting its scope. But I'm
not fully convinced it's worth the hackiness of introducing a second
for loop that does a single iteration, just to be able to declare a
variable of a different type though. But I don't know another way of
achieving this. If this hack/trick is deemed acceptable, we can do the
same for the other newly introduced macros. The type would not even
need to be specified for oid/xid/int because it's already known to be
Oid/TransactionId/int respectively.