pgsql: Separate RecoveryConflictReasons from procsignals

Started by Heikki Linnakangas2 months ago4 messageshackers
Jump to latest
#1Heikki Linnakangas
heikki.linnakangas@enterprisedb.com

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: /messages/by-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(-)

#2Bertrand Drouvot
bertranddrouvot.pg@gmail.com
In reply to: Heikki Linnakangas (#1)
Re: pgsql: Separate RecoveryConflictReasons from procsignals

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]/messages/by-id/tw53roer2j4quxh7vlyv62drc5fo6c6zdltvl6d2dttqa62uhi@stwlpdwlftpj).

[1]: /messages/by-id/tw53roer2j4quxh7vlyv62drc5fo6c6zdltvl6d2dttqa62uhi@stwlpdwlftpj

Regards,

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

#3Heikki Linnakangas
heikki.linnakangas@enterprisedb.com
In reply to: Bertrand Drouvot (#2)
Re: pgsql: Separate RecoveryConflictReasons from procsignals

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]: /messages/by-id/tw53roer2j4quxh7vlyv62drc5fo6c6zdltvl6d2dttqa62uhi@stwlpdwlftpj

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

#4Andres Freund
andres@anarazel.de
In reply to: Heikki Linnakangas (#3)
Re: pgsql: Separate RecoveryConflictReasons from procsignals

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]: /messages/by-id/tw53roer2j4quxh7vlyv62drc5fo6c6zdltvl6d2dttqa62uhi@stwlpdwlftpj

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