Re: Remove unused function parameters, part 2: replication

Hi,

On Fri, Nov 28, 2025 at 12:14:08PM -0500, Andres Freund wrote:
> Hi,
>
> On 2025-11-28 09:24:23 +0000, Bertrand Drouvot wrote:
> > PFA a patch to remove unused function parameters in replication (means
> > focusing on src/backend/replication).
> >
> > Those have been detected by a coccinelle script (that I need to polish before
> > sharing). It currently only focuses on static functions.
>
> Maybe I'm just a bit grumpy this morning, but what do we gain by changes like
> this?

I think we gain the same as in:

b91067c8995
4be9024d573
6fbd7b93c61
432c30dc4ee
2f8b4007dbb
5b7ba75f7ff
8354e7b27eb
c02767d2415
1dec091d5b0
76af9744db1
96cfcadd26e
fd5e3b29141
5cb882675ae
ecf70b916b4

to name a few.

From my point of view, mainly:

1) code clarity
2) reduce conflicts in the mid/long term
3) and last but not least: sometimes avoid unnecessary cycles in the caller(s) to
get the required parameter in existing or new code using them. For example,
postgresBeginForeignModify() is doing unnecessary work to retrieve the rte to pass
to create_foreign_modify() while not used in create_foreign_modify().

That said I agree that this kind of "massive" patches produces some noise.

I think that we have 3 options:

a) do nothing when we cross them accidentally
b) remove them when we cross them accidentally (as in the commits above)
c) use a tool that can detect them and produce the changes

I think that a) is not a good option, b) is fine and c) is the best option, but
is hard to review due to the amount of changes though.

Also by using a tool to detect them we may find some bugs in passing.

> > While reviewing the coccinelle script output, I did a bit of Archeology to know
> > where the oversights come from (which helps with review), and that gives:
> >
> > [...]
> > ReorderBuffer *rb in ReorderBufferRestoreCleanup(): b89e151054a0
> > ReorderBuffer *rb in ReorderBufferMaybeMarkTXNStreamed(): 072ee847ad4c
> > ReplicationSlot *slot in ReorderBufferSerializedPath(): 8aa75e1384b1
> > ReorderBuffer *rb in ReorderBufferFreeSnap(): b89e151054a0
> > TransactionId xid in ReorderBufferReplay(): a271a1b50e9b
>
> I don't think these are oversights. They intentionally get the reorderbuffer,
> it's an implementation detail that the *txn argument happens to currently be
> sufficient.

I agree that "Oversights" might not be the correct wording for those, but if
*txn is sufficient then maybe that's the right API. Keeping unused parameters
"just in case" can still lead to issue 3) above: current or future callers
doing unnecessary work to get the unused parameter.

That said, if there's a strong preference to keep parameters that are
"conceptually" part of the API, I'm happy to just work on the clearly dead
parameters (like the off_t *offset, StringInfo s, etc ...) as the risk for issue
3) is less for parameters that are "conceptually" part of the API.

What do you think?

Regards,

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