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

On Mon, 18 Dec 2023 at 19:00, Jelte Fennema-Nio <postgres(at)jeltef(dot)nl> wrote:
>
> The more I think about it and look at the code, the more I like the
> usage of the loop style proposed in the previous 0003 patch (which
> automatically declares a loop variable for the scope of the loop using
> a second for loop).
>
> I did some testing on godbolt.org and both versions of the macros
> result in the same assembly when compiling with -O2 (and even -O1)
> when compiling with ancient versions of gcc (5.1) and clang (3.0):
> https://godbolt.org/z/WqfTbhe4e
>
> So attached is now an updated patchset that only includes these even
> easier to use foreach macros. I also updated some of the comments and
> moved modifying foreach_delete_current and foreach_current_index to
> their own commit.
>
> On Thu, 14 Dec 2023 at 16:54, Jelte Fennema-Nio <postgres(at)jeltef(dot)nl> wrote:
> >
> > On Fri, 1 Dec 2023 at 05:20, Nathan Bossart <nathandbossart(at)gmail(dot)com> wrote:
> > > Could we simplify it with something like the following?
> >
> > Great suggestion! Updated the patchset accordingly.
> >
> > This made it also easy to change the final patch to include the
> > automatic scoped declaration logic for all of the new macros. I quite
> > like how the calling code changes to not have to declare the variable.
> > But it's definitely a larger divergence from the status quo than
> > without patch 0003. So I'm not sure if it's desired.
> >
> > Finally, I also renamed the functions to use foreach instead of
> > for_each, since based on this thread that seems to be the generally
> > preferred naming.

Thanks for working on this, this simplifies foreach further.
I noticed that this change can be done in several other places too. I
had seen the following parts of code from logical replication files
can be changed:
1) The below in pa_detach_all_error_mq function can be changed to foreach_ptr
foreach(lc, ParallelApplyWorkerPool)
{
shm_mq_result res;
Size nbytes;
void *data;
ParallelApplyWorkerInfo *winfo = (ParallelApplyWorkerInfo *) lfirst(lc);

2) The below in logicalrep_worker_detach function can be changed to foreach_ptr
foreach(lc, workers)
{
LogicalRepWorker *w = (LogicalRepWorker *) lfirst(lc);

if (isParallelApplyWorker(w))
logicalrep_worker_stop_internal(w, SIGTERM);
}

3) The below in ApplyLauncherMain function can be changed to foreach_ptr
/* Start any missing workers for enabled subscriptions. */
sublist = get_subscription_list();
foreach(lc, sublist)
{
Subscription *sub = (Subscription *) lfirst(lc);
LogicalRepWorker *w;
TimestampTz last_start;
TimestampTz now;
long elapsed;

if (!sub->enabled)
continue;

4) The below in pa_launch_parallel_worker function can be changed to
foreach_ptr
ListCell *lc;

/* Try to get an available parallel apply worker from the worker pool. */
foreach(lc, ParallelApplyWorkerPool)
{
winfo = (ParallelApplyWorkerInfo *) lfirst(lc);

if (!winfo->in_use)
return winfo;
}

Should we start doing these changes too now?

Regards,
Vignesh