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
