On 04/07/2024 15:20, Jelte Fennema-Nio wrote:
On Thu, 4 Jul 2024 at 12:32, Heikki Linnakangas <hlinn...@iki.fi> wrote:
We currently don't do any locking on the ProcSignal array. For query
cancellations, that's good because a query cancel packet is processed
without having a PGPROC entry, so we cannot take LWLocks. We could use
spinlocks though. In this patch, I used memory barriers to ensure that
we load/store the pid and the cancellation key in a sensible order, so
that you cannot e.g. send a cancellation signal to a backend that's just
starting up and hasn't advertised its cancellation key in ProcSignal
yet. But I think this might be simpler and less error-prone by just
adding a spinlock to each ProcSignal slot. That would also fix the
existing race condition where we might set the pss_signalFlags flag for
a slot, when the process concurrently terminates and the slot is reused
for a different process. Because of that, we currently have this caveat:
"... all the signals are such that no harm is done if they're mistakenly
fired". With a spinlock, we could eliminate that race.
I think a spinlock would make this thing a whole concurrency stuff a
lot easier to reason about.
+ slot->pss_cancel_key_valid = false;
+ slot->pss_cancel_key = 0;
If no spinlock is added, I think these accesses should still be made
atomic writes. Otherwise data-races on those fields are still
possible, resulting in undefined behaviour. The memory barriers you
added don't prevent that afaict. With atomic operations there are
still race conditions, but no data-races.
Actually it seems like that same argument applies to the already
existing reading/writing of pss_pid: it's written/read using
non-atomic operations so data-races can occur and thus undefined
behaviour too.
Ok, here's a version with spinlocks.
I went back and forth on what exactly is protected by the spinlock. I
kept the "volatile sig_atomic_t" type for pss_signalFlags, so that it
can still be safely read without holding the spinlock in
CheckProcSignal, but all the functions that set the flags now hold the
spinlock. That removes the race condition that you might set the flag
for wrong slot, if the target backend exits and the slot is recycled.
The race condition was harmless and there were comments to note it, but
it doesn't occur anymore with the spinlock.
(Note: Thomas's "Interrupts vs signals" patch will remove ps_signalFlags
altogether. I'm looking forward to that.)
- volatile pid_t pss_pid;
+ pid_t pss_pid;
Why remove the volatile modifier?
Because I introduced a memory barrier to ensure the reads/writes of
pss_pid become visible to other processes in right order. That makes the
'volatile' unnecessary IIUC. With the spinlock, the point is moot however.
--
Heikki Linnakangas
Neon (https://neon.tech)
From 2e6035837c028abd3b645a8c038cac7179016eef Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakan...@iki.fi>
Date: Wed, 24 Jul 2024 19:06:32 +0300
Subject: [PATCH v4 1/1] 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-e7e5edfe3...@iki.fi
---
src/backend/postmaster/auxprocess.c | 2 +-
src/backend/postmaster/launch_backend.c | 6 -
src/backend/postmaster/postmaster.c | 196 +-----------------------
src/backend/storage/ipc/ipci.c | 11 --
src/backend/storage/ipc/procsignal.c | 172 ++++++++++++++++-----
src/backend/tcop/backend_startup.c | 9 +-
src/backend/tcop/postgres.c | 21 +++
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 | 3 +-
12 files changed, 175 insertions(+), 260 deletions(-)
diff --git a/src/backend/postmaster/auxprocess.c b/src/backend/postmaster/auxprocess.c
index b21eae7136..74b8a00c94 100644
--- a/src/backend/postmaster/auxprocess.c
+++ b/src/backend/postmaster/auxprocess.c
@@ -71,7 +71,7 @@ AuxiliaryProcessMainCommon(void)
BaseInit();
- ProcSignalInit();
+ ProcSignalInit(false, 0);
/*
* Auxiliary processes don't run transactions, but they may need a
diff --git a/src/backend/postmaster/launch_backend.c b/src/backend/postmaster/launch_backend.c
index e9fc982787..f235248c54 100644
--- a/src/backend/postmaster/launch_backend.c
+++ b/src/backend/postmaster/launch_backend.c
@@ -93,7 +93,6 @@ typedef int InheritableSocket;
typedef struct
{
char DataDir[MAXPGPATH];
- int32 MyCancelKey;
int MyPMChildSlot;
#ifndef WIN32
unsigned long UsedShmemSegID;
@@ -103,7 +102,6 @@ typedef struct
#endif
void *UsedShmemSegAddr;
slock_t *ShmemLock;
- struct bkend *ShmemBackendArray;
#ifndef HAVE_SPINLOCKS
PGSemaphore *SpinlockSemaArray;
#endif
@@ -698,7 +696,6 @@ save_backend_variables(BackendParameters *param, ClientSocket *client_sock,
strlcpy(param->DataDir, DataDir, MAXPGPATH);
- param->MyCancelKey = MyCancelKey;
param->MyPMChildSlot = MyPMChildSlot;
#ifdef WIN32
@@ -708,7 +705,6 @@ save_backend_variables(BackendParameters *param, ClientSocket *client_sock,
param->UsedShmemSegAddr = UsedShmemSegAddr;
param->ShmemLock = ShmemLock;
- param->ShmemBackendArray = ShmemBackendArray;
#ifndef HAVE_SPINLOCKS
param->SpinlockSemaArray = SpinlockSemaArray;
@@ -957,7 +953,6 @@ restore_backend_variables(BackendParameters *param)
SetDataDir(param->DataDir);
- MyCancelKey = param->MyCancelKey;
MyPMChildSlot = param->MyPMChildSlot;
#ifdef WIN32
@@ -967,7 +962,6 @@ restore_backend_variables(BackendParameters *param)
UsedShmemSegAddr = param->UsedShmemSegAddr;
ShmemLock = param->ShmemLock;
- ShmemBackendArray = param->ShmemBackendArray;
#ifndef HAVE_SPINLOCKS
SpinlockSemaArray = param->SpinlockSemaArray;
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index 6f974a8d21..02442a4b85 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -168,7 +168,6 @@
typedef struct bkend
{
pid_t pid; /* process id of backend */
- int32 cancel_key; /* cancel key for cancels for this backend */
int child_slot; /* PMChildSlot for this backend, if any */
int bkend_type; /* child process flavor, see above */
bool dead_end; /* is it going to send an error and quit? */
@@ -178,10 +177,6 @@ typedef struct bkend
static dlist_head BackendList = DLIST_STATIC_INIT(BackendList);
-#ifdef EXEC_BACKEND
-NON_EXEC_STATIC Backend *ShmemBackendArray;
-#endif
-
BackgroundWorker *MyBgworkerEntry = NULL;
@@ -413,7 +408,6 @@ static int ServerLoop(void);
static int BackendStartup(ClientSocket *client_sock);
static void report_fork_failure_to_client(ClientSocket *client_sock, int errnum);
static CAC_state canAcceptConnections(int backend_type);
-static bool RandomCancelKey(int32 *cancel_key);
static void signal_child(pid_t pid, int signal);
static void sigquit_child(pid_t pid);
static bool SignalSomeChildren(int signal, int target);
@@ -444,8 +438,6 @@ static void MaybeStartSlotSyncWorker(void);
(pmState == PM_RECOVERY || pmState == PM_HOT_STANDBY))) && \
PgArchCanRestart())
-#ifdef EXEC_BACKEND
-
#ifdef WIN32
#define WNOHANG 0 /* ignored, so any integer value will do */
@@ -462,10 +454,6 @@ typedef struct
} win32_deadchild_waitinfo;
#endif /* WIN32 */
-static void ShmemBackendArrayAdd(Backend *bn);
-static void ShmemBackendArrayRemove(Backend *bn);
-#endif /* EXEC_BACKEND */
-
/* Macros to check exit status of a child process */
#define EXIT_STATUS_0(st) ((st) == 0)
#define EXIT_STATUS_1(st) (WIFEXITED(st) && WEXITSTATUS(st) == 1)
@@ -1826,65 +1814,6 @@ ServerLoop(void)
}
}
-/*
- * The client has sent a cancel request packet, not a normal
- * start-a-new-connection packet. Perform the necessary processing.
- * Nothing is sent back to the client.
- */
-void
-processCancelRequest(int backendPID, int32 cancelAuthCode)
-{
- Backend *bp;
-
-#ifndef EXEC_BACKEND
- dlist_iter iter;
-#else
- int i;
-#endif
-
- /*
- * See if we have a matching backend. In the EXEC_BACKEND case, we can no
- * longer access the postmaster's own backend list, and must rely on the
- * duplicate array in shared memory.
- */
-#ifndef EXEC_BACKEND
- dlist_foreach(iter, &BackendList)
- {
- bp = dlist_container(Backend, elem, iter.cur);
-#else
- for (i = MaxLivePostmasterChildren() - 1; i >= 0; i--)
- {
- bp = (Backend *) &ShmemBackendArray[i];
-#endif
- if (bp->pid == backendPID)
- {
- if (bp->cancel_key == cancelAuthCode)
- {
- /* Found a match; signal that backend to cancel current op */
- ereport(DEBUG2,
- (errmsg_internal("processing cancel request: sending SIGINT to process %d",
- backendPID)));
- signal_child(bp->pid, SIGINT);
- }
- else
- /* Right PID, wrong key: no way, Jose */
- ereport(LOG,
- (errmsg("wrong key in cancel request for process %d",
- backendPID)));
- return;
- }
-#ifndef EXEC_BACKEND /* make GNU Emacs 26.1 see brace balance */
- }
-#else
- }
-#endif
-
- /* No matching backend */
- ereport(LOG,
- (errmsg("PID %d in cancel request did not match any process",
- backendPID)));
-}
-
/*
* canAcceptConnections --- check to see if database state allows connections
* of the specified type. backend_type can be BACKEND_TYPE_NORMAL,
@@ -2752,9 +2681,6 @@ CleanupBackgroundWorker(int pid,
/* Get it out of the BackendList and clear out remaining data */
dlist_delete(&rw->rw_backend->elem);
-#ifdef EXEC_BACKEND
- ShmemBackendArrayRemove(rw->rw_backend);
-#endif
/*
* It's possible that this background worker started some OTHER
@@ -2840,9 +2766,6 @@ CleanupBackend(int pid,
HandleChildCrash(pid, exitstatus, _("server process"));
return;
}
-#ifdef EXEC_BACKEND
- ShmemBackendArrayRemove(bp);
-#endif
}
if (bp->bgworker_notify)
{
@@ -2910,9 +2833,6 @@ HandleChildCrash(int pid, int exitstatus, const char *procname)
*/
(void) ReleasePostmasterChildSlot(rw->rw_child_slot);
dlist_delete(&rw->rw_backend->elem);
-#ifdef EXEC_BACKEND
- ShmemBackendArrayRemove(rw->rw_backend);
-#endif
pfree(rw->rw_backend);
rw->rw_backend = NULL;
rw->rw_pid = 0;
@@ -2945,9 +2865,6 @@ HandleChildCrash(int pid, int exitstatus, const char *procname)
if (!bp->dead_end)
{
(void) ReleasePostmasterChildSlot(bp->child_slot);
-#ifdef EXEC_BACKEND
- ShmemBackendArrayRemove(bp);
-#endif
}
dlist_delete(iter.cur);
pfree(bp);
@@ -3560,24 +3477,9 @@ BackendStartup(ClientSocket *client_sock)
return STATUS_ERROR;
}
- /*
- * Compute the cancel key that will be assigned to this backend. The
- * backend will have its own copy in the forked-off process' value of
- * MyCancelKey, so that it can transmit the key to the frontend.
- */
- if (!RandomCancelKey(&MyCancelKey))
- {
- pfree(bn);
- ereport(LOG,
- (errcode(ERRCODE_INTERNAL_ERROR),
- errmsg("could not generate random cancel key")));
- return STATUS_ERROR;
- }
-
/* Pass down canAcceptConnections state */
startup_data.canAcceptConnections = canAcceptConnections(BACKEND_TYPE_NORMAL);
bn->dead_end = (startup_data.canAcceptConnections != CAC_OK);
- bn->cancel_key = MyCancelKey;
/*
* Unless it's a dead_end child, assign it a child slot number
@@ -3621,11 +3523,6 @@ BackendStartup(ClientSocket *client_sock)
bn->bkend_type = BACKEND_TYPE_NORMAL; /* Can change later to WALSND */
dlist_push_head(&BackendList, &bn->elem);
-#ifdef EXEC_BACKEND
- if (!bn->dead_end)
- ShmemBackendArrayAdd(bn);
-#endif
-
return STATUS_OK;
}
@@ -3861,15 +3758,6 @@ dummy_handler(SIGNAL_ARGS)
{
}
-/*
- * Generate a random cancel key.
- */
-static bool
-RandomCancelKey(int32 *cancel_key)
-{
- return pg_strong_random(cancel_key, sizeof(int32));
-}
-
/*
* Count up number of child processes of specified types (dead_end children
* are always excluded).
@@ -3970,25 +3858,9 @@ StartAutovacuumWorker(void)
*/
if (canAcceptConnections(BACKEND_TYPE_AUTOVAC) == CAC_OK)
{
- /*
- * Compute the cancel key that will be assigned to this session. We
- * probably don't need cancel keys for autovac workers, but we'd
- * better have something random in the field to prevent unfriendly
- * people from sending cancels to them.
- */
- if (!RandomCancelKey(&MyCancelKey))
- {
- ereport(LOG,
- (errcode(ERRCODE_INTERNAL_ERROR),
- errmsg("could not generate random cancel key")));
- return;
- }
-
bn = (Backend *) palloc_extended(sizeof(Backend), MCXT_ALLOC_NO_OOM);
if (bn)
{
- bn->cancel_key = MyCancelKey;
-
/* Autovac workers are not dead_end and need a child slot */
bn->dead_end = false;
bn->child_slot = MyPMChildSlot = AssignPostmasterChildSlot();
@@ -3999,9 +3871,6 @@ StartAutovacuumWorker(void)
{
bn->bkend_type = BACKEND_TYPE_AUTOVAC;
dlist_push_head(&BackendList, &bn->elem);
-#ifdef EXEC_BACKEND
- ShmemBackendArrayAdd(bn);
-#endif
/* all OK */
return;
}
@@ -4133,11 +4002,10 @@ CreateOptsFile(int argc, char *argv[], char *fullprogname)
/*
* MaxLivePostmasterChildren
*
- * This reports the number of entries needed in per-child-process arrays
- * (the PMChildFlags array, and if EXEC_BACKEND the ShmemBackendArray).
- * These arrays include regular backends, autovac workers, walsenders
+ * This reports the number of entries needed in the per-child-process array
+ * (PMChildFlags). It includes regular backends, autovac workers, walsenders
* and background workers, but not special children nor dead_end children.
- * This allows the arrays to have a fixed maximum size, to wit the same
+ * This allows the array to have a fixed maximum size, to wit the same
* too-many-children limit enforced by canAcceptConnections(). The exact value
* isn't too critical as long as it's more than MaxBackends.
*/
@@ -4206,9 +4074,6 @@ do_start_bgworker(RegisteredBgWorker *rw)
ReportBackgroundWorkerPID(rw);
/* add new worker to lists of backends */
dlist_push_head(&BackendList, &rw->rw_backend->elem);
-#ifdef EXEC_BACKEND
- ShmemBackendArrayAdd(rw->rw_backend);
-#endif
return true;
}
@@ -4276,20 +4141,6 @@ assign_backendlist_entry(RegisteredBgWorker *rw)
return false;
}
- /*
- * Compute the cancel key that will be assigned to this session. We
- * probably don't need cancel keys for background workers, but we'd better
- * have something random in the field to prevent unfriendly people from
- * sending cancels to them.
- */
- if (!RandomCancelKey(&MyCancelKey))
- {
- ereport(LOG,
- (errcode(ERRCODE_INTERNAL_ERROR),
- errmsg("could not generate random cancel key")));
- return false;
- }
-
bn = palloc_extended(sizeof(Backend), MCXT_ALLOC_NO_OOM);
if (bn == NULL)
{
@@ -4299,7 +4150,6 @@ assign_backendlist_entry(RegisteredBgWorker *rw)
return false;
}
- bn->cancel_key = MyCancelKey;
bn->child_slot = MyPMChildSlot = AssignPostmasterChildSlot();
bn->bkend_type = BACKEND_TYPE_BGWORKER;
bn->dead_end = false;
@@ -4459,46 +4309,6 @@ PostmasterMarkPIDForWorkerNotify(int pid)
return false;
}
-#ifdef EXEC_BACKEND
-
-Size
-ShmemBackendArraySize(void)
-{
- return mul_size(MaxLivePostmasterChildren(), sizeof(Backend));
-}
-
-void
-ShmemBackendArrayAllocation(void)
-{
- Size size = ShmemBackendArraySize();
-
- ShmemBackendArray = (Backend *) ShmemAlloc(size);
- /* Mark all slots as empty */
- memset(ShmemBackendArray, 0, size);
-}
-
-static void
-ShmemBackendArrayAdd(Backend *bn)
-{
- /* The array slot corresponding to my PMChildSlot should be free */
- int i = bn->child_slot - 1;
-
- Assert(ShmemBackendArray[i].pid == 0);
- ShmemBackendArray[i] = *bn;
-}
-
-static void
-ShmemBackendArrayRemove(Backend *bn)
-{
- int i = bn->child_slot - 1;
-
- Assert(ShmemBackendArray[i].pid == bn->pid);
- /* Mark the slot as empty */
- ShmemBackendArray[i].pid = 0;
-}
-#endif /* EXEC_BACKEND */
-
-
#ifdef WIN32
/*
diff --git a/src/backend/storage/ipc/ipci.c b/src/backend/storage/ipc/ipci.c
index 2100150f01..c63384e7f3 100644
--- a/src/backend/storage/ipc/ipci.c
+++ b/src/backend/storage/ipc/ipci.c
@@ -152,9 +152,6 @@ CalculateShmemSize(int *num_semaphores)
size = add_size(size, WaitEventCustomShmemSize());
size = add_size(size, InjectionPointShmemSize());
size = add_size(size, SlotSyncShmemSize());
-#ifdef EXEC_BACKEND
- size = add_size(size, ShmemBackendArraySize());
-#endif
/* include additional requested shmem from preload libraries */
size = add_size(size, total_addin_request);
@@ -244,14 +241,6 @@ CreateSharedMemoryAndSemaphores(void)
/* Initialize subsystems */
CreateOrAttachShmemStructs();
-#ifdef EXEC_BACKEND
-
- /*
- * Alloc the win32 shared backend array
- */
- ShmemBackendArrayAllocation();
-#endif
-
/* Initialize dynamic shared memory facilities. */
dsm_postmaster_startup(shim);
diff --git a/src/backend/storage/ipc/procsignal.c b/src/backend/storage/ipc/procsignal.c
index 4ed9cedcdd..1c346dcc8f 100644
--- a/src/backend/storage/ipc/procsignal.c
+++ b/src/backend/storage/ipc/procsignal.c
@@ -47,9 +47,9 @@
* know the ProcNumber of the process you're signaling. (We do support
* signaling without ProcNumber, but it's a bit less efficient.)
*
- * The flags are actually declared as "volatile sig_atomic_t" for maximum
- * portability. This should ensure that loads and stores of the flag
- * values are atomic, allowing us to dispense with any explicit locking.
+ * The fields in each slot are protected by a spinlock, pss_mutex. pss_pid can
+ * also be read without holding the spinlock, as a quick preliminary check
+ * when searching for a particular PID in the array.
*
* pss_signalFlags are intended to be set in cases where we don't need to
* keep track of whether or not the target process has handled the signal,
@@ -62,8 +62,13 @@
*/
typedef struct
{
- volatile pid_t pss_pid;
+ pg_atomic_uint32 pss_pid;
+ bool pss_cancel_key_valid;
+ int32 pss_cancel_key;
volatile sig_atomic_t pss_signalFlags[NUM_PROCSIGNALS];
+ slock_t pss_mutex; /* protects the above fields */
+
+ /* Barrier-related fields (not protected by pss_mutex) */
pg_atomic_uint64 pss_barrierGeneration;
pg_atomic_uint32 pss_barrierCheckMask;
ConditionVariable pss_barrierCV;
@@ -141,7 +146,10 @@ ProcSignalShmemInit(void)
{
ProcSignalSlot *slot = &ProcSignal->psh_slot[i];
- slot->pss_pid = 0;
+ SpinLockInit(&slot->pss_mutex);
+ pg_atomic_init_u32(&slot->pss_pid, 0);
+ slot->pss_cancel_key_valid = false;
+ slot->pss_cancel_key = 0;
MemSet(slot->pss_signalFlags, 0, sizeof(slot->pss_signalFlags));
pg_atomic_init_u64(&slot->pss_barrierGeneration, PG_UINT64_MAX);
pg_atomic_init_u32(&slot->pss_barrierCheckMask, 0);
@@ -155,7 +163,7 @@ ProcSignalShmemInit(void)
* Register the current process in the ProcSignal array
*/
void
-ProcSignalInit(void)
+ProcSignalInit(bool cancel_key_valid, int32 cancel_key)
{
ProcSignalSlot *slot;
uint64 barrier_generation;
@@ -167,9 +175,13 @@ ProcSignalInit(void)
slot = &ProcSignal->psh_slot[MyProcNumber];
/* sanity check */
- if (slot->pss_pid != 0)
+ SpinLockAcquire(&slot->pss_mutex);
+ if (pg_atomic_read_u32(&slot->pss_pid) != 0)
+ {
+ SpinLockRelease(&slot->pss_mutex);
elog(LOG, "process %d taking over ProcSignal slot %d, but it's not empty",
MyProcPid, MyProcNumber);
+ }
/* Clear out any leftover signal reasons */
MemSet(slot->pss_signalFlags, 0, NUM_PROCSIGNALS * sizeof(sig_atomic_t));
@@ -189,10 +201,12 @@ ProcSignalInit(void)
barrier_generation =
pg_atomic_read_u64(&ProcSignal->psh_barrierGeneration);
pg_atomic_write_u64(&slot->pss_barrierGeneration, barrier_generation);
- pg_memory_barrier();
- /* Mark slot with my PID */
- slot->pss_pid = MyProcPid;
+ slot->pss_cancel_key_valid = cancel_key_valid;
+ slot->pss_cancel_key = cancel_key;
+ pg_atomic_write_u32(&slot->pss_pid, MyProcPid);
+
+ SpinLockRelease(&slot->pss_mutex);
/* Remember slot location for CheckProcSignal */
MyProcSignalSlot = slot;
@@ -210,6 +224,7 @@ ProcSignalInit(void)
static void
CleanupProcSignalState(int status, Datum arg)
{
+ pid_t old_pid;
ProcSignalSlot *slot = MyProcSignalSlot;
/*
@@ -221,25 +236,34 @@ CleanupProcSignalState(int status, Datum arg)
MyProcSignalSlot = NULL;
/* sanity check */
- if (slot->pss_pid != MyProcPid)
+ SpinLockAcquire(&slot->pss_mutex);
+ old_pid = pg_atomic_read_u32(&slot->pss_pid);
+ if (old_pid != MyProcPid)
{
/*
* don't ERROR here. We're exiting anyway, and don't want to get into
* infinite loop trying to exit
*/
+ SpinLockRelease(&slot->pss_mutex);
elog(LOG, "process %d releasing ProcSignal slot %d, but it contains %d",
- MyProcPid, (int) (slot - ProcSignal->psh_slot), (int) slot->pss_pid);
+ MyProcPid, (int) (slot - ProcSignal->psh_slot), (int) old_pid);
return; /* XXX better to zero the slot anyway? */
}
+ /* Mark the slot as unused */
+ pg_atomic_write_u32(&slot->pss_pid, 0);
+ slot->pss_cancel_key_valid = false;
+ slot->pss_cancel_key = 0;
+
/*
* Make this slot look like it's absorbed all possible barriers, so that
* no barrier waits block on it.
*/
pg_atomic_write_u64(&slot->pss_barrierGeneration, PG_UINT64_MAX);
- ConditionVariableBroadcast(&slot->pss_barrierCV);
- slot->pss_pid = 0;
+ SpinLockRelease(&slot->pss_mutex);
+
+ ConditionVariableBroadcast(&slot->pss_barrierCV);
}
/*
@@ -262,21 +286,16 @@ SendProcSignal(pid_t pid, ProcSignalReason reason, ProcNumber procNumber)
{
slot = &ProcSignal->psh_slot[procNumber];
- /*
- * Note: Since there's no locking, it's possible that the target
- * process detaches from shared memory and exits right after this
- * test, before we set the flag and send signal. And the signal slot
- * might even be recycled by a new process, so it's remotely possible
- * that we set a flag for a wrong process. That's OK, all the signals
- * are such that no harm is done if they're mistakenly fired.
- */
- if (slot->pss_pid == pid)
+ SpinLockAcquire(&slot->pss_mutex);
+ if (pg_atomic_read_u32(&slot->pss_pid) == pid)
{
/* Atomically set the proper flag */
slot->pss_signalFlags[reason] = true;
+ SpinLockRelease(&slot->pss_mutex);
/* Send signal */
return kill(pid, SIGUSR1);
}
+ SpinLockRelease(&slot->pss_mutex);
}
else
{
@@ -293,14 +312,18 @@ SendProcSignal(pid_t pid, ProcSignalReason reason, ProcNumber procNumber)
{
slot = &ProcSignal->psh_slot[i];
- if (slot->pss_pid == pid)
+ if (pg_atomic_read_u32(&slot->pss_pid) == pid)
{
- /* the above note about race conditions applies here too */
-
- /* Atomically set the proper flag */
- slot->pss_signalFlags[reason] = true;
- /* Send signal */
- return kill(pid, SIGUSR1);
+ SpinLockAcquire(&slot->pss_mutex);
+ if (pg_atomic_read_u32(&slot->pss_pid) == pid)
+ {
+ /* Atomically set the proper flag */
+ slot->pss_signalFlags[reason] = true;
+ SpinLockRelease(&slot->pss_mutex);
+ /* Send signal */
+ return kill(pid, SIGUSR1);
+ }
+ SpinLockRelease(&slot->pss_mutex);
}
}
}
@@ -368,13 +391,20 @@ EmitProcSignalBarrier(ProcSignalBarrierType type)
for (int i = NumProcSignalSlots - 1; i >= 0; i--)
{
volatile ProcSignalSlot *slot = &ProcSignal->psh_slot[i];
- pid_t pid = slot->pss_pid;
+ pid_t pid = pg_atomic_read_u32(&slot->pss_pid);
if (pid != 0)
{
- /* see SendProcSignal for details */
- slot->pss_signalFlags[PROCSIG_BARRIER] = true;
- kill(pid, SIGUSR1);
+ SpinLockAcquire(&slot->pss_mutex);
+ pid = pg_atomic_read_u32(&slot->pss_pid);
+ if (pid != 0)
+ {
+ /* see SendProcSignal for details */
+ slot->pss_signalFlags[PROCSIG_BARRIER] = true;
+ SpinLockRelease(&slot->pss_mutex);
+ kill(pid, SIGUSR1);
+ }
+ SpinLockRelease(&slot->pss_mutex);
}
}
@@ -414,7 +444,7 @@ WaitForProcSignalBarrier(uint64 generation)
WAIT_EVENT_PROC_SIGNAL_BARRIER))
ereport(LOG,
(errmsg("still waiting for backend with PID %d to accept ProcSignalBarrier",
- (int) slot->pss_pid)));
+ (int) pg_atomic_read_u32(&slot->pss_pid))));
oldval = pg_atomic_read_u64(&slot->pss_barrierGeneration);
}
ConditionVariableCancelSleep();
@@ -617,7 +647,11 @@ CheckProcSignal(ProcSignalReason reason)
if (slot != NULL)
{
- /* Careful here --- don't clear flag if we haven't seen it set */
+ /*
+ * Careful here --- don't clear flag if we haven't seen it set.
+ * pss_signalFlags is of type "volatile sig_atomic_t" to allow us to
+ * read it here safely, without holding the spinlock.
+ */
if (slot->pss_signalFlags[reason])
{
slot->pss_signalFlags[reason] = false;
@@ -678,3 +712,69 @@ procsignal_sigusr1_handler(SIGNAL_ARGS)
SetLatch(MyLatch);
}
+
+/*
+ * Send a query cancellation signal to backend.
+ *
+ * Note: This is called from a backend process before authentication. We
+ * cannot take LWLocks yet, but that's OK; we rely on atomic reads of the
+ * fields in the ProcSignal slots.
+ */
+void
+SendCancelRequest(int backendPID, int32 cancelAuthCode)
+{
+ Assert(backendPID != 0);
+
+ /*
+ * See if we have a matching backend. Reading the pss_pid and
+ * pss_cancel_key fields is racy, a backend might die and remove itself
+ * from the array at any time. The probability of the cancellation key
+ * matching wrong process is miniscule, however, so we can live with that.
+ * PIDs are reused too, so sending the signal based on PID is inherently
+ * racy anyway, although OS's avoid reusing PIDs too soon.
+ */
+ for (int i = 0; i < NumProcSignalSlots; i++)
+ {
+ ProcSignalSlot *slot = &ProcSignal->psh_slot[i];
+ bool match;
+
+ if (pg_atomic_read_u32(&slot->pss_pid) != backendPID)
+ continue;
+
+ /* Acquire the spinlock and re-check */
+ SpinLockAcquire(&slot->pss_mutex);
+ if (pg_atomic_read_u32(&slot->pss_pid) != backendPID)
+ {
+ SpinLockRelease(&slot->pss_mutex);
+ continue;
+ }
+ else
+ {
+ match = slot->pss_cancel_key_valid && slot->pss_cancel_key == cancelAuthCode;
+
+ SpinLockRelease(&slot->pss_mutex);
+
+ if (match)
+ {
+ /* Found a match; signal that backend to cancel current op */
+ ereport(DEBUG2,
+ (errmsg_internal("processing cancel request: sending SIGINT to process %d",
+ backendPID)));
+ kill(backendPID, SIGINT);
+ }
+ else
+ {
+ /* Right PID, wrong key: no way, Jose */
+ ereport(LOG,
+ (errmsg("wrong key in cancel request for process %d",
+ backendPID)));
+ }
+ return;
+ }
+ }
+
+ /* No matching backend */
+ ereport(LOG,
+ (errmsg("PID %d in cancel request did not match any process",
+ backendPID)));
+}
diff --git a/src/backend/tcop/backend_startup.c b/src/backend/tcop/backend_startup.c
index a2f94b1050..57733185f8 100644
--- a/src/backend/tcop/backend_startup.c
+++ b/src/backend/tcop/backend_startup.c
@@ -29,6 +29,7 @@
#include "replication/walsender.h"
#include "storage/fd.h"
#include "storage/ipc.h"
+#include "storage/procsignal.h"
#include "storage/proc.h"
#include "tcop/backend_startup.h"
#include "tcop/tcopprot.h"
@@ -525,6 +526,11 @@ ProcessStartupPacket(Port *port, bool ssl_done, bool gss_done)
if (proto == CANCEL_REQUEST_CODE)
{
+ /*
+ * The client has sent a cancel request packet, not a normal
+ * start-a-new-connection packet. Perform the necessary processing.
+ * Nothing is sent back to the client.
+ */
CancelRequestPacket *canc;
int backendPID;
int32 cancelAuthCode;
@@ -540,7 +546,8 @@ ProcessStartupPacket(Port *port, bool ssl_done, bool gss_done)
backendPID = (int) pg_ntoh32(canc->backendPID);
cancelAuthCode = (int32) pg_ntoh32(canc->cancelAuthCode);
- processCancelRequest(backendPID, cancelAuthCode);
+ if (backendPID != 0)
+ SendCancelRequest(backendPID, cancelAuthCode);
/* Not really an error, but we don't want to proceed further */
return STATUS_ERROR;
}
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index ea517f4b9b..f4f5e56132 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -4224,6 +4224,26 @@ PostgresMain(const char *dbname, const char *username)
/* We need to allow SIGINT, etc during the initial transaction */
sigprocmask(SIG_SETMASK, &UnBlockSig, NULL);
+ /*
+ * Generate a random cancel key, if this is a backend serving a
+ * connection. InitPostgres() will advertise it in shared memory.
+ *
+ * We probably don't need cancel keys for non-backend processes, but we'd
+ * better have something random in the field to prevent unfriendly people
+ * from sending cancels to them.
+ */
+ Assert(!MyCancelKeyValid);
+ if (whereToSendOutput == DestRemote)
+ {
+ if (!pg_strong_random(&MyCancelKey, sizeof(int32)))
+ {
+ ereport(ERROR,
+ (errcode(ERRCODE_INTERNAL_ERROR),
+ errmsg("could not generate random cancel key")));
+ }
+ MyCancelKeyValid = true;
+ }
+
/*
* General initialization.
*
@@ -4276,6 +4296,7 @@ PostgresMain(const char *dbname, const char *username)
{
StringInfoData buf;
+ Assert(MyCancelKeyValid);
pq_beginmessage(&buf, PqMsg_BackendKeyData);
pq_sendint32(&buf, (int32) MyProcPid);
pq_sendint32(&buf, (int32) MyCancelKey);
diff --git a/src/backend/utils/init/globals.c b/src/backend/utils/init/globals.c
index 927bccfbea..d8debd160e 100644
--- a/src/backend/utils/init/globals.c
+++ b/src/backend/utils/init/globals.c
@@ -48,7 +48,8 @@ pg_time_t MyStartTime;
TimestampTz MyStartTimestamp;
struct ClientSocket *MyClientSocket;
struct Port *MyProcPort;
-int32 MyCancelKey;
+bool MyCancelKeyValid = false;
+int32 MyCancelKey = 0;
int MyPMChildSlot;
/*
diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c
index 25867c8bd5..4d3fe43b08 100644
--- a/src/backend/utils/init/postinit.c
+++ b/src/backend/utils/init/postinit.c
@@ -716,7 +716,7 @@ InitPostgres(const char *in_dbname, Oid dboid,
*/
SharedInvalBackendInit(false);
- ProcSignalInit();
+ ProcSignalInit(MyCancelKeyValid, MyCancelKey);
/*
* Also set up timeout handlers needed for backend operation. We need
diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h
index 90f9b21b25..ac16233b71 100644
--- a/src/include/miscadmin.h
+++ b/src/include/miscadmin.h
@@ -191,6 +191,7 @@ extern PGDLLIMPORT pg_time_t MyStartTime;
extern PGDLLIMPORT TimestampTz MyStartTimestamp;
extern PGDLLIMPORT struct Port *MyProcPort;
extern PGDLLIMPORT struct Latch *MyLatch;
+extern PGDLLIMPORT bool MyCancelKeyValid;
extern PGDLLIMPORT int32 MyCancelKey;
extern PGDLLIMPORT int MyPMChildSlot;
diff --git a/src/include/postmaster/postmaster.h b/src/include/postmaster/postmaster.h
index d19e103937..4cc59a662e 100644
--- a/src/include/postmaster/postmaster.h
+++ b/src/include/postmaster/postmaster.h
@@ -36,10 +36,6 @@ extern PGDLLIMPORT bool remove_temp_files_after_crash;
extern PGDLLIMPORT bool send_abort_for_crash;
extern PGDLLIMPORT bool send_abort_for_kill;
-#ifdef EXEC_BACKEND
-extern struct bkend *ShmemBackendArray;
-#endif
-
#ifdef WIN32
extern PGDLLIMPORT HANDLE PostmasterHandle;
#else
@@ -69,14 +65,9 @@ extern bool PostmasterMarkPIDForWorkerNotify(int);
extern void processCancelRequest(int backendPID, int32 cancelAuthCode);
-#ifdef EXEC_BACKEND
-extern Size ShmemBackendArraySize(void);
-extern void ShmemBackendArrayAllocation(void);
-
#ifdef WIN32
extern void pgwin32_register_deadchild_callback(HANDLE procHandle, DWORD procId);
#endif
-#endif
/* defined in globals.c */
extern PGDLLIMPORT struct ClientSocket *MyClientSocket;
diff --git a/src/include/storage/procsignal.h b/src/include/storage/procsignal.h
index 7d290ea7d0..7b1fafe2c7 100644
--- a/src/include/storage/procsignal.h
+++ b/src/include/storage/procsignal.h
@@ -62,9 +62,10 @@ typedef enum
extern Size ProcSignalShmemSize(void);
extern void ProcSignalShmemInit(void);
-extern void ProcSignalInit(void);
+extern void ProcSignalInit(bool cancel_key_valid, int32 cancel_key);
extern int SendProcSignal(pid_t pid, ProcSignalReason reason,
ProcNumber procNumber);
+extern void SendCancelRequest(int backendPID, int32 cancelAuthCode);
extern uint64 EmitProcSignalBarrier(ProcSignalBarrierType type);
extern void WaitForProcSignalBarrier(uint64 generation);
--
2.39.2
