public inbox for [email protected]help / color / mirror / Atom feed
pgsql: Refactor how some aux processes advertise their ProcNumber 8+ messages / 6 participants [nested] [flat]
* pgsql: Refactor how some aux processes advertise their ProcNumber @ 2026-07-08 17:43 Heikki Linnakangas <[email protected]> 0 siblings, 2 replies; 8+ messages in thread From: Heikki Linnakangas @ 2026-07-08 17:43 UTC (permalink / raw) To: [email protected] Refactor how some aux processes advertise their ProcNumber This moves the responsibility of setting the ProcGlobal->walwriterProc and checkpointerProc fields into InitAuxiliaryProcess. Also switch to the same pattern to advertise the autovacuum launcher's ProcNumber, replacing the ad hoc av_launcherpid field in shared memory. This can easily be extended to other aux processes in the future, if other processes need to find them. Switch to pg_atomic_uint32 for the fields. Seems easier to reason about than volatile pointers. There was some precedence for that, as were already using pg_atomic_uint32 for the procArrayGroupFirst and clogGroupFirst fields, which also store ProcNumbers. Todo: We could also replace WalRecv->procno with this, but that's a little more code churn so I left that for the future. Reviewed-by: Andres Freund <[email protected]> Discussion: https://www.postgresql.org/message-id/[email protected] Branch ------ master Details ------- https://git.postgresql.org/pg/commitdiff/842169b6c110f48c9f7a2dc29d0cedcc97c2ff29 Modified Files -------------- src/backend/access/transam/xlog.c | 3 +-- src/backend/postmaster/autovacuum.c | 19 ++++++++++--------- src/backend/postmaster/checkpointer.c | 14 ++++---------- src/backend/postmaster/walwriter.c | 6 ------ src/backend/storage/lmgr/proc.c | 32 ++++++++++++++++++++++++++++++-- src/include/storage/proc.h | 7 ++++--- 6 files changed, 49 insertions(+), 32 deletions(-) ^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: pgsql: Refactor how some aux processes advertise their ProcNumber @ 2026-07-08 21:57 Thom Brown <[email protected]> parent: Heikki Linnakangas <[email protected]> 1 sibling, 1 reply; 8+ messages in thread From: Thom Brown @ 2026-07-08 21:57 UTC (permalink / raw) To: Heikki Linnakangas <[email protected]>; +Cc: [email protected] On Wed, 8 Jul 2026 at 18:43, Heikki Linnakangas <[email protected]> wrote: > > Refactor how some aux processes advertise their ProcNumber > > This moves the responsibility of setting the ProcGlobal->walwriterProc > and checkpointerProc fields into InitAuxiliaryProcess. Also switch to > the same pattern to advertise the autovacuum launcher's ProcNumber, > replacing the ad hoc av_launcherpid field in shared memory. This can > easily be extended to other aux processes in the future, if other > processes need to find them. > > Switch to pg_atomic_uint32 for the fields. Seems easier to reason > about than volatile pointers. There was some precedence for that, as > were already using pg_atomic_uint32 for the procArrayGroupFirst and > clogGroupFirst fields, which also store ProcNumbers. > > Todo: We could also replace WalRecv->procno with this, but that's a > little more code churn so I left that for the future. > > Reviewed-by: Andres Freund <[email protected]> > Discussion: https://www.postgresql.org/message-id/[email protected] > > Branch > ------ > master > > Details > ------- > https://git.postgresql.org/pg/commitdiff/842169b6c110f48c9f7a2dc29d0cedcc97c2ff29 > > Modified Files > -------------- > src/backend/access/transam/xlog.c | 3 +-- > src/backend/postmaster/autovacuum.c | 19 ++++++++++--------- > src/backend/postmaster/checkpointer.c | 14 ++++---------- > src/backend/postmaster/walwriter.c | 6 ------ > src/backend/storage/lmgr/proc.c | 32 ++++++++++++++++++++++++++++++-- > src/include/storage/proc.h | 7 ++++--- > 6 files changed, 49 insertions(+), 32 deletions(-) @@ -724,6 +725,14 @@ InitAuxiliaryProcess(void) */ PGSemaphoreReset(MyProc->sem); + /* Some aux processes are also advertised in ProcGlobal */ + if (MyBackendType == B_AUTOVAC_LAUNCHER) + pg_atomic_write_u32(&ProcGlobal->avLauncherProc, MyProcNumber); I'm looking for the places this gets called from. I can only see auxprocess.c which the launcher doesn't go through. The same question for similar code in AuxiliaryProcKill(). Thom ^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: pgsql: Refactor how some aux processes advertise their ProcNumber @ 2026-07-09 00:57 Fujii Masao <[email protected]> parent: Thom Brown <[email protected]> 0 siblings, 1 reply; 8+ messages in thread From: Fujii Masao @ 2026-07-09 00:57 UTC (permalink / raw) To: Thom Brown <[email protected]>; +Cc: Heikki Linnakangas <[email protected]>; [email protected] On Thu, Jul 9, 2026 at 6:57 AM Thom Brown <[email protected]> wrote: > @@ -724,6 +725,14 @@ InitAuxiliaryProcess(void) > */ > PGSemaphoreReset(MyProc->sem); > > + /* Some aux processes are also advertised in ProcGlobal */ > + if (MyBackendType == B_AUTOVAC_LAUNCHER) > + pg_atomic_write_u32(&ProcGlobal->avLauncherProc, MyProcNumber); > > I'm looking for the places this gets called from. I can only see > auxprocess.c which the launcher doesn't go through. The same question > for similar code in AuxiliaryProcKill(). You're right. The autovacuum launcher does not go through AuxiliaryProcessMainCommon(), so it never calls InitAuxiliaryProcess(). Instead, it initializes its PGPROC with InitProcess(). As a result, the avLauncherProc assignment added to InitAuxiliaryProcess() is never executed for the launcher, and the corresponding cleanup in AuxiliaryProcKill() is never reached either. It seems we should move the avLauncherProc set/clear operations to InitProcess()/ProcKill() path, as in the attached patch. Regards, -- Fujii Masao Attachments: [application/octet-stream] v1-0001-Advertise-autovacuum-launcher-s-ProcNumber-from-I.patch (2.0K, ../../CAHGQGwH8n4BMjBJcL1ob6YTJYn1kTmVwO6mEYoEGyrGyNXaaXw@mail.gmail.com/2-v1-0001-Advertise-autovacuum-launcher-s-ProcNumber-from-I.patch) download | inline diff: From 86d84b81e53497cdda9722cb223df53970491714 Mon Sep 17 00:00:00 2001 From: Fujii Masao <[email protected]> Date: Thu, 9 Jul 2026 09:23:46 +0900 Subject: [PATCH v1] Advertise autovacuum launcher's ProcNumber from InitProcess --- src/backend/storage/lmgr/proc.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c index 59640bb4f97..ce4425b5568 100644 --- a/src/backend/storage/lmgr/proc.c +++ b/src/backend/storage/lmgr/proc.c @@ -550,6 +550,9 @@ InitProcess(void) */ PGSemaphoreReset(MyProc->sem); + if (MyBackendType == B_AUTOVAC_LAUNCHER) + pg_atomic_write_u32(&ProcGlobal->avLauncherProc, MyProcNumber); + /* * Arrange to clean up at backend exit. */ @@ -726,8 +729,6 @@ InitAuxiliaryProcess(void) PGSemaphoreReset(MyProc->sem); /* Some aux processes are also advertised in ProcGlobal */ - if (MyBackendType == B_AUTOVAC_LAUNCHER) - pg_atomic_write_u32(&ProcGlobal->avLauncherProc, MyProcNumber); if (MyBackendType == B_WAL_WRITER) pg_atomic_write_u32(&ProcGlobal->walwriterProc, MyProcNumber); if (MyBackendType == B_CHECKPOINTER) @@ -989,6 +990,12 @@ ProcKill(int code, Datum arg) SwitchBackToLocalLatch(); DisownLatch(&MyProc->procLatch); + if (MyBackendType == B_AUTOVAC_LAUNCHER) + { + Assert(pg_atomic_read_u32(&ProcGlobal->avLauncherProc) == MyProcNumber); + pg_atomic_write_u32(&ProcGlobal->avLauncherProc, INVALID_PROC_NUMBER); + } + proc = MyProc; procgloballist = proc->procgloballist; @@ -1113,11 +1120,6 @@ AuxiliaryProcKill(int code, Datum arg) /* * If this was one of the aux processes advertised in ProcGlobal, clear it */ - if (MyBackendType == B_AUTOVAC_LAUNCHER) - { - Assert(pg_atomic_read_u32(&ProcGlobal->avLauncherProc) == MyProcNumber); - pg_atomic_write_u32(&ProcGlobal->avLauncherProc, INVALID_PROC_NUMBER); - } if (MyBackendType == B_WAL_WRITER) { Assert(pg_atomic_read_u32(&ProcGlobal->walwriterProc) == MyProcNumber); -- 2.55.0 ^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: pgsql: Refactor how some aux processes advertise their ProcNumber @ 2026-07-09 03:10 Nathan Bossart <[email protected]> parent: Heikki Linnakangas <[email protected]> 1 sibling, 1 reply; 8+ messages in thread From: Nathan Bossart @ 2026-07-09 03:10 UTC (permalink / raw) To: Heikki Linnakangas <[email protected]>; +Cc: [email protected] On Wed, Jul 08, 2026 at 05:43:01PM +0000, Heikki Linnakangas wrote: > Switch to pg_atomic_uint32 for the fields. Seems easier to reason > about than volatile pointers. There was some precedence for that, as > were already using pg_atomic_uint32 for the procArrayGroupFirst and > clogGroupFirst fields, which also store ProcNumbers. Since removing volatile qualifiers is on my mind, I noticed that this patch makes it possible to remove two more. Patch attached. -- nathan diff --git a/src/backend/postmaster/checkpointer.c b/src/backend/postmaster/checkpointer.c index a09d4097d51..f6351f1eb10 100644 --- a/src/backend/postmaster/checkpointer.c +++ b/src/backend/postmaster/checkpointer.c @@ -1114,8 +1114,7 @@ RequestCheckpoint(int flags) #define MAX_SIGNAL_TRIES 600 /* max wait 60.0 sec */ for (ntries = 0;; ntries++) { - volatile PROC_HDR *procglobal = ProcGlobal; - ProcNumber checkpointerProc = pg_atomic_read_u32(&procglobal->checkpointerProc); + ProcNumber checkpointerProc = pg_atomic_read_u32(&ProcGlobal->checkpointerProc); if (checkpointerProc == INVALID_PROC_NUMBER) { @@ -1536,8 +1535,7 @@ FirstCallSinceLastCheckpoint(void) void WakeupCheckpointer(void) { - volatile PROC_HDR *procglobal = ProcGlobal; - ProcNumber checkpointerProc = pg_atomic_read_u32(&procglobal->checkpointerProc); + ProcNumber checkpointerProc = pg_atomic_read_u32(&ProcGlobal->checkpointerProc); if (checkpointerProc != INVALID_PROC_NUMBER) SetLatch(&GetPGProcByNumber(checkpointerProc)->procLatch); Attachments: [text/plain] remove_more_volatile.patch (1.0K, ../../ak8RP8xn5_H0BrJV@nathan/2-remove_more_volatile.patch) download | inline diff: diff --git a/src/backend/postmaster/checkpointer.c b/src/backend/postmaster/checkpointer.c index a09d4097d51..f6351f1eb10 100644 --- a/src/backend/postmaster/checkpointer.c +++ b/src/backend/postmaster/checkpointer.c @@ -1114,8 +1114,7 @@ RequestCheckpoint(int flags) #define MAX_SIGNAL_TRIES 600 /* max wait 60.0 sec */ for (ntries = 0;; ntries++) { - volatile PROC_HDR *procglobal = ProcGlobal; - ProcNumber checkpointerProc = pg_atomic_read_u32(&procglobal->checkpointerProc); + ProcNumber checkpointerProc = pg_atomic_read_u32(&ProcGlobal->checkpointerProc); if (checkpointerProc == INVALID_PROC_NUMBER) { @@ -1536,8 +1535,7 @@ FirstCallSinceLastCheckpoint(void) void WakeupCheckpointer(void) { - volatile PROC_HDR *procglobal = ProcGlobal; - ProcNumber checkpointerProc = pg_atomic_read_u32(&procglobal->checkpointerProc); + ProcNumber checkpointerProc = pg_atomic_read_u32(&ProcGlobal->checkpointerProc); if (checkpointerProc != INVALID_PROC_NUMBER) SetLatch(&GetPGProcByNumber(checkpointerProc)->procLatch); ^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: pgsql: Refactor how some aux processes advertise their ProcNumber @ 2026-07-09 11:09 Thom Brown <[email protected]> parent: Nathan Bossart <[email protected]> 0 siblings, 0 replies; 8+ messages in thread From: Thom Brown @ 2026-07-09 11:09 UTC (permalink / raw) To: Nathan Bossart <[email protected]>; +Cc: Heikki Linnakangas <[email protected]>; [email protected] On Thu, 9 Jul 2026 at 04:11, Nathan Bossart <[email protected]> wrote: > > On Wed, Jul 08, 2026 at 05:43:01PM +0000, Heikki Linnakangas wrote: > > Switch to pg_atomic_uint32 for the fields. Seems easier to reason > > about than volatile pointers. There was some precedence for that, as > > were already using pg_atomic_uint32 for the procArrayGroupFirst and > > clogGroupFirst fields, which also store ProcNumbers. > > Since removing volatile qualifiers is on my mind, I noticed that this patch > makes it possible to remove two more. Patch attached. +1 ^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: pgsql: Refactor how some aux processes advertise their ProcNumber @ 2026-07-09 12:17 Heikki Linnakangas <[email protected]> parent: Fujii Masao <[email protected]> 0 siblings, 2 replies; 8+ messages in thread From: Heikki Linnakangas @ 2026-07-09 12:17 UTC (permalink / raw) To: Fujii Masao <[email protected]>; Thom Brown <[email protected]>; +Cc: [email protected] On 09/07/2026 03:57, Fujii Masao wrote: > On Thu, Jul 9, 2026 at 6:57 AM Thom Brown <[email protected]> wrote: >> @@ -724,6 +725,14 @@ InitAuxiliaryProcess(void) >> */ >> PGSemaphoreReset(MyProc->sem); >> >> + /* Some aux processes are also advertised in ProcGlobal */ >> + if (MyBackendType == B_AUTOVAC_LAUNCHER) >> + pg_atomic_write_u32(&ProcGlobal->avLauncherProc, MyProcNumber); >> >> I'm looking for the places this gets called from. I can only see >> auxprocess.c which the launcher doesn't go through. The same question >> for similar code in AuxiliaryProcKill(). > > You're right. The autovacuum launcher does not go through > AuxiliaryProcessMainCommon(), so it never calls > InitAuxiliaryProcess(). Instead, it initializes its PGPROC with > InitProcess(). > > As a result, the avLauncherProc assignment added to > InitAuxiliaryProcess() is never executed for the launcher, and the > corresponding cleanup in AuxiliaryProcKill() is never reached either. Huh, you're right. That raises some questions: Why is the autovacuum launcher not an aux process? I guess it's because it needs to acquire a heavy-weight lock in order to read the list of databases. How come none of the tests failed? I guess the path to wake up the launcher isn't critical for correctness, it just makes autovacuum slower to react. > It seems we should move the avLauncherProc set/clear operations to > InitProcess()/ProcKill() path, as in the attached patch. Thanks! Committed that, and also Nathan's volatile removal. - Heikki ^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: pgsql: Refactor how some aux processes advertise their ProcNumber @ 2026-07-09 12:34 Fujii Masao <[email protected]> parent: Heikki Linnakangas <[email protected]> 1 sibling, 0 replies; 8+ messages in thread From: Fujii Masao @ 2026-07-09 12:34 UTC (permalink / raw) To: Heikki Linnakangas <[email protected]>; +Cc: Thom Brown <[email protected]>; [email protected] On Thu, Jul 9, 2026 at 9:17 PM Heikki Linnakangas <[email protected]> wrote: > Why is the autovacuum launcher not an aux process? I guess it's because > it needs to acquire a heavy-weight lock in order to read the list of > databases. That seems to be one of the reasons, based on the comment about auxiliary processes in miscadmin.h: ------------------------- * Auxiliary processes. These have PGPROC entries, but they are not * attached to any particular database, and cannot run transactions or * even take heavyweight locks. There can be only one of each of these * running at a time, except for IO workers. ------------------------- Regards, -- Fujii Masao ^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: pgsql: Refactor how some aux processes advertise their ProcNumber @ 2026-07-09 12:36 Álvaro Herrera <[email protected]> parent: Heikki Linnakangas <[email protected]> 1 sibling, 0 replies; 8+ messages in thread From: Álvaro Herrera @ 2026-07-09 12:36 UTC (permalink / raw) To: Heikki Linnakangas <[email protected]>; +Cc: Fujii Masao <[email protected]>; Thom Brown <[email protected]>; [email protected] On 2026-Jul-09, Heikki Linnakangas wrote: > Why is the autovacuum launcher not an aux process? I guess it's because it > needs to acquire a heavy-weight lock in order to read the list of databases. Because it scans pg_database. commit 00e6a16d01683762c2f34eb4909fc739093ab3bf Author: Tom Lane <[email protected]> [] AuthorDate: Mon Aug 31 19:41:00 2009 +0000 CommitDate: Mon Aug 31 19:41:00 2009 +0000 Change the autovacuum launcher to read pg_database directly, rather than via the "flat files" facility. This requires making it enough like a backend to be able to run transactions; it's no longer an "auxiliary process" but more like the autovacuum worker processes. Also, its signal handling has to be brought into line with backends/workers. In particular, since it now has to handle procsignal.c processing, the special autovac-launcher-only signal conditions are moved to SIGUSR2. Alvaro, with some cleanup from Tom -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/ "If you want to have good ideas, you must have many ideas. Most of them will be wrong, and what you have to learn is which ones to throw away." (Linus Pauling) ^ permalink raw reply [nested|flat] 8+ messages in thread
end of thread, other threads:[~2026-07-09 12:36 UTC | newest] Thread overview: 8+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2026-07-08 17:43 pgsql: Refactor how some aux processes advertise their ProcNumber Heikki Linnakangas <[email protected]> 2026-07-08 21:57 ` Thom Brown <[email protected]> 2026-07-09 00:57 ` Fujii Masao <[email protected]> 2026-07-09 12:17 ` Heikki Linnakangas <[email protected]> 2026-07-09 12:34 ` Fujii Masao <[email protected]> 2026-07-09 12:36 ` Álvaro Herrera <[email protected]> 2026-07-09 03:10 ` Nathan Bossart <[email protected]> 2026-07-09 11:09 ` Thom Brown <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox