Обсуждение: Remove unused function parameters, part 2: replication

Hi hackers,

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.

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:

off_t *offset in HandleUploadManifestPacket(): dc212340058b
PGOutputData *data in check_and_init_gencol(): 7054186c4ebe
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
Oid relid in ApplyLogicalMappingFile(): b89e151054a0
RetainDeadTuplesData *rdt_data in can_advance_nonremovable_xid(): 228c37086855
RetainDeadTuplesData *rdt_data in resume_conflict_info_retention(): 0d48d393d465
StringInfo s in apply_handle_origin(): 665d1fad99e7

Regards,

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

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?


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


> Oid relid in ApplyLogicalMappingFile(): b89e151054a0

Same, except it's in parallel to UpdateLogicalMappings().

Greetings,

Andres Freund

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

Hi,

On 2025-11-28 18:29:15 +0000, Bertrand Drouvot wrote:
> On Fri, Nov 28, 2025 at 12:14:08PM -0500, Andres Freund wrote:
> > 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

Sometimes maybe, but not in the cases you patched here. What you did was to
make the arguments much more inline with implementation details, which means
the callers will have to change more often, not less.



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

How can it do that in this case?


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

I am strongly against the ReorderBuffer changes. It's pretty obvious that the
ReorderBuffer is conceptually a "this" OOP style parameter. Removing it from
some cases that happen to not need it makes no sense.

I am pretty unconvinced this kind of stuff is worth the noise they produce in
the more general case too.

Greetings,

Andres Freund

On Fri, Nov 28, 2025 at 2:54 PM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:
>
> RetainDeadTuplesData *rdt_data in can_advance_nonremovable_xid(): 228c37086855
> RetainDeadTuplesData *rdt_data in resume_conflict_info_retention(): 0d48d393d465
>

All nearby static functions introduced for the same feature have
passed this RetainDeadTuplesData structure. At this point, it is not
used but it can be used in future. So, I'm not sure if it is a good
idea to remove it now. Added Hou-San to see if he has any opinion on
this.

> StringInfo s in apply_handle_origin(): 665d1fad99e7
>

All apply_handle_* functions passed the same parameter and that
function has some TODO as well, so again not sure if it is a good idea
to remove it.

--
With Regards,
Amit Kapila.

Hi,

On Fri, Nov 28, 2025 at 01:39:19PM -0500, Andres Freund wrote:
> I am pretty unconvinced this kind of stuff is worth the noise they produce in
> the more general case too.

I agree that it's noisy and time consuming to review, that's the drawback of using
automated tools when they find and produce a noticeable number of changes.

I'd say the tool is there [1], we know we can use it if we feel the need and the
energy to review.

We can still continue to fix them when we cross them "accidentally".

That said, it somehow sounds weird to wait to cross them accidentally knowing we
have the tool to find them, so I'm still not convinced that just ignoring them
is the right thing to do.

[1]: https://github.com/bdrouvot/coccinelle_on_pg/blob/main/misc/unused_function_parameters.cocci

Regards,

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

On Mon, Dec 01, 2025 at 06:49:25AM +0000, Bertrand Drouvot wrote:
> We can still continue to fix them when we cross them "accidentally".
>
> That said, it somehow sounds weird to wait to cross them accidentally knowing we
> have the tool to find them, so I'm still not convinced that just ignoring them
> is the right thing to do.

There are a couple of concepts that usually come in the balance here.
For example, in some cases, we may not want to remove function
arguments because it can make API definitions more consistent across
the board, aka leaner for the reader.  It may be also possible that
having these function arguments lying around could help in future
backpatches, not to mention that it reduces the chances of conflicts.
Andres' arguments are on this side of the balance, as far as I
understand.

An argument that can argue in favor of a removal is if this simplifies
the stack of functions calling the function where the removal happens.
Simple example I have seen in the past: a Relation argument not used
(I think there has been at least one such example in tablecmds.c,
whatever).  Removing this argument also meant that we don't require
function callers to open a Relation, removing the need to think about
the lock it would require at open.  In such a case, removing an
argument has more value than what a script detects, even more if this
routine is published in a header, as it could be called by some
out-of-core extension code, or in a fork.
--
Michael

> On 2 Dec 2025, at 08:32, Michael Paquier <michael@paquier.xyz> wrote:

> Simple example I have seen in the past: a Relation argument not used
> (I think there has been at least one such example in tablecmds.c,
> whatever).  Removing this argument also meant that we don't require
> function callers to open a Relation, removing the need to think about
> the lock it would require at open.

I think this is the really interesting case and the angle to focus on.  If we
can simplify callers to perhaps even avoid locks then that's a stronger case
when considering potential API breaks.  It might still be more value in not
breaking API, but that would have to be considered on a case by case basis.

--
Daniel Gustafsson

Hi,

On Tue, Dec 02, 2025 at 04:32:09PM +0900, Michael Paquier wrote:
> On Mon, Dec 01, 2025 at 06:49:25AM +0000, Bertrand Drouvot wrote:
> > We can still continue to fix them when we cross them "accidentally".
> > 
> > That said, it somehow sounds weird to wait to cross them accidentally knowing we
> > have the tool to find them, so I'm still not convinced that just ignoring them
> > is the right thing to do.
> 
> There are a couple of concepts that usually come in the balance here.
> For example, in some cases, we may not want to remove function
> arguments because it can make API definitions more consistent across
> the board, aka leaner for the reader.

Yeah, I got this point. The "not convinced" above was related to the general
case (not the API related one).

> It may be also possible that
> having these function arguments lying around could help in future
> backpatches, not to mention that it reduces the chances of conflicts.

I'm not sure I agree with it: just keeping unused parameters in case of
backpatches. I mean how could we predict that the ones that have been removed
in the commits I mentioned above will not produce conflicts?

Regards,

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

Hi,

On Tue, Dec 02, 2025 at 09:31:34AM +0100, Daniel Gustafsson wrote:
> > On 2 Dec 2025, at 08:32, Michael Paquier <michael@paquier.xyz> wrote:
> 
> > Simple example I have seen in the past: a Relation argument not used
> > (I think there has been at least one such example in tablecmds.c,
> > whatever).  Removing this argument also meant that we don't require
> > function callers to open a Relation, removing the need to think about
> > the lock it would require at open.

Yeah, that's a strong argument.

> I think this is the really interesting case and the angle to focus on.  If we
> can simplify callers to perhaps even avoid locks then that's a stronger case
> when considering potential API breaks.  It might still be more value in not
> breaking API, but that would have to be considered on a case by case basis.

I fully agree. That said I'm still skeptical that we need to provide a strong
justification (as the one above) to remove an unused parameter.

Regards,

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