On Tue, 19 Dec 2023 at 11:59, vignesh C <vignesh21@gmail.com> wrote:
> I noticed that this change can be done in several other places too.
My guess would be that ~90% of all existing foreach loops in the
codebase can be easily rewritten (and simplified) using these new
macros. So converting all of those would likely be quite a bit of
work. In patch 0003 I only converted a few of them to get some
coverage of the new macros and show how much simpler the usage of them
is.
> Should we start doing these changes too now?
I think we should at least wait until this patchset is merged before
we start changing other places. If there's some feedback on the macros
and decide we change how they get called, then it would be a waste of
time to have to change all the call sites.
And even once these patches are merged to master, I think we should
only do any bulk changes if/when we backport these macros to all
supported PG versions. Backporting to PG12 is probably the hardest,
since List its internal layout got heavily changed in PG13. Probably
not too hard though, in Citus we've had similar macros work since
PG11. I'm also not sure what the policy is for backporting patches
that introduce new functions/macros in public headers.
We probably even want to consider some automatic rewriting script (for
the obvious cases) and/or timing the merge, to avoid having to do many
rebases of the patch.