Обсуждение: Remove useless pointer advance in StatsShmemInit()

Hi hackers,

While reviewing [1], 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]: https://www.postgresql.org/message-id/CAA5RZ0ukmNd%2BC1jH4V6BGEea-wmyLxDtDE5QoEtfXd2W5HNHfQ%40mail.gmail.com

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

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]. 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

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 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.

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 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.
> 
> 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 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.
> >
> > 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