Remove useless pointer advance in StatsShmemInit()
Hi hackers,
While reviewing [1]/messages/by-id/CAA5RZ0ukmNd+C1jH4V6BGEea-wmyLxDtDE5QoEtfXd2W5HNHfQ@mail.gmail.com, I noticed a useless pointer advance and saw that StatsShmemInit()
is doing the same.
As StatsShmemInit() is existing code, let's fix it: the pointer is not used after
its last advance, so that advance is unnecessary and can be removed.
[1]: /messages/by-id/CAA5RZ0ukmNd+C1jH4V6BGEea-wmyLxDtDE5QoEtfXd2W5HNHfQ@mail.gmail.com
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
Attachments:
v1-0001-Remove-useless-pointer-advance-in-StatsShmemInit.patchtext/x-diff; charset=us-asciiDownload
From b8f56fe7df72830b0054c3652170716341d9dee2 Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Date: Mon, 18 Aug 2025 08:22:27 +0000
Subject: [PATCH v1] Remove useless pointer advance in StatsShmemInit()
A pointer is not used after its last advance, so that advance is unnecessary and
can be removed.
---
src/backend/utils/activity/pgstat_shmem.c | 1 -
1 file changed, 1 deletion(-)
100.0% src/backend/utils/activity/
diff --git a/src/backend/utils/activity/pgstat_shmem.c b/src/backend/utils/activity/pgstat_shmem.c
index cca4277f234..62de3474453 100644
--- a/src/backend/utils/activity/pgstat_shmem.c
+++ b/src/backend/utils/activity/pgstat_shmem.c
@@ -180,7 +180,6 @@ StatsShmemInit(void)
* provides a small efficiency win.
*/
ctl->raw_dsa_area = p;
- p += MAXALIGN(pgstat_dsa_init_size());
dsa = dsa_create_in_place(ctl->raw_dsa_area,
pgstat_dsa_init_size(),
LWTRANCHE_PGSTATS_DSA, NULL);
--
2.34.1
On Mon, Aug 18, 2025 at 09:04:59AM +0000, Bertrand Drouvot wrote:
As StatsShmemInit() is existing code, let's fix it: the pointer is not used after
its last advance, so that advance is unnecessary and can be removed.
@@ -180,7 +180,6 @@ StatsShmemInit(void)
* provides a small efficiency win.
*/
ctl->raw_dsa_area = p;
- p += MAXALIGN(pgstat_dsa_init_size());
dsa = dsa_create_in_place(ctl->raw_dsa_area,
pgstat_dsa_init_size(),
LWTRANCHE_PGSTATS_DSA, NULL);
I'd bet that this is a vestige of the earlier versions discussed for
the pgstats shmem patch, where !IsUnderPostmaster was doing a few more
things with this pointer going down.
One could argue that "p" could be removed, moving the
sizeof(PgStat_ShmemControl) when we set raw_dsa_area, but that's a bit
cleaner with the extra pointer assignment and the comment for
pgStatLocal.shmem. So, why not.
--
Michael
Hi,
On Mon, Aug 18, 2025 at 06:13:05PM +0900, Michael Paquier wrote:
On Mon, Aug 18, 2025 at 09:04:59AM +0000, Bertrand Drouvot wrote:
As StatsShmemInit() is existing code, let's fix it: the pointer is not used after
its last advance, so that advance is unnecessary and can be removed.
@@ -180,7 +180,6 @@ StatsShmemInit(void)* provides a small efficiency win.
*/
ctl->raw_dsa_area = p;
- p += MAXALIGN(pgstat_dsa_init_size());
dsa = dsa_create_in_place(ctl->raw_dsa_area,
pgstat_dsa_init_size(),
LWTRANCHE_PGSTATS_DSA, NULL);One could argue that "p" could be removed, moving the
sizeof(PgStat_ShmemControl) when we set raw_dsa_area, but that's a bit
cleaner with the extra pointer assignment and the comment for
pgStatLocal.shmem.
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));
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
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.
--
Michael
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]https://github.com/bdrouvot/coccinelle_on_pg/blob/main/misc/unused_pointers_after_last_update.cocci. 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
Attachments:
v1-0001-Remove-useless-pointer-updates.patchtext/x-diff; charset=us-asciiDownload
From 2fefb69f1462ce1057bb5c3d07ed70c769ec961a Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
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
On Tue, Dec 02, 2025 at 07:40:44AM +0000, Bertrand Drouvot wrote:
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.
@@ -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);
I'd rather leave this one untouched. maindataptr is initialized at
the beginning of the routine.
left_hikey = (IndexTuple) datapos;
left_hikeysz = MAXALIGN(IndexTupleSize(left_hikey));
- datapos += left_hikeysz;
datalen -= left_hikeysz;
This one looks intentional to me, in line with datalen.
@@ -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;
This one is definitely intentional, and I've looked at this code a lot.
/* account for alignment */
ring_mem_remain -= ring_mem_next - shmem;
- shmem += ring_mem_next - shmem;
-
- shmem += ring_mem_remain;
I'd leave these ones as well, except if its author argues for changing
it. It is documentation.
The one in gin_desc() is pointless, indeed. This looks like a remnant
of 5dc851afde8d to me.
#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
There is an argument about block reordering on this one, where we'd
still want data to be incremented to a maxaligned area if we read more
data past the number of events.
Not sure about the ones in ReorderBufferSerializeChange(). There's an
effort done so as the computations could still matter if the code is
reordered or refactored for a reason or another.
--
Michael
Hi,
On 2025-12-02 07:40:44 +0000, Bertrand Drouvot wrote:
From 2fefb69f1462ce1057bb5c3d07ed70c769ec961a Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Date: Sat, 22 Nov 2025 14:47:25 +0000
Subject: [PATCH v1] Remove useless pointer updatesSame 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.
I think this is a bad idea. To the degree that I think 9b7eb6f02e8 ought to be
reverted. All these changes do is to make future extensions of the relevant
code more failure prone. Omitting the pointer update means that the pointer
at the end points before the last "chunk", rather than at the end.
What's the point of this? Compilers are perfectly capable of removing a
trailing store if the updated value isn't ever used afterwards.
Greetings,
Andres
Hi,
On Tue, Dec 02, 2025 at 03:00:39PM -0500, Andres Freund wrote:
Hi,
On 2025-12-02 07:40:44 +0000, Bertrand Drouvot wrote:
From 2fefb69f1462ce1057bb5c3d07ed70c769ec961a Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Date: Sat, 22 Nov 2025 14:47:25 +0000
Subject: [PATCH v1] Remove useless pointer updatesSame 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.What's the point of this? Compilers are perfectly capable of removing a
trailing store if the updated value isn't ever used afterwards.
yeah, but my motivation isn't execution efficiency but rather code clarity and
maintenance burden.
I'm proposing to "remove the useless updates or document why we want
to keep them". If the consensus is to keep them all, that's fine, but I think
we should at least add a comment. That would avoid:
- people spending time trying to understand why these updates exist, only to
eventually realize it's dead code, and potentially sending unnecessary cleanup
patches.
- code (including the dead code) being copy/pasted into new patches where the
increment might actually cause bugs or confusion. FWIW, that's exactly how I
discovered the one in 9b7eb6f02e8 (during a patch review where this code was
copy/pasted).
So, what about documenting them all?
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
Hi,
On 2025-12-03 08:14:41 +0000, Bertrand Drouvot wrote:
On Tue, Dec 02, 2025 at 03:00:39PM -0500, Andres Freund wrote:
On 2025-12-02 07:40:44 +0000, Bertrand Drouvot wrote:
From 2fefb69f1462ce1057bb5c3d07ed70c769ec961a Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Date: Sat, 22 Nov 2025 14:47:25 +0000
Subject: [PATCH v1] Remove useless pointer updatesSame 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.What's the point of this? Compilers are perfectly capable of removing a
trailing store if the updated value isn't ever used afterwards.yeah, but my motivation isn't execution efficiency but rather code clarity and
maintenance burden.I'm proposing to "remove the useless updates or document why we want
to keep them". If the consensus is to keep them all, that's fine, but I think
we should at least add a comment. That would avoid:- people spending time trying to understand why these updates exist, only to
eventually realize it's dead code, and potentially sending unnecessary cleanup
patches.- code (including the dead code) being copy/pasted into new patches where the
increment might actually cause bugs or confusion. FWIW, that's exactly how I
discovered the one in 9b7eb6f02e8 (during a patch review where this code was
copy/pasted).So, what about documenting them all?
I'm -0.1 on that. I think it's just as likely that those code comments will
not be moved when another chunk of memory is needed and cause confusion that
way. Sorry, but to me this is just a case of restating obvious code in an
obvious mechanical comment.
Greetings,
Andres Freund
Hi,
On Wed, Dec 03, 2025 at 10:13:55AM -0500, Andres Freund wrote:
On 2025-12-03 08:14:41 +0000, Bertrand Drouvot wrote:
So, what about documenting them all?
I'm -0.1 on that. I think it's just as likely that those code comments will
not be moved when another chunk of memory is needed and cause confusion that
way. Sorry, but to me this is just a case of restating obvious code in an
obvious mechanical comment.
No problem at all, thanks for sharing your thoughts!
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com