Thread: pgsql: Move cancel key generation to after forking the backend
Move cancel key generation to after forking the backend Move responsibility of generating the cancel key to the backend process. The cancel key is now generated after forking, and the backend advertises it in the ProcSignal array. When a cancel request arrives, the backend handling it scans the ProcSignal array to find the target pid and cancel key. This is similar to how this previously worked in the EXEC_BACKEND case with the ShmemBackendArray, just reusing the ProcSignal array. One notable change is that we no longer generate cancellation keys for non-backend processes. We generated them before just to prevent a malicious user from canceling them; the keys for non-backend processes were never actually given to anyone. There is now an explicit flag indicating whether a process has a valid key or not. I wrote this originally in preparation for supporting longer cancel keys, but it's a nice cleanup on its own. Reviewed-by: Jelte Fennema-Nio Discussion: https://www.postgresql.org/message-id/508d0505-8b7a-4864-a681-e7e5edfe32aa@iki.fi Branch ------ master Details ------- https://git.postgresql.org/pg/commitdiff/9d9b9d46f3c509c722ebbf2a1e7dc6296a6c711d Modified Files -------------- src/backend/postmaster/auxprocess.c | 2 +- src/backend/postmaster/launch_backend.c | 10 +- src/backend/postmaster/postmaster.c | 196 +------------------------------- src/backend/storage/ipc/ipci.c | 11 -- src/backend/storage/ipc/procsignal.c | 185 +++++++++++++++++++++++------- src/backend/tcop/backend_startup.c | 9 +- src/backend/tcop/postgres.c | 17 +++ src/backend/utils/init/globals.c | 3 +- src/backend/utils/init/postinit.c | 2 +- src/include/miscadmin.h | 1 + src/include/postmaster/postmaster.h | 9 -- src/include/storage/procsignal.h | 10 +- 12 files changed, 193 insertions(+), 262 deletions(-)
> On 29 Jul 2024, at 15:18, Heikki Linnakangas <heikki.linnakangas@iki.fi> wrote: > > Move cancel key generation to after forking the backend longfin seems a tad sad about this one: procsignal.c:87:3: error: redefinition of typedef 'ProcSignalHeader' is a C11 feature [-Werror,-Wtypedef-redefinition] } ProcSignalHeader; ^ ../../../../src/include/storage/procsignal.h:77:33: note: previous definition is here typedef struct ProcSignalHeader ProcSignalHeader; ^ 1 error generated. make[4]: *** [procsignal.o] Error 1 -- Daniel Gustafsson
On 29/07/2024 16:23, Daniel Gustafsson wrote: >> On 29 Jul 2024, at 15:18, Heikki Linnakangas <heikki.linnakangas@iki.fi> wrote: >> >> Move cancel key generation to after forking the backend > > longfin seems a tad sad about this one: > > procsignal.c:87:3: error: redefinition of typedef 'ProcSignalHeader' is a C11 feature [-Werror,-Wtypedef-redefinition] > } ProcSignalHeader; > ^ > ../../../../src/include/storage/procsignal.h:77:33: note: previous definition is here > typedef struct ProcSignalHeader ProcSignalHeader; > ^ > 1 error generated. > make[4]: *** [procsignal.o] Error 1 Yep, thanks, just noticed and fixed it myself. I should add that to the list of compiler options I use... -- Heikki Linnakangas Neon (https://neon.tech)
On 29/07/2024 16:25, Heikki Linnakangas wrote: > On 29/07/2024 16:23, Daniel Gustafsson wrote: >>> On 29 Jul 2024, at 15:18, Heikki Linnakangas <heikki.linnakangas@iki.fi> wrote: >>> >>> Move cancel key generation to after forking the backend >> >> longfin seems a tad sad about this one: >> >> procsignal.c:87:3: error: redefinition of typedef 'ProcSignalHeader' is a C11 feature [-Werror,-Wtypedef-redefinition] >> } ProcSignalHeader; >> ^ >> ../../../../src/include/storage/procsignal.h:77:33: note: previous definition is here >> typedef struct ProcSignalHeader ProcSignalHeader; >> ^ >> 1 error generated. >> make[4]: *** [procsignal.o] Error 1 > > Yep, thanks, just noticed and fixed it myself. I should add that to the > list of compiler options I use... The --disable-spinlocks animals are still failing, however. Weird. I can reproduce it locally, and started to debug but I have no clue what's wrong. I'll keep debugging, but I'm all ears if anyone has ideas. -- Heikki Linnakangas Neon (https://neon.tech)
On 29/07/2024 17:30, Heikki Linnakangas wrote: > On 29/07/2024 16:25, Heikki Linnakangas wrote: >> On 29/07/2024 16:23, Daniel Gustafsson wrote: >>>> On 29 Jul 2024, at 15:18, Heikki Linnakangas <heikki.linnakangas@iki.fi> wrote: >>>> >>>> Move cancel key generation to after forking the backend >>> >>> longfin seems a tad sad about this one: >>> >>> procsignal.c:87:3: error: redefinition of typedef 'ProcSignalHeader' is a C11 feature [-Werror,-Wtypedef-redefinition] >>> } ProcSignalHeader; >>> ^ >>> ../../../../src/include/storage/procsignal.h:77:33: note: previous definition is here >>> typedef struct ProcSignalHeader ProcSignalHeader; >>> ^ >>> 1 error generated. >>> make[4]: *** [procsignal.o] Error 1 >> >> Yep, thanks, just noticed and fixed it myself. I should add that to the >> list of compiler options I use... > > The --disable-spinlocks animals are still failing, however. Weird. I can > reproduce it locally, and started to debug but I have no clue what's > wrong. I'll keep debugging, but I'm all ears if anyone has ideas. Found & fixed. I released the spinlock twice in EmitProcSignalBarrier. Oops. I'm a little surprised there isn't an assertion for that. Would be useful. This might have gone unnoticed for a long time if not for the --disable-spinlocks implementation, and we've been talking about removing that. -- Heikki Linnakangas Neon (https://neon.tech)