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
Attachments:
v1-0001-Removing-unused-function-parameters-in-replicatio.patchtext/x-diff; charset=us-asciiDownload
From 3f30ac9840e31d36a7ecfcbd733b7cbabcdf7fbb Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Date: Fri, 28 Nov 2025 07:43:02 +0000
Subject: [PATCH v1] Removing unused function parameters in replication
Oversights in commits:
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
---
.../replication/logical/reorderbuffer.c | 61 ++++++++++---------
src/backend/replication/logical/worker.c | 16 ++---
src/backend/replication/pgoutput/pgoutput.c | 4 +-
src/backend/replication/walsender.c | 7 +--
4 files changed, 44 insertions(+), 44 deletions(-)
87.6% src/backend/replication/logical/
4.6% src/backend/replication/pgoutput/
7.6% src/backend/replication/
diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c
index eb6a84554b7..0bf3c00617d 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -270,17 +270,17 @@ static Size ReorderBufferRestoreChanges(ReorderBuffer *rb, ReorderBufferTXN *txn
TXNEntryFile *file, XLogSegNo *segno);
static void ReorderBufferRestoreChange(ReorderBuffer *rb, ReorderBufferTXN *txn,
char *data);
-static void ReorderBufferRestoreCleanup(ReorderBuffer *rb, ReorderBufferTXN *txn);
+static void ReorderBufferRestoreCleanup(ReorderBufferTXN *txn);
static void ReorderBufferTruncateTXN(ReorderBuffer *rb, ReorderBufferTXN *txn,
bool txn_prepared);
-static void ReorderBufferMaybeMarkTXNStreamed(ReorderBuffer *rb, ReorderBufferTXN *txn);
+static void ReorderBufferMaybeMarkTXNStreamed(ReorderBufferTXN *txn);
static bool ReorderBufferCheckAndTruncateAbortedTXN(ReorderBuffer *rb, ReorderBufferTXN *txn);
static void ReorderBufferCleanupSerializedTXNs(const char *slotname);
-static void ReorderBufferSerializedPath(char *path, ReplicationSlot *slot,
+static void ReorderBufferSerializedPath(char *path,
TransactionId xid, XLogSegNo segno);
static int ReorderBufferTXNSizeCompare(const pairingheap_node *a, const pairingheap_node *b, void *arg);
-static void ReorderBufferFreeSnap(ReorderBuffer *rb, Snapshot snap);
+static void ReorderBufferFreeSnap(Snapshot snap);
static Snapshot ReorderBufferCopySnap(ReorderBuffer *rb, Snapshot orig_snap,
ReorderBufferTXN *txn, CommandId cid);
@@ -562,7 +562,7 @@ ReorderBufferFreeChange(ReorderBuffer *rb, ReorderBufferChange *change,
case REORDER_BUFFER_CHANGE_INTERNAL_SNAPSHOT:
if (change->data.snapshot)
{
- ReorderBufferFreeSnap(rb, change->data.snapshot);
+ ReorderBufferFreeSnap(change->data.snapshot);
change->data.snapshot = NULL;
}
break;
@@ -1612,7 +1612,7 @@ ReorderBufferCleanupTXN(ReorderBuffer *rb, ReorderBufferTXN *txn)
if (txn->snapshot_now != NULL)
{
Assert(rbtxn_is_streamed(txn));
- ReorderBufferFreeSnap(rb, txn->snapshot_now);
+ ReorderBufferFreeSnap(txn->snapshot_now);
}
/*
@@ -1634,7 +1634,7 @@ ReorderBufferCleanupTXN(ReorderBuffer *rb, ReorderBufferTXN *txn)
/* remove entries spilled to disk */
if (rbtxn_is_serialized(txn))
- ReorderBufferRestoreCleanup(rb, txn);
+ ReorderBufferRestoreCleanup(txn);
/* deallocate */
ReorderBufferFreeTXN(rb, txn);
@@ -1673,7 +1673,7 @@ ReorderBufferTruncateTXN(ReorderBuffer *rb, ReorderBufferTXN *txn, bool txn_prep
Assert(rbtxn_is_known_subxact(subtxn));
Assert(subtxn->nsubtxns == 0);
- ReorderBufferMaybeMarkTXNStreamed(rb, subtxn);
+ ReorderBufferMaybeMarkTXNStreamed(subtxn);
ReorderBufferTruncateTXN(rb, subtxn, txn_prepared);
}
@@ -1742,7 +1742,7 @@ ReorderBufferTruncateTXN(ReorderBuffer *rb, ReorderBufferTXN *txn, bool txn_prep
/* If this txn is serialized then clean the disk space. */
if (rbtxn_is_serialized(txn))
{
- ReorderBufferRestoreCleanup(rb, txn);
+ ReorderBufferRestoreCleanup(txn);
txn->txn_flags &= ~RBTXN_IS_SERIALIZED;
/*
@@ -1965,7 +1965,7 @@ ReorderBufferCopySnap(ReorderBuffer *rb, Snapshot orig_snap,
* Free a previously ReorderBufferCopySnap'ed snapshot
*/
static void
-ReorderBufferFreeSnap(ReorderBuffer *rb, Snapshot snap)
+ReorderBufferFreeSnap(Snapshot snap)
{
if (snap->copied)
pfree(snap);
@@ -2135,7 +2135,7 @@ ReorderBufferSaveTXNSnapshot(ReorderBuffer *rb, ReorderBufferTXN *txn,
* or has changes.
*/
static void
-ReorderBufferMaybeMarkTXNStreamed(ReorderBuffer *rb, ReorderBufferTXN *txn)
+ReorderBufferMaybeMarkTXNStreamed(ReorderBufferTXN *txn)
{
/*
* The top-level transaction, is marked as streamed always, even if it
@@ -2534,7 +2534,7 @@ ReorderBufferProcessTXN(ReorderBuffer *rb, ReorderBufferTXN *txn,
if (snapshot_now->copied)
{
- ReorderBufferFreeSnap(rb, snapshot_now);
+ ReorderBufferFreeSnap(snapshot_now);
snapshot_now =
ReorderBufferCopySnap(rb, change->data.snapshot,
txn, command_id);
@@ -2667,7 +2667,7 @@ ReorderBufferProcessTXN(ReorderBuffer *rb, ReorderBufferTXN *txn,
if (streaming)
ReorderBufferSaveTXNSnapshot(rb, txn, snapshot_now, command_id);
else if (snapshot_now->copied)
- ReorderBufferFreeSnap(rb, snapshot_now);
+ ReorderBufferFreeSnap(snapshot_now);
/* cleanup */
TeardownHistoricSnapshot(false);
@@ -2717,7 +2717,7 @@ ReorderBufferProcessTXN(ReorderBuffer *rb, ReorderBufferTXN *txn,
if (streaming || rbtxn_is_prepared(txn))
{
if (streaming)
- ReorderBufferMaybeMarkTXNStreamed(rb, txn);
+ ReorderBufferMaybeMarkTXNStreamed(txn);
ReorderBufferTruncateTXN(rb, txn, rbtxn_is_prepared(txn));
/* Reset the CheckXidAlive */
@@ -2792,7 +2792,7 @@ ReorderBufferProcessTXN(ReorderBuffer *rb, ReorderBufferTXN *txn,
/* Mark the transaction is streamed if appropriate */
if (stream_started)
- ReorderBufferMaybeMarkTXNStreamed(rb, txn);
+ ReorderBufferMaybeMarkTXNStreamed(txn);
/* Reset the TXN so that it is allowed to stream remaining data. */
ReorderBufferResetTXN(rb, txn, snapshot_now,
@@ -2821,7 +2821,7 @@ ReorderBufferProcessTXN(ReorderBuffer *rb, ReorderBufferTXN *txn,
*/
static void
ReorderBufferReplay(ReorderBufferTXN *txn,
- ReorderBuffer *rb, TransactionId xid,
+ ReorderBuffer *rb,
XLogRecPtr commit_lsn, XLogRecPtr end_lsn,
TimestampTz commit_time,
RepOriginId origin_id, XLogRecPtr origin_lsn)
@@ -2895,7 +2895,7 @@ ReorderBufferCommit(ReorderBuffer *rb, TransactionId xid,
if (txn == NULL)
return;
- ReorderBufferReplay(txn, rb, xid, commit_lsn, end_lsn, commit_time,
+ ReorderBufferReplay(txn, rb, commit_lsn, end_lsn, commit_time,
origin_id, origin_lsn);
}
@@ -2979,8 +2979,9 @@ ReorderBufferPrepare(ReorderBuffer *rb, TransactionId xid,
txn->gid = pstrdup(gid);
- ReorderBufferReplay(txn, rb, xid, txn->final_lsn, txn->end_lsn,
- txn->prepare_time, txn->origin_id, txn->origin_lsn);
+ ReorderBufferReplay(txn, rb, txn->final_lsn, txn->end_lsn,
+ txn->prepare_time, txn->origin_id,
+ txn->origin_lsn);
/*
* Send a prepare if not already done so. This might occur if we have
@@ -3050,8 +3051,9 @@ ReorderBufferFinishPrepared(ReorderBuffer *rb, TransactionId xid,
* then downstream can behave as it has already replayed commit
* prepared after the restart.
*/
- ReorderBufferReplay(txn, rb, xid, txn->final_lsn, txn->end_lsn,
- txn->prepare_time, txn->origin_id, txn->origin_lsn);
+ ReorderBufferReplay(txn, rb, txn->final_lsn, txn->end_lsn,
+ txn->prepare_time, txn->origin_id,
+ txn->origin_lsn);
}
txn->final_lsn = commit_lsn;
@@ -4043,7 +4045,7 @@ ReorderBufferSerializeTXN(ReorderBuffer *rb, ReorderBufferTXN *txn)
* No need to care about TLIs here, only used during a single run,
* so each LSN only maps to a specific WAL record.
*/
- ReorderBufferSerializedPath(path, MyReplicationSlot, txn->xid,
+ ReorderBufferSerializedPath(path, txn->xid,
curOpenSegNo);
/* open segment, create it if necessary */
@@ -4424,7 +4426,7 @@ ReorderBufferStreamTXN(ReorderBuffer *rb, ReorderBufferTXN *txn)
/* Free the previously copied snapshot. */
Assert(txn->snapshot_now->copied);
- ReorderBufferFreeSnap(rb, txn->snapshot_now);
+ ReorderBufferFreeSnap(txn->snapshot_now);
txn->snapshot_now = NULL;
}
@@ -4590,8 +4592,7 @@ ReorderBufferRestoreChanges(ReorderBuffer *rb, ReorderBufferTXN *txn,
* No need to care about TLIs here, only used during a single run,
* so each LSN only maps to a specific WAL record.
*/
- ReorderBufferSerializedPath(path, MyReplicationSlot, txn->xid,
- *segno);
+ ReorderBufferSerializedPath(path, txn->xid, *segno);
*fd = PathNameOpenFile(path, O_RDONLY | PG_BINARY);
@@ -4854,7 +4855,7 @@ ReorderBufferRestoreChange(ReorderBuffer *rb, ReorderBufferTXN *txn,
* Remove all on-disk stored for the passed in transaction.
*/
static void
-ReorderBufferRestoreCleanup(ReorderBuffer *rb, ReorderBufferTXN *txn)
+ReorderBufferRestoreCleanup(ReorderBufferTXN *txn)
{
XLogSegNo first;
XLogSegNo cur;
@@ -4871,7 +4872,7 @@ ReorderBufferRestoreCleanup(ReorderBuffer *rb, ReorderBufferTXN *txn)
{
char path[MAXPGPATH];
- ReorderBufferSerializedPath(path, MyReplicationSlot, txn->xid, cur);
+ ReorderBufferSerializedPath(path, txn->xid, cur);
if (unlink(path) != 0 && errno != ENOENT)
ereport(ERROR,
(errcode_for_file_access(),
@@ -4923,7 +4924,7 @@ ReorderBufferCleanupSerializedTXNs(const char *slotname)
* at least MAXPGPATH.
*/
static void
-ReorderBufferSerializedPath(char *path, ReplicationSlot *slot, TransactionId xid,
+ReorderBufferSerializedPath(char *path, TransactionId xid,
XLogSegNo segno)
{
XLogRecPtr recptr;
@@ -5364,7 +5365,7 @@ DisplayMapping(HTAB *tuplecid_data)
* transaction c) applied in LSN order.
*/
static void
-ApplyLogicalMappingFile(HTAB *tuplecid_data, Oid relid, const char *fname)
+ApplyLogicalMappingFile(HTAB *tuplecid_data, const char *fname)
{
char path[MAXPGPATH];
int fd;
@@ -5547,7 +5548,7 @@ UpdateLogicalMappings(HTAB *tuplecid_data, Oid relid, Snapshot snapshot)
elog(DEBUG1, "applying mapping: \"%s\" in %u", f->fname,
snapshot->subxip[0]);
- ApplyLogicalMappingFile(tuplecid_data, relid, f->fname);
+ ApplyLogicalMappingFile(tuplecid_data, f->fname);
pfree(f);
}
}
diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.c
index 93970c6af29..6d0867c70d2 100644
--- a/src/backend/replication/logical/worker.c
+++ b/src/backend/replication/logical/worker.c
@@ -568,7 +568,7 @@ static void send_feedback(XLogRecPtr recvpos, bool force, bool requestReply);
static void maybe_advance_nonremovable_xid(RetainDeadTuplesData *rdt_data,
bool status_received);
-static bool can_advance_nonremovable_xid(RetainDeadTuplesData *rdt_data);
+static bool can_advance_nonremovable_xid(void);
static void process_rdt_phase_transition(RetainDeadTuplesData *rdt_data,
bool status_received);
static void get_candidate_xid(RetainDeadTuplesData *rdt_data);
@@ -578,7 +578,7 @@ static void wait_for_publisher_status(RetainDeadTuplesData *rdt_data,
static void wait_for_local_flush(RetainDeadTuplesData *rdt_data);
static bool should_stop_conflict_info_retention(RetainDeadTuplesData *rdt_data);
static void stop_conflict_info_retention(RetainDeadTuplesData *rdt_data);
-static void resume_conflict_info_retention(RetainDeadTuplesData *rdt_data);
+static void resume_conflict_info_retention(void);
static bool update_retention_status(bool active);
static void reset_retention_data_fields(RetainDeadTuplesData *rdt_data);
static void adjust_xid_advance_interval(RetainDeadTuplesData *rdt_data,
@@ -1663,7 +1663,7 @@ apply_handle_stream_prepare(StringInfo s)
* TODO, support tracking of multiple origins
*/
static void
-apply_handle_origin(StringInfo s)
+apply_handle_origin(void)
{
/*
* ORIGIN message can only come inside streaming transaction or inside
@@ -3820,7 +3820,7 @@ apply_dispatch(StringInfo s)
break;
case LOGICAL_REP_MSG_ORIGIN:
- apply_handle_origin(s);
+ apply_handle_origin();
break;
case LOGICAL_REP_MSG_MESSAGE:
@@ -4387,7 +4387,7 @@ static void
maybe_advance_nonremovable_xid(RetainDeadTuplesData *rdt_data,
bool status_received)
{
- if (!can_advance_nonremovable_xid(rdt_data))
+ if (!can_advance_nonremovable_xid())
return;
process_rdt_phase_transition(rdt_data, status_received);
@@ -4398,7 +4398,7 @@ maybe_advance_nonremovable_xid(RetainDeadTuplesData *rdt_data,
* is allowed.
*/
static bool
-can_advance_nonremovable_xid(RetainDeadTuplesData *rdt_data)
+can_advance_nonremovable_xid(void)
{
/*
* It is sufficient to manage non-removable transaction ID for a
@@ -4441,7 +4441,7 @@ process_rdt_phase_transition(RetainDeadTuplesData *rdt_data,
stop_conflict_info_retention(rdt_data);
break;
case RDT_RESUME_CONFLICT_INFO_RETENTION:
- resume_conflict_info_retention(rdt_data);
+ resume_conflict_info_retention();
break;
}
}
@@ -4839,7 +4839,7 @@ stop_conflict_info_retention(RetainDeadTuplesData *rdt_data)
* Workhorse for the RDT_RESUME_CONFLICT_INFO_RETENTION phase.
*/
static void
-resume_conflict_info_retention(RetainDeadTuplesData *rdt_data)
+resume_conflict_info_retention(void)
{
/* We can't resume retention without updating retention status. */
if (!update_retention_status(true))
diff --git a/src/backend/replication/pgoutput/pgoutput.c b/src/backend/replication/pgoutput/pgoutput.c
index 942e1abdb58..1830a07c022 100644
--- a/src/backend/replication/pgoutput/pgoutput.c
+++ b/src/backend/replication/pgoutput/pgoutput.c
@@ -1058,7 +1058,7 @@ pgoutput_row_filter_init(PGOutputData *data, List *publications,
* values of 'publish_generated_columns' parameter in the publications.
*/
static void
-check_and_init_gencol(PGOutputData *data, List *publications,
+check_and_init_gencol(List *publications,
RelationSyncEntry *entry)
{
Relation relation = RelationIdGetRelation(entry->publish_as_relid);
@@ -2314,7 +2314,7 @@ get_rel_sync_entry(PGOutputData *data, Relation relation)
pgoutput_row_filter_init(data, rel_publications, entry);
/* Check whether to publish generated columns. */
- check_and_init_gencol(data, rel_publications, entry);
+ check_and_init_gencol(rel_publications, entry);
/* Initialize the column list */
pgoutput_column_list_init(data, rel_publications, entry);
diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c
index fc8f8559073..fce044c5e06 100644
--- a/src/backend/replication/walsender.c
+++ b/src/backend/replication/walsender.c
@@ -265,7 +265,7 @@ static void XLogSendLogical(void);
static void WalSndDone(WalSndSendDataCallback send_data);
static void IdentifySystem(void);
static void UploadManifest(void);
-static bool HandleUploadManifestPacket(StringInfo buf, off_t *offset,
+static bool HandleUploadManifestPacket(StringInfo buf,
IncrementalBackupInfo *ib);
static void ReadReplicationSlot(ReadReplicationSlotCmd *cmd);
static void CreateReplicationSlot(CreateReplicationSlotCmd *cmd);
@@ -672,7 +672,6 @@ UploadManifest(void)
{
MemoryContext mcxt;
IncrementalBackupInfo *ib;
- off_t offset = 0;
StringInfoData buf;
/*
@@ -698,7 +697,7 @@ UploadManifest(void)
pq_flush();
/* Receive packets from client until done. */
- while (HandleUploadManifestPacket(&buf, &offset, ib))
+ while (HandleUploadManifestPacket(&buf, ib))
;
/* Finish up manifest processing. */
@@ -734,7 +733,7 @@ UploadManifest(void)
* additional packets and false if the UPLOAD_MANIFEST operation is complete.
*/
static bool
-HandleUploadManifestPacket(StringInfo buf, off_t *offset,
+HandleUploadManifestPacket(StringInfo buf,
IncrementalBackupInfo *ib)
{
int mtype;
--
2.34.1
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(): a271a1b50e9bI 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
ecf70b916b4to 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 changesI 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(): a271a1b50e9bI 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]https://github.com/bdrouvot/coccinelle_on_pg/blob/main/misc/unused_function_parameters.cocci, 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
On 2 Dec 2025, at 15:28, Bertrand Drouvot <bertranddrouvot.pg@gmail.com> wrote:
That said I'm still skeptical that we need to provide a strong
justification (as the one above) to remove an unused parameter.
If it breaks an existing published API thus causing extensions to fail to
compile then IMHO that's a pretty strong argument against removing a parameter
even if it's unused, likewise if the change can be expected to cause
backpatching conflicts for the coming five years. For static functions at
least it seems that compilers are fairly happy to remove the parameter in
greater than -O0 levels (though I know that won't move the needle on one of
your main drivers being readability).
--
Daniel Gustafsson
Hi,
On Thu, Dec 04, 2025 at 10:34:34AM +0100, Daniel Gustafsson wrote:
On 2 Dec 2025, at 15:28, Bertrand Drouvot <bertranddrouvot.pg@gmail.com> wrote:
That said I'm still skeptical that we need to provide a strong
justification (as the one above) to remove an unused parameter.If it breaks an existing published API thus causing extensions to fail to
compile then IMHO that's a pretty strong argument against removing a parameter
even if it's unused,
Yeah, that's why I did focus on static functions only.
likewise if the change can be expected to cause backpatching conflicts for
the coming five years.
Fair point about backpatching. That said, I did find examples of commits
removing unused parameters (see above). I'm trying to understand
when this kind of cleanup is considered acceptable vs. when the backpatching
cost outweighs the benefit. Any guidance would be helpful.
For static functions at
least it seems that compilers are fairly happy to remove the parameter in
greater than -O0 levels (though I know that won't move the needle on one of
your main drivers being readability).
Yeah, my motivation isn't execution efficiency.
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com