Subject: Re: pgsql: Separate RecoveryConflictReasons from procsignals

From: Heikki Linnakangas <[email-masked]>
Date: Tue, 10 Feb 2026 17:52:16 +0200

On 10/02/2026 17:19, Bertrand Drouvot wrote:
> Hi,
> 
> On Tue, Feb 10, 2026 at 02:32:37PM +0000, Heikki Linnakangas wrote:
>> Separate RecoveryConflictReasons from procsignals
>>
>> Share the same PROCSIG_RECOVERY_CONFLICT flag for all recovery
>> conflict reasons. To distinguish, have a bitmask in PGPROC to indicate
>> the reason(s).
> 
> I did not look at the thread, so sorry to be late, but that makes the size of PGPROC
> going from 832 to 840 bytes, so not a multiple of 64 anymore. Is that something
> to worry about? (same kind of discussion in [1]).
> 
> [1]: https://postgr.es/m/tw53roer2j4quxh7vlyv62drc5fo6c6zdltvl6d2dttqa62uhi%40stwlpdwlftpj

Right, that's a fair question. I hope the cache line alignment is not 
critical for performance, because the alignment is completely accidental 
today. I checked the size on different versions:

master:  840 (after this commit)
v18:     832
v17:     888
v14-v16: 880

So v18 was the outlier in that it happened to be 64-byte aligned.

If there's a performance reason to keep have it be aligned - and maybe 
there is - we should pad it explicitly.

- Heikki

---

From: Andres Freund <[email-masked]>
Date: Tue, 10 Feb 2026 11:41:40 -0500

Hi,

On 2026-02-10 17:52:16 +0200, Heikki Linnakangas wrote:
> If there's a performance reason to keep have it be aligned - and maybe there
> is - we should pad it explicitly.

We should make it a power of two or such. There are some workloads where the
indexing from GetPGProcByNumber() shows up, because it ends up having to be
implemented as a 64 bit multiplication, which has a reasonably high latency
(3-5 cycles). Whereas a shift has a latency of 1 and typically higher
throughput too.

Re false sharing: We should really separate stuff that changes (like
e.g. pendingRecoveryConflicts) and never changing stuff (backendType). You
don't need overlapping structs to have false sharing issues if you mix
different access patterns inside a struct that's accessed across processes...

Greetings,

Andres Freund

---

From: Heikki Linnakangas <[email-masked]>
Date: Tue, 10 Feb 2026 19:14:44 +0200

(moving to pgsql-hackers)

> (3-5 cycles). Whereas a shift has a latency of 1 and typically higher

Power of two means going to 1024 bytes. That's a lot of padding. Where 
have you seen that show up?

Attached is a patch to align to cache line boundary. That's 
straightforward if that's what we want to do.

> different access patterns inside a struct that's accessed across processes...

Makes sense, although I don't want to optimize too hard for performance, 
at the expense of readability. The current order is pretty random 
anyway, though.

It'd probably be good to move the subxids cache to the end of the 
struct. That'd act as natural padding, as it's not very frequently used, 
especially the tail end of the cache. Or come to think of it, it might 
be good to move the subxids cache out of PGPROC altogether. It's mostly 
frequently accessed in GetSnapshotData(), and for that it'd actually be 
better if it was in a separate "mirrored" array, similar to the main xid 
and subxidStates. That would eliminate the pgprocnos[pgxactoff] lookup 
from GetSnapshotdata() altogether.

I'm a little reluctant to mess with this without a concrete benchmark 
though. Got one in mind?

- Heikki
From a3e2d94f6a3ab215afd3e8cee624bd9c09ec5391 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <[email-masked]>
Date: Tue, 10 Feb 2026 18:53:31 +0200
Subject: [PATCH 1/1] Align PGPROC to cache line boundary

---
 src/include/storage/proc.h | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
index ac0df4aeaaa..53acce8a5a1 100644
--- a/src/include/storage/proc.h
+++ b/src/include/storage/proc.h
@@ -182,7 +182,7 @@ typedef enum
  *
  * See PROC_HDR for details.
  */
-struct PGPROC
+typedef struct PGPROC
 {
 	dlist_node	links;			/* list link if process is in a list */
 	dlist_head *procgloballist; /* procglobal list that owns this PGPROC */
@@ -337,10 +337,18 @@ struct PGPROC
 	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. */
+}
 
+/*
+ * 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;
 
 extern PGDLLIMPORT PGPROC *MyProc;
 
-- 
2.47.3

---

From: Andres Freund <[email-masked]>
Date: Tue, 10 Feb 2026 13:15:01 -0500

Hi,

On 2026-02-10 19:14:44 +0200, Heikki Linnakangas wrote:
> Power of two means going to 1024 bytes. That's a lot of padding. Where have
> you seen that show up?

LWLock contention heavy code, due to the GetPGProcByNumber() in LWLockWakeup()
and other similar places.

> Attached is a patch to align to cache line boundary. That's straightforward
> if that's what we want to do.

Yea, I think we should do that. Even if we don't see a difference today, just
because it's a hell to find production issues around this, because it is so
dependent on what processes use which PGPROC etc and because false sharing
issues are generally expensive to debug.

> Makes sense, although I don't want to optimize too hard for performance, at
> the expense of readability. The current order is pretty random anyway,
> though.

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)

> It'd probably be good to move the subxids cache to the end of the struct.
> That'd act as natural padding, as it's not very frequently used, especially
> the tail end of the cache.

Yea, that'd make sense.

> Or come to think of it, it might be good to move the subxids cache out of
> PGPROC altogether. It's mostly frequently accessed in GetSnapshotData(), and
> for that it'd actually be better if it was in a separate "mirrored" array,
> similar to the main xid and subxidStates. That would eliminate the
> pgprocnos[pgxactoff] lookup from GetSnapshotdata() altogether.

I doubt it's worth it - that way we'd need to move a lot larger array around
during [dis]connect. The subxids stuff is a lot larger than the xid,
statusFlags arrays...

> I'm a little reluctant to mess with this without a concrete benchmark

I'm travelling this week, but I'll try to recreate the benchmarks I've seen
this on.  Unfortunately you really need a large machine to really see
differences, without that the memory latency between cores is just too low to
realistically see issues.

Greetings,

Andres Freund

---

From: Bertrand Drouvot <[email-masked]>
Date: Tue, 10 Feb 2026 19:15:27 +0000

Hi,

>   accessed across processes)

With an ordering like in the attached (to apply on top of Heikki's patch), we're
back to 832 bytes.

But, then the pg_attribute_aligned() added in Heikki's patch makes it 896 bytes...

"
/*    816      |      16 */    dlist_node lockGroupLink;
/* XXX 64-byte padding   */

                               /* total size (bytes):  896 */
                             }
"

What about applying this new ordering and remove the pg_attribute_aligned()? (I
thought the aligned attribute would be smarter than that and not add this 64 padding
bytes).

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
commit ea3aded2c4dec487c8a2ff5470cd7bae83e8dd47
Author: Bertrand Drouvot <[email-masked]>
Date:   Tue Feb 10 18:54:29 2026 +0000

    reorder

diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
index 53acce8a5a1..5d41301c966 100644
--- a/src/include/storage/proc.h
+++ b/src/include/storage/proc.h
@@ -184,29 +184,16 @@ typedef enum
  */
 typedef struct PGPROC
 {
-	dlist_node	links;			/* list link if process is in a list */
-	dlist_head *procgloballist; /* procglobal list that owns this PGPROC */
-
-	PGSemaphore sem;			/* ONE semaphore to sleep on */
-	ProcWaitStatus waitStatus;
-
-	Latch		procLatch;		/* generic latch for process */
-
-
-	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 ! */
-
 	int			pid;			/* Backend's process ID; 0 if prepared xact */
-
 	int			pgxactoff;		/* offset into various ProcGlobal->arrays with
 								 * data mirrored from this PGPROC */
+	BackendType backendType;	/* what kind of process is this? */
+
+	/* 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 */
+	Oid			tempNamespaceId;	/* OID of temp schema this backend is
+									 * using */
 
 	/*
 	 * Currently running top-level transaction's virtual xid. Together these
@@ -227,24 +214,44 @@ typedef 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 */
+	dlist_node	links;			/* list link if process is in a list */
+	dlist_head *procgloballist; /* procglobal list that owns this PGPROC */
 
-	Oid			tempNamespaceId;	/* OID of temp schema this backend is
-									 * using */
+	uint32		wait_event_info;	/* proc's wait information */
+	pg_atomic_uint64 waitStart; /* time at which wait for lock acquisition
+								 * started */
 
-	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] */
 
-	/*
-	 * While in hot standby mode, shows that a conflict signal has been sent
-	 * for the current transaction. Set/cleared while holding ProcArrayLock,
-	 * though not required. Accessed without lock, if needed.
-	 *
-	 * This is a bitmask; each bit corresponds to a RecoveryConflictReason
-	 * enum value.
-	 */
-	pg_atomic_uint32 pendingRecoveryConflicts;
+	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 ! */
+
+	ProcWaitStatus waitStatus;
+	int			delayChkptFlags;	/* for DELAY_CHKPT_* flags */
+
+	LOCKMODE	waitLockMode;	/* type of lock we're waiting for */
+	LOCKMASK	heldLocks;		/* bitmask for lock types already held on this
+								 * lock object by this backend */
+
+	uint8		statusFlags;	/* this backend's status flags, see PROC_*
+								 * above. mirrored in
+								 * ProcGlobal->statusFlags[pgxactoff] */
+	XidCacheStatus subxidStatus;	/* mirrored with
+									 * ProcGlobal->subxidStates[i] */
+
+	bool		procArrayGroupMember;	/* true, if member of ProcArray group
+										 * waiting for XID clear */
+	bool		clogGroupMember;	/* true, if member of clog group */
+	bool		fpVXIDLock;		/* are we holding a fast-path VXID lock? */
+
+	LocalTransactionId fpLocalTransactionId;	/* lxid for fast-path VXID
+												 * lock */
+	PGSemaphore sem;			/* ONE semaphore to sleep on */
 
 	/*
 	 * Info about LWLock the process is currently waiting for, if any.
@@ -255,6 +262,19 @@ typedef struct PGPROC
 	 */
 	uint8		lwWaiting;		/* see LWLockWaitState */
 	uint8		lwWaitMode;		/* lwlock mode being waited for */
+
+	Latch		procLatch;		/* generic latch for process */
+
+	/*
+	 * While in hot standby mode, shows that a conflict signal has been sent
+	 * for the current transaction. Set/cleared while holding ProcArrayLock,
+	 * though not required. Accessed without lock, if needed.
+	 *
+	 * This is a bitmask; each bit corresponds to a RecoveryConflictReason
+	 * enum value.
+	 */
+	pg_atomic_uint32 pendingRecoveryConflicts;
+
 	proclist_node lwWaitLink;	/* position in LW lock wait list */
 
 	/* Support for condition variables. */
@@ -264,17 +284,6 @@ typedef struct PGPROC
 	/* waitLock and waitProcLock are NULL if not currently waiting. */
 	LOCK	   *waitLock;		/* Lock object we're sleeping on ... */
 	PROCLOCK   *waitProcLock;	/* Per-holder info for awaited lock */
-	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 */
-
-	uint8		statusFlags;	/* this backend's status flags, see PROC_*
-								 * above. mirrored in
-								 * ProcGlobal->statusFlags[pgxactoff] */
 
 	/*
 	 * Info to allow us to wait for synchronous replication, if needed.
@@ -293,15 +302,11 @@ typedef struct PGPROC
 	 */
 	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. */
-	/* true, if member of ProcArray group waiting for XID clear */
-	bool		procArrayGroupMember;
-	/* next ProcArray group member waiting for XID clear */
-	pg_atomic_uint32 procArrayGroupNext;
+	pg_atomic_uint32 procArrayGroupNext;	/* next ProcArray group member
+											 * waiting for XID clear */
 
 	/*
 	 * latest transaction id among the transaction's main XID and
@@ -309,10 +314,7 @@ typedef struct PGPROC
 	 */
 	TransactionId procArrayGroupMemberXid;
 
-	uint32		wait_event_info;	/* proc's wait information */
-
 	/* 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 */
 	XidStatus	clogGroupMemberXidStatus;	/* transaction status of clog
@@ -326,9 +328,6 @@ typedef struct PGPROC
 	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 */
 
 	/*
 	 * Support for lock groups.  Use LockHashPartitionLockByProc on the group