Re: PGPROC alignment (was Re: pgsql: Separate RecoveryConflictReasons from procsignals)

Hi,

On Tue, Feb 10, 2026 at 10:53:58PM +0200, Heikki Linnakangas wrote:
> On 10/02/2026 21:46, Andres Freund wrote:
> > On 2026-02-10 19:15:27 +0000, Bertrand Drouvot wrote:
> > > On Tue, Feb 10, 2026 at 01:15:01PM -0500, Andres Freund wrote:
> > > > On 2026-02-10 19:14:44 +0200, Heikki Linnakangas wrote:
> > > > Yea, I don't think we need to be perfect here. Just a bit less bad. And, as
> > > > you say, the current order doesn't make a lot of sense.
> > > > Just grouping things like
> > > > - pid, pgxactoff, backendType (i.e. barely if ever changing)
> > > > - wait_event_info, waitStart (i.e. very frequently changing, but typically
> > > >    accessed within one proc)
> > > > - sem, lwWaiting, waitLockMode (i.e. stuff that is updated frequently and
> > > >    accessed across processes)
> > > 
> > > With an ordering like in the attached (to apply on top of Heikki's patch), we're
> > > back to 832 bytes.
> > 
> > You'd really need to insert padding between the sections to make it work...
> 
> Here's my attempt at grouping things more logically.

Thanks!

> I didn't insert padding
> and also didn't try to avoid alignment padding. I tried to optimize for
> readability rather than size or performance.

Yeah, my attempt was to put the size back to 832 bytes but that's probably not
worth it as stated by Andres up-thread.

> That said, I would assume that
> grouping things logically like this would also help to avoid false sharing.
> If not, inserting explicit padding seems like a a good fix.
> 
> I also think we should split 'links' into two fields. For clarity.
>
 
A few comments:

0001:

+ * and (b) to make the multiplication / division to convert between PGPROC *
+ * and ProcNumber be a little cheaper

Is that correct if PGPROC size is not a power of 2?

0002: Good catch!

0003:

1/ There is one missing change in PrintLockQueue() ("links" is still used, and
that should be replaced by "waitLink").

2/ change the comment on top of ProcWakeup?

"
/*
 * ProcWakeup -- wake up a process by setting its latch.
 *
 *   Also remove the process from the wait queue and set its links invalid.
"

s/links/waitLink/?

Also, out of curiosity, with 0003 in place PGPROC size goes from 840 to 856.

0004:

The grouping looks Ok to me. Just one nit for the added comments:

+       /*---- Backend identity ----*/
+       /*---- Transactions and snapshots ----*/
+       /*---- Inter-process signaling ----*/
+       /*---- LWLock waiting ----*/
+       /*---- Lock manager data ----*/
+       /*---- Synchronous replication waiting ----*/
+       /*---- Support for group XID clearing. ----*/
+       /*---- Support for group transaction status update. ----*/
+       /*---- Status reporting ----*/

Some have period and some don't.

> With this, sizeof(PGPROC) == 864 without the explicit alignment to
> PG_CACHE_LINE_SIZE, and 896 with it.

I can see 876 -> 896 on my side:

/*    872      |       4 */    uint32 wait_event_info;
/* XXX 20-byte padding   */

                               /* total size (bytes):  896 */
                             }
Regards,

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