Обсуждение: 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