On 11/02/2026 06:40, Bertrand Drouvot wrote:
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.
Fixed that, and changed to different style for the delimiter comments.
It's a matter of taste, but I think the new style is closer to what we
use elsewhere.
On 13/02/2026 10:03, Bertrand Drouvot wrote:
and what the patch adds:
+/*
+ * If compiler understands aligned pragma, use it to align the struct at cache
+ * line boundaries. This is just for performance, to (a) avoid false sharing
+ * and (b) to make the multiplication / division to convert between PGPROC *
+ * and ProcNumber be a little cheaper.
+ */
+#if defined(pg_attribute_aligned)
+ pg_attribute_aligned(PG_CACHE_LINE_SIZE)
+#endif
+PGPROC;
It means that PGPROC is "acceptable" without padding (on compiler that does not
understand the aligned attribute).
OTOH, looking at:
"
typedef union WALInsertLockPadded
{
WALInsertLock l;
char pad[PG_CACHE_LINE_SIZE];
} WALInsertLockPadded;
"
It seems to mean that WALInsertLockPadded is unacceptable without padding (since
it's not using pg_attribute_aligned()).
That looks ok to see PGPROC as an "acceptable" one, if not, should we use the
union trick?
It seems acceptable to just not align it if the compiler doesn't support
it. This is just a performance optimization, after all.
Attached is new versions the remaining patches. I think these are ready
to be committed.
I don't have the hardware and test case that would be sensitive enough
for these changes in memory layout, so I haven't done any performance
testing of this. I'd guess this no worse than the old layout. Grouping
fields together which are used together is usually a good strategy. I
don't feel obliged to do performance testing of this, given that no one
did that for the old layout either, so if it happened to be really good
from a performance point of view, it was purely accidental. But if
anyone has the means and interest to do that, that'd be much appreciated
of course.
- Heikki
From cc75db39184b6560ded5f97b5202c14187e9f801 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <[email protected]>
Date: Fri, 20 Feb 2026 22:30:01 +0200
Subject: [PATCH v2 1/2] Rearrange fields in PGPROC, for clarity
The ordering was pretty random, making it hard to get an overview of
what's in it. Group related fields together, and add comments to act
as separators between the groups.
Reviewed-by: Bertrand Drouvot <[email protected]>
Discussion: https://www.postgresql.org/message-id/[email protected]
---
src/include/storage/proc.h | 147 +++++++++++++++++++++++--------------
1 file changed, 92 insertions(+), 55 deletions(-)
diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
index e165b414241..b2fd4d02959 100644
--- a/src/include/storage/proc.h
+++ b/src/include/storage/proc.h
@@ -178,33 +178,41 @@ typedef enum
*
* See PROC_HDR for details.
*/
-struct PGPROC
+typedef struct PGPROC
{
dlist_head *procgloballist; /* procglobal list that owns this PGPROC */
dlist_node freeProcsLink; /* link in procgloballist, when in recycled
* state */
- PGSemaphore sem; /* ONE semaphore to sleep on */
- ProcWaitStatus waitStatus;
-
- Latch procLatch; /* generic latch for process */
+ /************************************************************************
+ * Backend identity
+ ************************************************************************/
+ /*
+ * These fields that don't change after backend startup, or only very
+ * rarely
+ */
+ int pid; /* Backend's process ID; 0 if prepared xact */
+ BackendType backendType; /* what kind of process is this? */
- TransactionId xid; /* id of top-level transaction currently being
- * executed by this proc, if running and XID
- * is assigned; else InvalidTransactionId.
- * mirrored in ProcGlobal->xids[pgxactoff] */
-
- TransactionId xmin; /* minimal running XID as it was when we were
- * starting our xact, excluding LAZY VACUUM:
- * vacuum must not remove tuples deleted by
- * xid >= xmin ! */
+ /* These fields are zero while a backend is still starting up: */
+ Oid databaseId; /* OID of database this backend is using */
+ Oid roleId; /* OID of role using this backend */
- int pid; /* Backend's process ID; 0 if prepared xact */
+ Oid tempNamespaceId; /* OID of temp schema this backend is
+ * using */
int pgxactoff; /* offset into various ProcGlobal->arrays with
* data mirrored from this PGPROC */
+ uint8 statusFlags; /* this backend's status flags, see PROC_*
+ * above. mirrored in
+ * ProcGlobal->statusFlags[pgxactoff] */
+
+ /************************************************************************
+ * Transactions and snapshots
+ ************************************************************************/
+
/*
* Currently running top-level transaction's virtual xid. Together these
* form a VirtualTransactionId, but we don't use that struct because this
@@ -224,14 +232,30 @@ struct PGPROC
* InvalidLocalTransactionId */
} vxid;
- /* These fields are zero while a backend is still starting up: */
- Oid databaseId; /* OID of database this backend is using */
- Oid roleId; /* OID of role using this backend */
+ TransactionId xid; /* id of top-level transaction currently being
+ * executed by this proc, if running and XID
+ * is assigned; else InvalidTransactionId.
+ * mirrored in ProcGlobal->xids[pgxactoff] */
- Oid tempNamespaceId; /* OID of temp schema this backend is
- * using */
+ TransactionId xmin; /* minimal running XID as it was when we were
+ * starting our xact, excluding LAZY VACUUM:
+ * vacuum must not remove tuples deleted by
+ * xid >= xmin ! */
- BackendType backendType; /* what kind of process is this? */
+ XidCacheStatus subxidStatus; /* mirrored with
+ * ProcGlobal->subxidStates[i] */
+ struct XidCache subxids; /* cache for subtransaction XIDs */
+
+
+ /************************************************************************
+ * Inter-process signaling
+ ************************************************************************/
+
+ Latch procLatch; /* generic latch for process */
+
+ PGSemaphore sem; /* ONE semaphore to sleep on */
+
+ int delayChkptFlags; /* for DELAY_CHKPT_* flags */
/*
* While in hot standby mode, shows that a conflict signal has been sent
@@ -243,6 +267,10 @@ struct PGPROC
*/
pg_atomic_uint32 pendingRecoveryConflicts;
+ /************************************************************************
+ * LWLock waiting
+ ************************************************************************/
+
/*
* Info about LWLock the process is currently waiting for, if any.
*
@@ -257,6 +285,18 @@ struct PGPROC
/* Support for condition variables. */
proclist_node cvWaitLink; /* position in CV wait list */
+ /************************************************************************
+ * Lock manager data
+ ************************************************************************/
+
+ /*
+ * Support for lock groups. Use LockHashPartitionLockByProc on the group
+ * leader to get the LWLock protecting these fields.
+ */
+ PGPROC *lockGroupLeader; /* lock group leader, if I'm a member */
+ dlist_head lockGroupMembers; /* list of members, if I'm a leader */
+ dlist_node lockGroupLink; /* my member link, if I'm a member */
+
/* Info about lock the process is currently waiting for, if any. */
/* waitLock and waitProcLock are NULL if not currently waiting. */
LOCK *waitLock; /* Lock object we're sleeping on ... */
@@ -265,14 +305,30 @@ struct PGPROC
LOCKMODE waitLockMode; /* type of lock we're waiting for */
LOCKMASK heldLocks; /* bitmask for lock types already held on this
* lock object by this backend */
+
pg_atomic_uint64 waitStart; /* time at which wait for lock acquisition
* started */
- int delayChkptFlags; /* for DELAY_CHKPT_* flags */
+ ProcWaitStatus waitStatus;
- uint8 statusFlags; /* this backend's status flags, see PROC_*
- * above. mirrored in
- * ProcGlobal->statusFlags[pgxactoff] */
+ /*
+ * All PROCLOCK objects for locks held or awaited by this backend are
+ * linked into one of these lists, according to the partition number of
+ * their lock.
+ */
+ dlist_head myProcLocks[NUM_LOCK_PARTITIONS];
+
+ /*-- recording fast-path locks taken by this backend. --*/
+ LWLock fpInfoLock; /* protects per-backend fast-path state */
+ uint64 *fpLockBits; /* lock modes held for each fast-path slot */
+ Oid *fpRelId; /* slots for rel oids */
+ bool fpVXIDLock; /* are we holding a fast-path VXID lock? */
+ LocalTransactionId fpLocalTransactionId; /* lxid for fast-path VXID
+ * lock */
+
+ /************************************************************************
+ * Synchronous replication waiting
+ ************************************************************************/
/*
* Info to allow us to wait for synchronous replication, if needed.
@@ -284,18 +340,10 @@ struct PGPROC
int syncRepState; /* wait state for sync rep */
dlist_node syncRepLinks; /* list link if process is in syncrep queue */
- /*
- * All PROCLOCK objects for locks held or awaited by this backend are
- * linked into one of these lists, according to the partition number of
- * their lock.
- */
- dlist_head myProcLocks[NUM_LOCK_PARTITIONS];
-
- XidCacheStatus subxidStatus; /* mirrored with
- * ProcGlobal->subxidStates[i] */
- struct XidCache subxids; /* cache for subtransaction XIDs */
+ /************************************************************************
+ * Support for group XID clearing
+ ************************************************************************/
- /* Support for group XID clearing. */
/* true, if member of ProcArray group waiting for XID clear */
bool procArrayGroupMember;
/* next ProcArray group member waiting for XID clear */
@@ -307,9 +355,10 @@ struct PGPROC
*/
TransactionId procArrayGroupMemberXid;
- uint32 wait_event_info; /* proc's wait information */
+ /************************************************************************
+ * Support for group transaction status update
+ ************************************************************************/
- /* Support for group transaction status update. */
bool clogGroupMember; /* true, if member of clog group */
pg_atomic_uint32 clogGroupNext; /* next clog group member */
TransactionId clogGroupMemberXid; /* transaction id of clog group member */
@@ -320,24 +369,12 @@ struct PGPROC
XLogRecPtr clogGroupMemberLsn; /* WAL location of commit record for clog
* group member */
- /* Lock manager data, recording fast-path locks taken by this backend. */
- LWLock fpInfoLock; /* protects per-backend fast-path state */
- uint64 *fpLockBits; /* lock modes held for each fast-path slot */
- Oid *fpRelId; /* slots for rel oids */
- bool fpVXIDLock; /* are we holding a fast-path VXID lock? */
- LocalTransactionId fpLocalTransactionId; /* lxid for fast-path VXID
- * lock */
+ /************************************************************************
+ * Status reporting
+ ************************************************************************/
- /*
- * Support for lock groups. Use LockHashPartitionLockByProc on the group
- * leader to get the LWLock protecting these fields.
- */
- PGPROC *lockGroupLeader; /* lock group leader, if I'm a member */
- dlist_head lockGroupMembers; /* list of members, if I'm a leader */
- dlist_node lockGroupLink; /* my member link, if I'm a member */
-};
-
-/* NOTE: "typedef struct PGPROC PGPROC" appears in storage/lock.h. */
+ uint32 wait_event_info; /* proc's wait information */
+} PGPROC;
extern PGDLLIMPORT PGPROC *MyProc;
--
2.47.3
From ce1218929f2e14106c0c1160bfa941ffe0b6b745 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <[email protected]>
Date: Fri, 20 Feb 2026 22:58:40 +0200
Subject: [PATCH v2 2/2] Align PGPROC to cache line boundary
On common architectures, the PGPROC happened to be a multiple of 64
bytes on PG 18, but it's changed on 'master' since. There was worry
that changing the alignment might hurt performance, due to false
cacheline sharing across PGPROC elements. However, there was no
explicit alignment, so any alignment to cache lines was
accidental. Add explicit alignment to remove worry about false
sharing.
Reviewed-by: Bertrand Drouvot <[email protected]>
Discussion: https://www.postgresql.org/message-id/[email protected]
---
src/include/storage/proc.h | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
index b2fd4d02959..a8d2e7db1a1 100644
--- a/src/include/storage/proc.h
+++ b/src/include/storage/proc.h
@@ -374,8 +374,16 @@ typedef struct PGPROC
************************************************************************/
uint32 wait_event_info; /* proc's wait information */
-} PGPROC;
+}
+/*
+ * If compiler understands aligned pragma, use it to align the struct at cache
+ * line boundaries. This is just for performance, to avoid false sharing.
+ */
+#if defined(pg_attribute_aligned)
+ pg_attribute_aligned(PG_CACHE_LINE_SIZE)
+#endif
+PGPROC;
extern PGDLLIMPORT PGPROC *MyProc;
--
2.47.3
