Horton

Thread

Commits

  1. Use C11 alignas in typedef definitions

  2. Align PGPROC to cache line boundary

  3. Rearrange fields in PGPROC, for clarity

  4. Split PGPROC 'links' field into two, for clarity

  5. Remove useless store to local variable

  6. Separate RecoveryConflictReasons from procsignals

  1. pgsql: Separate RecoveryConflictReasons from procsignals

    Heikki Linnakangas <heikki.linnakangas@iki.fi> — 2026-02-10T14:32:37Z

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

Reviewed-by: Chao Li <li.evan.chao@gmail.com>
Discussion: https://www.postgresql.org/message-id/4cc13ba1-4248-4884-b6ba-4805349e7f39@iki.fi

Branch
------
master

Details
-------
https://git.postgresql.org/pg/commitdiff/17f51ea818753093f929b4c235f3b89ebcc7c5fb

Modified Files
--------------
src/backend/commands/dbcommands.c            |   1 +
src/backend/commands/tablespace.c            |   1 +
src/backend/replication/logical/logicalctl.c |   1 +
src/backend/replication/slot.c               |   6 +-
src/backend/storage/buffer/bufmgr.c          |   5 +-
src/backend/storage/ipc/procarray.c          | 136 ++++++++++++++++++---------
src/backend/storage/ipc/procsignal.c         |  22 +----
src/backend/storage/ipc/standby.c            |  61 ++++++------
src/backend/storage/lmgr/proc.c              |   5 +-
src/backend/tcop/postgres.c                  | 117 ++++++++++++-----------
src/backend/utils/activity/pgstat_database.c |  18 ++--
src/backend/utils/adt/mcxtfuncs.c            |   1 +
src/include/storage/proc.h                   |  10 ++
src/include/storage/procarray.h              |   7 +-
src/include/storage/procsignal.h             |  16 +---
src/include/storage/standby.h                |  34 ++++++-
src/include/tcop/tcopprot.h                  |   2 +-
src/tools/pgindent/typedefs.list             |   1 +
18 files changed, 258 insertions(+), 186 deletions(-)

  2. Re: pgsql: Separate RecoveryConflictReasons from procsignals

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2026-02-10T15:19:09Z

    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

Regards,

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

  3. Re: pgsql: Separate RecoveryConflictReasons from procsignals

    Heikki Linnakangas <hlinnaka@iki.fi> — 2026-02-10T15:52:16Z

    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

  4. Re: pgsql: Separate RecoveryConflictReasons from procsignals

    Andres Freund <andres@anarazel.de> — 2026-02-10T16:41:40Z

    Hi,

On 2026-02-10 17:52:16 +0200, Heikki Linnakangas wrote:
> 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.

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

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

    Heikki Linnakangas <hlinnaka@iki.fi> — 2026-02-10T17:14:44Z

    (moving to pgsql-hackers)

On 10/02/2026 18:41, Andres Freund wrote:
> 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.

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.

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

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

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

    Andres Freund <andres@anarazel.de> — 2026-02-10T18:15:01Z

    Hi,

On 2026-02-10 19:14:44 +0200, Heikki Linnakangas wrote:
> On 10/02/2026 18:41, Andres Freund wrote:
> > 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.
> 
> 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.


> > 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...
> 
> 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
> though. Got one in mind?

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

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

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2026-02-10T19:15:27Z

    Hi,

On Tue, Feb 10, 2026 at 01:15:01PM -0500, Andres Freund wrote:
> Hi,
> 
> 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.

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

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

    Andres Freund <andres@anarazel.de> — 2026-02-10T19:46:31Z

    Hi,

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


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

That's just because we have

/*
 * Assumed cache line size.  This doesn't affect correctness, but can be used
 * for low-level optimizations.  This is mostly used to pad various data
 * structures, to ensure that highly-contended fields are on different cache
 * lines.  Too small a value can hurt performance due to false sharing, while
 * the only downside of too large a value is a few bytes of wasted memory.
 * The default is 128, which should be large enough for all supported
 * platforms.
 */
#define PG_CACHE_LINE_SIZE		128


I don't think we need to worry about the number of bytes here very much. This
isn't much compared to all the other overheads a connection slot has (like the
memory for locks).

Greetings,

Andres Freund

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

    Heikki Linnakangas <hlinnaka@iki.fi> — 2026-02-10T20:53:58Z

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

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

- Heikki

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

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2026-02-11T04:40:40Z

    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

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

    Heikki Linnakangas <hlinnaka@iki.fi> — 2026-02-11T10:03:51Z

    On 11/02/2026 06:40, Bertrand Drouvot wrote:
> 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?

You're right, it's not.

> 0002: Good catch!

Committed that.

>> 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 */
>                               }

Interesting. I've attached 'pahole bin/postgres' output from my laptop. 
It's Linux on arm64. This is with my v2 patches to rearrange the fields, 
but with the "pg_attribute_aligned(PG_CACHE_LINE_SIZE)" removed.

- Heikki

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

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2026-02-11T10:58:41Z

    Hi,

On Wed, Feb 11, 2026 at 12:03:51PM +0200, Heikki Linnakangas wrote:
> On 11/02/2026 06:40, Bertrand Drouvot wrote:
> > > 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 */
> >                               }
> 
> Interesting. I've attached 'pahole bin/postgres' output from my laptop. It's
> Linux on arm64.

Thanks!

Got it: I was using "-DCACHEDEBUG -DWAL_DEBUG -DLOCK_DEBUG -DDEBUG_DEADLOCK"
so that with LOCK_DEBUG in place then LWLock size is 32 bytes (vs 16 in your case).

Regards,

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

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

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2026-02-13T08:03:04Z

    Hi,

On Wed, Feb 11, 2026 at 12:03:51PM +0200, Heikki Linnakangas wrote:
> On 11/02/2026 06:40, Bertrand Drouvot wrote:
> > 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?
> 
> You're right, it's not.

Looking more closely at:

"
/* GCC supports aligned and packed */
#if defined(__GNUC__)
#define pg_attribute_aligned(a) __attribute__((aligned(a)))
#define pg_attribute_packed() __attribute__((packed))
#elif defined(_MSC_VER)
/*
 * MSVC supports aligned.
 *
 * Packing is also possible but only by wrapping the entire struct definition
 * which doesn't fit into our current macro declarations.
 */
#define pg_attribute_aligned(a) __declspec(align(a))
#else
/*
 * NB: aligned and packed are not given default definitions because they
 * affect code functionality; they *must* be implemented by the compiler
 * if they are to be used.
 */
#endif
"

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?

Regards,

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

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

    Heikki Linnakangas <hlinnaka@iki.fi> — 2026-02-20T21:01:33Z

    On 11/02/2026 06:40, Bertrand Drouvot wrote:
> On Tue, Feb 10, 2026 at 10:53:58PM +0200, Heikki Linnakangas wrote:
> 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/?

Fixed those and pushed this "Split PGPROC 'links' field into two, for 
clarity" patch. Thanks for the review!

- Heikki

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

    Heikki Linnakangas <hlinnaka@iki.fi> — 2026-02-20T21:03:09Z

    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

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

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2026-02-21T09:42:13Z

    Hi,

On Fri, Feb 20, 2026 at 11:03:09PM +0200, Heikki Linnakangas wrote:
> On 11/02/2026 06:40, Bertrand Drouvot wrote:
> > 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.

Agreed.

> Attached is new versions the remaining patches. I think these are ready to
> be committed.

Thanks!

One nit, 0001 is adding the typedef:

"
-struct PGPROC
+typedef struct PGPROC
.
.
.
-};
-
-/* NOTE: "typedef struct PGPROC PGPROC" appears in storage/lock.h. */
+       uint32          wait_event_info;        /* proc's wait information */
+} PGPROC;
"

Would that make more sense to add the typedef when we introduce the explicit
alignment in 0002 (like it was done in your previous
v2-0001-Align-PGPROC-to-cache-line-boundary.patch up-thread)?

Regards,

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

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

    Heikki Linnakangas <hlinnaka@iki.fi> — 2026-02-22T11:15:03Z

    On 21/02/2026 11:42, Bertrand Drouvot wrote:
> On Fri, Feb 20, 2026 at 11:03:09PM +0200, Heikki Linnakangas wrote:
>> On 11/02/2026 06:40, Bertrand Drouvot wrote:
>>> 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.
> 
> Agreed.
> 
>> Attached is new versions the remaining patches. I think these are ready to
>> be committed.
> 
> Thanks!
> 
> One nit, 0001 is adding the typedef:
> 
> "
> -struct PGPROC
> +typedef struct PGPROC
> .
> .
> .
> -};
> -
> -/* NOTE: "typedef struct PGPROC PGPROC" appears in storage/lock.h. */
> +       uint32          wait_event_info;        /* proc's wait information */
> +} PGPROC;
> "
> 
> Would that make more sense to add the typedef when we introduce the explicit
> alignment in 0002 (like it was done in your previous
> v2-0001-Align-PGPROC-to-cache-line-boundary.patch up-thread)?

Yeah, I had it as part of the other commit in the previous version, but 
decided to make it part of the other one, so that it's more clear what 
the alignment. I don't think it matters much either way though.

Pushed, thanks for the review!

- Heikki

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

    Peter Eisentraut <peter@eisentraut.org> — 2026-02-23T16:13:29Z

    On 13.02.26 09:03, Bertrand Drouvot wrote:
> +/*
> + * 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;

You can/should use C11 standard alignas(), so you don't need to worry 
about whether it's supported or not.

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

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2026-02-24T11:28:11Z

    Hi,

On Mon, Feb 23, 2026 at 05:13:29PM +0100, Peter Eisentraut wrote:
> On 13.02.26 09:03, Bertrand Drouvot wrote:
> > +/*
> > + * 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;
> 
> You can/should use C11 standard alignas(), so you don't need to worry about
> whether it's supported or not.

Oh right, I did not notice 300c8f53247 and following like e7075a3405c, d4c0f91f7d5
and 97e04c74bed.

PFA, 0001 doing so for PGPROC and PgAioUringContext. As those are typedef, 
the patch puts alignas within the struct.

For PGPROC at the start of the struct, I think that placing it on the first member
is the right location because it ensures the whole struct is aligned to PG_CACHE_LINE_SIZE
without adding padding before this member. For example if I set it on backendType,
then it adds 100 bytes of padding and the struct is obviously still a multiple of
PG_CACHE_LINE_SIZE but is now 1024 bytes (instead of 896).

For PgAioUringContext at completion_lock (like suggested by Andres in [1]), which
is also the start of the struct.

I checked and the padding for those are exactly the same after the changes.

0002, is also making use of alignas in ItemPointerData, but this one is more
tricky so I'm not sure that's worth it (given the fact that we still need to
keep pg_attribute_aligned() as explained by Peter in [2]).

[1]: https://postgr.es/m/lsgnps74ictxtm5karcobenxtt5rvoylvbvx7ja6r5rjcund7v%40owxqgv7gisns
[2]: https://postgr.es/m/46f05236-d4d4-4b4e-84d4-faa500f14691%40eisentraut.org

Regards,

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

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

    Peter Eisentraut <peter@eisentraut.org> — 2026-03-16T10:42:35Z

    On 24.02.26 12:28, Bertrand Drouvot wrote:
>> You can/should use C11 standard alignas(), so you don't need to worry about
>> whether it's supported or not.
> 
> Oh right, I did not notice 300c8f53247 and following like e7075a3405c, d4c0f91f7d5
> and 97e04c74bed.
> 
> PFA, 0001 doing so for PGPROC and PgAioUringContext. As those are typedef,
> the patch puts alignas within the struct.
> 
> For PGPROC at the start of the struct, I think that placing it on the first member
> is the right location because it ensures the whole struct is aligned to PG_CACHE_LINE_SIZE
> without adding padding before this member. For example if I set it on backendType,
> then it adds 100 bytes of padding and the struct is obviously still a multiple of
> PG_CACHE_LINE_SIZE but is now 1024 bytes (instead of 896).
> 
> For PgAioUringContext at completion_lock (like suggested by Andres in [1]), which
> is also the start of the struct.
> 
> I checked and the padding for those are exactly the same after the changes.

I have committed the 0001 patch.

> 0002, is also making use of alignas in ItemPointerData, but this one is more
> tricky so I'm not sure that's worth it (given the fact that we still need to
> keep pg_attribute_aligned() as explained by Peter in [2]).

This doesn't seem worth it to me.  Let's skip it.

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

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2026-03-16T11:16:51Z

    Hi,

On Mon, Mar 16, 2026 at 11:42:35AM +0100, Peter Eisentraut wrote:
> On 24.02.26 12:28, Bertrand Drouvot wrote:
> > > You can/should use C11 standard alignas(), so you don't need to worry about
> > > whether it's supported or not.
> > 
> > Oh right, I did not notice 300c8f53247 and following like e7075a3405c, d4c0f91f7d5
> > and 97e04c74bed.
> > 
> > PFA, 0001 doing so for PGPROC and PgAioUringContext. As those are typedef,
> > the patch puts alignas within the struct.
> > 
> > For PGPROC at the start of the struct, I think that placing it on the first member
> > is the right location because it ensures the whole struct is aligned to PG_CACHE_LINE_SIZE
> > without adding padding before this member. For example if I set it on backendType,
> > then it adds 100 bytes of padding and the struct is obviously still a multiple of
> > PG_CACHE_LINE_SIZE but is now 1024 bytes (instead of 896).
> > 
> > For PgAioUringContext at completion_lock (like suggested by Andres in [1]), which
> > is also the start of the struct.
> > 
> > I checked and the padding for those are exactly the same after the changes.
> 
> I have committed the 0001 patch.

Thanks! I don't know why but I don't see it in https://git.postgresql.org/gitweb/?p=postgresql.git;a=summary,
or in the github repo though.

> > 0002, is also making use of alignas in ItemPointerData, but this one is more
> > tricky so I'm not sure that's worth it (given the fact that we still need to
> > keep pg_attribute_aligned() as explained by Peter in [2]).
> 
> This doesn't seem worth it to me.  Let's skip it.

Yeah, that makes sense.

Regards,

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

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

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2026-03-16T15:52:59Z

    Hi,

On Mon, Mar 16, 2026 at 11:16:51AM +0000, Bertrand Drouvot wrote:
> Hi,
> 
> On Mon, Mar 16, 2026 at 11:42:35AM +0100, Peter Eisentraut wrote:
> > On 24.02.26 12:28, Bertrand Drouvot wrote:
> > > > You can/should use C11 standard alignas(), so you don't need to worry about
> > > > whether it's supported or not.
> > > 
> > > Oh right, I did not notice 300c8f53247 and following like e7075a3405c, d4c0f91f7d5
> > > and 97e04c74bed.
> > > 
> > > PFA, 0001 doing so for PGPROC and PgAioUringContext. As those are typedef,
> > > the patch puts alignas within the struct.
> > > 
> > > For PGPROC at the start of the struct, I think that placing it on the first member
> > > is the right location because it ensures the whole struct is aligned to PG_CACHE_LINE_SIZE
> > > without adding padding before this member. For example if I set it on backendType,
> > > then it adds 100 bytes of padding and the struct is obviously still a multiple of
> > > PG_CACHE_LINE_SIZE but is now 1024 bytes (instead of 896).
> > > 
> > > For PgAioUringContext at completion_lock (like suggested by Andres in [1]), which
> > > is also the start of the struct.
> > > 
> > > I checked and the padding for those are exactly the same after the changes.
> > 
> > I have committed the 0001 patch.
> 
> Thanks! I don't know why but I don't see it in https://git.postgresql.org/gitweb/?p=postgresql.git;a=summary,
> or in the github repo though.

FWIW, I reached out to sysadmins@ and it has now been fixed.

Regards,

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