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
