Hi, On Tue, Aug 19, 2025 at 10:00:37AM +0900, Michael Paquier wrote: > On Mon, Aug 18, 2025 at 09:31:14AM +0000, Bertrand Drouvot wrote: > > Yeah, that's probably just a matter of taste, but I also prefer keeping the > > pointer over just doing: > > > > ctl->raw_dsa_area = (char *) ctl + MAXALIGN(sizeof(PgStat_ShmemControl)); > > Done as you have suggested.
Here are a few more that have been found with [1]. I did review all of them carefully and they look legitimate to remove. That said, I chose not to remove some and added comments instead for: - the last ones in ParseCommitRecord(), ParseAbortRecord() and ParsePrepareRecord() - some in ReorderBufferSerializeChange() - the one in SnapBuildSerialize() The reason is that, while they are currently useless, they would need to be added back if we add more branches/cases. So I preferred to stay on the safe side of thing. Remarks: - we could also add a comment instead of removing the one in DecodeXLogRecord(), but we're in the "and finally, the main data" part so I don't think there are risks to have to add it back. - for the ones in ReorderBufferRestoreChange(): While data is a parameter, modifying the pointer itself (not *data) only affects the local copy, so these increments are dead code. - for the ones in pgaio_uring_shmem_init(), I'm not sure if we should keep them for "documentation" purpose or remove them. The current patch removes them, but feedback is welcome. [1]: https://github.com/bdrouvot/coccinelle_on_pg/blob/main/misc/unused_pointers_after_last_update.cocci Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
>From 2fefb69f1462ce1057bb5c3d07ed70c769ec961a Mon Sep 17 00:00:00 2001 From: Bertrand Drouvot <[email protected]> Date: Sat, 22 Nov 2025 14:47:25 +0000 Subject: [PATCH v1] Remove useless pointer updates Same idea as in commit 9b7eb6f02e8. Those pointers are updated but are not used after the updates, so let's remove the useless updates or document why we want to keep them. --- src/backend/access/heap/heapam_xlog.c | 1 - src/backend/access/nbtree/nbtxlog.c | 1 - src/backend/access/rmgrdesc/gindesc.c | 1 - src/backend/access/rmgrdesc/xactdesc.c | 3 +++ src/backend/access/transam/xlogreader.c | 1 - src/backend/replication/logical/reorderbuffer.c | 7 ++----- src/backend/replication/logical/snapbuild.c | 1 + src/backend/storage/aio/method_io_uring.c | 3 --- src/backend/storage/ipc/waiteventset.c | 4 ---- 9 files changed, 6 insertions(+), 16 deletions(-) 4.2% src/backend/access/heap/ 23.9% src/backend/access/rmgrdesc/ 3.5% src/backend/access/transam/ 36.7% src/backend/replication/logical/ 6.7% src/backend/storage/aio/ 21.9% src/backend/storage/ipc/ diff --git a/src/backend/access/heap/heapam_xlog.c b/src/backend/access/heap/heapam_xlog.c index 11cb3f74da5..76e34633b71 100644 --- a/src/backend/access/heap/heapam_xlog.c +++ b/src/backend/access/heap/heapam_xlog.c @@ -75,7 +75,6 @@ heap_xlog_prune_freeze(XLogReaderState *record) /* memcpy() because snapshot_conflict_horizon is stored unaligned */ memcpy(&snapshot_conflict_horizon, maindataptr, sizeof(TransactionId)); - maindataptr += sizeof(TransactionId); if (InHotStandby) ResolveRecoveryConflictWithSnapshot(snapshot_conflict_horizon, diff --git a/src/backend/access/nbtree/nbtxlog.c b/src/backend/access/nbtree/nbtxlog.c index d4a26de06a3..6da34558061 100644 --- a/src/backend/access/nbtree/nbtxlog.c +++ b/src/backend/access/nbtree/nbtxlog.c @@ -355,7 +355,6 @@ btree_xlog_split(bool newitemonleft, XLogReaderState *record) */ left_hikey = (IndexTuple) datapos; left_hikeysz = MAXALIGN(IndexTupleSize(left_hikey)); - datapos += left_hikeysz; datalen -= left_hikeysz; Assert(datalen == 0); diff --git a/src/backend/access/rmgrdesc/gindesc.c b/src/backend/access/rmgrdesc/gindesc.c index 075c4a0ae93..fca11ac1426 100644 --- a/src/backend/access/rmgrdesc/gindesc.c +++ b/src/backend/access/rmgrdesc/gindesc.c @@ -95,7 +95,6 @@ gin_desc(StringInfo buf, XLogReaderState *record) leftChildBlkno = BlockIdGetBlockNumber((BlockId) payload); payload += sizeof(BlockIdData); rightChildBlkno = BlockIdGetBlockNumber((BlockId) payload); - payload += sizeof(BlockNumber); appendStringInfo(buf, " children: %u/%u", leftChildBlkno, rightChildBlkno); } diff --git a/src/backend/access/rmgrdesc/xactdesc.c b/src/backend/access/rmgrdesc/xactdesc.c index f0f696855b9..0b0f7155efa 100644 --- a/src/backend/access/rmgrdesc/xactdesc.c +++ b/src/backend/access/rmgrdesc/xactdesc.c @@ -133,6 +133,7 @@ ParseCommitRecord(uint8 info, xl_xact_commit *xlrec, xl_xact_parsed_commit *pars parsed->origin_lsn = xl_origin.origin_lsn; parsed->origin_timestamp = xl_origin.origin_timestamp; + /* keep pointer position updated for potential future use */ data += sizeof(xl_xact_origin); } } @@ -228,6 +229,7 @@ ParseAbortRecord(uint8 info, xl_xact_abort *xlrec, xl_xact_parsed_abort *parsed) parsed->origin_lsn = xl_origin.origin_lsn; parsed->origin_timestamp = xl_origin.origin_timestamp; + /* keep pointer position updated for potential future use */ data += sizeof(xl_xact_origin); } } @@ -275,6 +277,7 @@ ParsePrepareRecord(uint8 info, xl_xact_prepare *xlrec, xl_xact_parsed_prepare *p bufptr += MAXALIGN(xlrec->nabortstats * sizeof(xl_xact_stats_item)); parsed->msgs = (SharedInvalidationMessage *) bufptr; + /* keep pointer position updated for potential future use */ bufptr += MAXALIGN(xlrec->ninvalmsgs * sizeof(SharedInvalidationMessage)); } diff --git a/src/backend/access/transam/xlogreader.c b/src/backend/access/transam/xlogreader.c index 9cc7488e892..81f96d04f74 100644 --- a/src/backend/access/transam/xlogreader.c +++ b/src/backend/access/transam/xlogreader.c @@ -1960,7 +1960,6 @@ DecodeXLogRecord(XLogReaderState *state, out = (char *) MAXALIGN(out); decoded->main_data = out; memcpy(decoded->main_data, ptr, decoded->main_data_len); - ptr += decoded->main_data_len; out += decoded->main_data_len; } diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c index eb6a84554b7..ddd14cd1a66 100644 --- a/src/backend/replication/logical/reorderbuffer.c +++ b/src/backend/replication/logical/reorderbuffer.c @@ -4156,6 +4156,7 @@ ReorderBufferSerializeChange(ReorderBuffer *rb, ReorderBufferTXN *txn, data += sizeof(HeapTupleData); memcpy(data, newtup->t_data, newlen); + /* keep pointer position updated for potential future use */ data += newlen; } break; @@ -4186,7 +4187,6 @@ ReorderBufferSerializeChange(ReorderBuffer *rb, ReorderBufferTXN *txn, data += sizeof(Size); memcpy(data, change->data.msg.message, change->data.msg.message_size); - data += change->data.msg.message_size; break; } @@ -4204,7 +4204,6 @@ ReorderBufferSerializeChange(ReorderBuffer *rb, ReorderBufferTXN *txn, /* might have been reallocated above */ ondisk = (ReorderBufferDiskChange *) rb->outbuf; memcpy(data, change->data.inval.invalidations, inval_size); - data += inval_size; break; } @@ -4239,6 +4238,7 @@ ReorderBufferSerializeChange(ReorderBuffer *rb, ReorderBufferTXN *txn, { memcpy(data, snap->subxip, sizeof(TransactionId) * snap->subxcnt); + /* keep pointer position updated for potential future use */ data += sizeof(TransactionId) * snap->subxcnt; } break; @@ -4260,7 +4260,6 @@ ReorderBufferSerializeChange(ReorderBuffer *rb, ReorderBufferTXN *txn, ondisk = (ReorderBufferDiskChange *) rb->outbuf; memcpy(data, change->data.truncate.relids, size); - data += size; break; } @@ -4753,7 +4752,6 @@ ReorderBufferRestoreChange(ReorderBuffer *rb, ReorderBufferTXN *txn, /* restore tuple data itself */ memcpy(change->data.tp.newtuple->t_data, data, tuplelen); - data += tuplelen; } break; @@ -4777,7 +4775,6 @@ ReorderBufferRestoreChange(ReorderBuffer *rb, ReorderBufferTXN *txn, change->data.msg.message_size); memcpy(change->data.msg.message, data, change->data.msg.message_size); - data += change->data.msg.message_size; break; } diff --git a/src/backend/replication/logical/snapbuild.c b/src/backend/replication/logical/snapbuild.c index 6e18baa33cb..0d8bc875114 100644 --- a/src/backend/replication/logical/snapbuild.c +++ b/src/backend/replication/logical/snapbuild.c @@ -1637,6 +1637,7 @@ SnapBuildSerialize(SnapBuild *builder, XLogRecPtr lsn) sz = sizeof(TransactionId) * catchange_xcnt; memcpy(ondisk_c, catchange_xip, sz); COMP_CRC32C(ondisk->checksum, ondisk_c, sz); + /* keep pointer position updated for potential future use */ ondisk_c += sz; } diff --git a/src/backend/storage/aio/method_io_uring.c b/src/backend/storage/aio/method_io_uring.c index bb06da63a8e..a0c73628deb 100644 --- a/src/backend/storage/aio/method_io_uring.c +++ b/src/backend/storage/aio/method_io_uring.c @@ -307,9 +307,6 @@ pgaio_uring_shmem_init(bool first_time) /* account for alignment */ ring_mem_remain -= ring_mem_next - shmem; - shmem += ring_mem_next - shmem; - - shmem += ring_mem_remain; } for (int contextno = 0; contextno < TotalProcs; contextno++) diff --git a/src/backend/storage/ipc/waiteventset.c b/src/backend/storage/ipc/waiteventset.c index 465cee40ccc..d463693e267 100644 --- a/src/backend/storage/ipc/waiteventset.c +++ b/src/backend/storage/ipc/waiteventset.c @@ -400,16 +400,12 @@ CreateWaitEventSet(ResourceOwner resowner, int nevents) #if defined(WAIT_USE_EPOLL) set->epoll_ret_events = (struct epoll_event *) data; - data += MAXALIGN(sizeof(struct epoll_event) * nevents); #elif defined(WAIT_USE_KQUEUE) set->kqueue_ret_events = (struct kevent *) data; - data += MAXALIGN(sizeof(struct kevent) * nevents); #elif defined(WAIT_USE_POLL) set->pollfds = (struct pollfd *) data; - data += MAXALIGN(sizeof(struct pollfd) * nevents); #elif defined(WAIT_USE_WIN32) set->handles = (HANDLE) data; - data += MAXALIGN(sizeof(HANDLE) * nevents); #endif set->latch = NULL; -- 2.34.1
