Does this signal multiplexing solve the "out of signals" problem we
have generally? I need another signal for the progress indicator too.
Or is this only useful for other users which need the same locks or
other resources?
--
Greg
On 8 Dec 2008, at 08:04, Heikki Linnakangas <[EMAIL PROTECTED]
> wrote:
I've been looking at the signal handling part of the synchronous
replication patch. It looks OK, but one thing makes me worry.
To set or clear the flag from PGPROC, to send or handle a signal, we
have to acquire ProcArrayLock. Is that safe to do in a signal
handler? And is the performance impact of that acceptable?
Another observation is that the patch introduces a new function
called SendProcSignal. Nothing wrong with that, except that there's
an existing function called ProcSendSignal, just above
SendProcSignal, so there's some potential for confusion. The old
ProcSendSignal function uses the per-backend semaphore to wake up a
backend. It's only used to wait for the cleanup lock in bufmgr.c.
I'm tempted to remove that altogether, and use the new signal
multiplexing for that too, but OTOH if it works, maybe I shouldn't
touch it.
Attached is a patch with some minor changes I've made. Mostly
cosmetic, but I did modify the sinval code so that ProcState has
PGPROC pointer instead of backend pid, so that we don't need to
search the ProcArray to find the PGPROC struct of the backend we're
signaling.
--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com
*** a/src/backend/access/transam/twophase.c --- b/src/backend/access/
transam/twophase.c *************** *** 287,292 ****
MarkAsPreparing(TransactionId xid, const char *gid, --- 287,293 ----
gxact->proc.databaseId = databaseid; gxact->proc.roleId = owner;
gxact->proc.inCommit = false; + gxact->proc.signalFlags = 0;
gxact->proc.vacuumFlags = 0; gxact->proc.lwWaiting = false; gxact-
>proc.lwExclusive = false; *** a/src/backend/commands/async.c --- b/
src/backend/commands/async.c *************** *** 915,923 ****
EnableNotifyInterrupt(void) * a frontend command. Signal handler
execution of inbound notifies * is disabled until the next
EnableNotifyInterrupt call. * ! * The SIGUSR1 signal handler also
needs to call this, so as to ! * prevent conflicts if one signal
interrupts the other. So we ! * must return the previous state of
the flag. */ bool DisableNotifyInterrupt(void) --- 915,924 ---- * a
frontend command. Signal handler execution of inbound notifies * is
disabled until the next EnableNotifyInterrupt call. * ! * This also
needs to be called when SIGUSR1 with ! * PROCSIG_CATCHUP_INTERRUPT
is received, so as to prevent conflicts ! * if one signal
interrupts the other. So we must return the previous ! * state of
the flag. */ bool DisableNotifyInterrupt(void) *************** ***
954,960 **** ProcessIncomingNotify(void) nulls[Natts_pg_listener];
bool catchup_enabled; ! /* Must prevent SIGUSR1 interrupt while I am
running */ catchup_enabled = DisableCatchupInterrupt(); if
(Trace_notify) --- 955,961 ---- nulls[Natts_pg_listener]; bool
catchup_enabled; ! /* Must prevent catchup interrupt while I am
running */ catchup_enabled = DisableCatchupInterrupt(); if
(Trace_notify) *** a/src/backend/postmaster/autovacuum.c --- b/src/
backend/postmaster/autovacuum.c *************** *** 1477,1483 ****
AutoVacWorkerMain(int argc, char *argv[]) pqsignal(SIGALRM,
handle_sig_alarm); pqsignal(SIGPIPE, SIG_IGN); ! pqsignal(SIGUSR1,
CatchupInterruptHandler); /* We don't listen for async notifies */
pqsignal(SIGUSR2, SIG_IGN); pqsignal(SIGFPE, FloatExceptionHandler);
--- 1477,1483 ---- pqsignal(SIGALRM, handle_sig_alarm);
pqsignal(SIGPIPE, SIG_IGN); ! pqsignal(SIGUSR1,
proc_sigusr1_handler); /* We don't listen for async notifies */
pqsignal(SIGUSR2, SIG_IGN); pqsignal(SIGFPE, FloatExceptionHandler);
*** a/src/backend/storage/ipc/sinval.c --- b/src/backend/storage/ipc/
sinval.c *************** *** 27,33 **** * need a way to give an idle
backend a swift kick in the rear and make * it catch up before the
sinval queue overflows and forces it to go * through a cache reset
exercise. This is done by sending SIGUSR1 ! * to any backend that
gets too far behind. * * State for catchup events consists of two
flags: one saying whether * the signal handler is currently allowed
to call ProcessCatchupEvent --- 27,34 ---- * need a way to give an
idle backend a swift kick in the rear and make * it catch up before
the sinval queue overflows and forces it to go * through a cache
reset exercise. This is done by sending SIGUSR1 ! * with
PROCSIG_CATCHUP_INTERRUPT to any backend that gets too far ! *
behind. * * State for catchup events consists of two flags: one
saying whether * the signal handler is currently allowed to call
ProcessCatchupEvent *************** *** 144,152 ****
ReceiveSharedInvalidMessages( /* ! * CatchupInterruptHandler * ! *
This is the signal handler for SIGUSR1. * * If we are idle
(catchupInterruptEnabled is set), we can safely * invoke
ProcessCatchupEvent directly. Otherwise, just set a flag --- 145,154
---- /* ! * HandleCatchupInterrupt * ! * This is called when SIGUSR1
with PROCSIG_CATCHUP_INTERRUPT is ! * received. * * If we are idle
(catchupInterruptEnabled is set), we can safely * invoke
ProcessCatchupEvent directly. Otherwise, just set a flag
*************** *** 156,168 **** ReceiveSharedInvalidMessages( *
since there's no longer any reason to do anything.) */ void !
CatchupInterruptHandler(SIGNAL_ARGS) { - int save_errno = errno; - /
* ! * Note: this is a SIGNAL HANDLER. You must be very wary what
you do ! * here. */ /* Don't joggle the elbow of proc_exit */ ---
158,168 ---- * since there's no longer any reason to do anything.)
*/ void ! HandleCatchupInterrupt(void) { /* ! * Note: this is called
by a SIGNAL HANDLER. ! * You must be very wary what you do here.
*/ /* Don't joggle the elbow of proc_exit */ *************** ***
216,223 **** CatchupInterruptHandler(SIGNAL_ARGS) */
catchupInterruptOccurred = 1; } - - errno = save_errno; } /* ---
216,221 ---- *************** *** 289,295 ****
DisableCatchupInterrupt(void) /* * ProcessCatchupEvent * ! * Respond
to a catchup event (SIGUSR1) from another backend. * * This is
called either directly from the SIGUSR1 signal handler, * or the
next time control reaches the outer idle loop (assuming --- 287,294
---- /* * ProcessCatchupEvent * ! * Respond to a catchup event
(SIGUSR1 with PROCSIG_CATCHUP_INTERRUPT) ! * from another backend. *
* This is called either directly from the SIGUSR1 signal handler, *
or the next time control reaches the outer idle loop (assuming *** a/
src/backend/storage/ipc/sinvaladt.c --- b/src/backend/storage/ipc/
sinvaladt.c *************** *** 21,26 **** --- 21,27 ---- #include
"storage/backendid.h" #include "storage/ipc.h" #include "storage/
proc.h" + #include "storage/procarray.h" #include "storage/shmem.h"
#include "storage/sinvaladt.h" #include "storage/spin.h"
*************** *** 136,144 **** /* Per-backend state in shared
invalidation structure */ typedef struct ProcState { ! /* procPid is
zero in an inactive ProcState array entry. */ ! pid_t procPid; /*
PID of backend, for signaling */ ! /* nextMsgNum is meaningless if
procPid == 0 or resetState is true. */ int nextMsgNum; /* next
message number to read */ bool resetState; /* backend needs to
reset its state */ bool signaled; /* backend has been sent catchup
signal */ --- 137,145 ---- /* Per-backend state in shared
invalidation structure */ typedef struct ProcState { ! /* proc is
NULL in an inactive ProcState array entry. */ ! PGPROC *proc; /*
PGPROC entry of backend, for signaling */ ! /* nextMsgNum is
meaningless if proc == NULL or resetState is true. */ int
nextMsgNum; /* next message number to read */ bool resetState; /*
backend needs to reset its state */ bool signaled; /* backend has
been sent catchup signal */ *************** *** 235,241 ****
CreateSharedInvalidationState(void) /* Mark all backends
inactive, and initialize nextLXID */ for (i = 0; i < shmInvalBuffer-
>maxBackends; i++) { ! shmInvalBuffer->procState[i].procPid =
0; /* inactive */ shmInvalBuffer->procState[i].nextMsgNum = 0; /*
meaningless */ shmInvalBuffer->procState[i].resetState = false;
shmInvalBuffer->procState[i].signaled = false; --- 236,242 ---- /*
Mark all backends inactive, and initialize nextLXID */ for (i = 0; i
< shmInvalBuffer->maxBackends; i++) { ! shmInvalBuffer-
>procState[i].proc = NULL; /* inactive */ shmInvalBuffer-
>procState[i].nextMsgNum = 0; /* meaningless */ shmInvalBuffer-
>procState[i].resetState = false; shmInvalBuffer-
>procState[i].signaled = false; *************** *** 266,272 ****
SharedInvalBackendInit(void) /* Look for a free entry in the
procState array */ for (index = 0; index < segP->lastBackend; index+
+) { ! if (segP->procState[index].procPid == 0) /* inactive slot?
*/ { stateP = &segP->procState[index]; break; --- 267,273 ---- /*
Look for a free entry in the procState array */ for (index = 0;
index < segP->lastBackend; index++) { ! if (segP-
>procState[index].proc == NULL) /* inactive slot? */ { stateP =
&segP->procState[index]; break; *************** *** 278,284 ****
SharedInvalBackendInit(void) if (segP->lastBackend < segP-
>maxBackends) { stateP = &segP->procState[segP->lastBackend]; !
Assert(stateP->procPid == 0); segP->lastBackend++; } else ---
279,285 ---- if (segP->lastBackend < segP->maxBackends) { stateP =
&segP->procState[segP->lastBackend]; ! Assert(stateP->proc == NULL);
segP->lastBackend++; } else *************** *** 303,309 ****
SharedInvalBackendInit(void) nextLocalTransactionId = stateP-
>nextLXID; /* mark myself active, with all extant messages already
read */ ! stateP->procPid = MyProcPid; stateP->nextMsgNum = segP-
>maxMsgNum; stateP->resetState = false; stateP->signaled = false;
--- 304,310 ---- nextLocalTransactionId = stateP->nextLXID; /* mark
myself active, with all extant messages already read */ ! stateP-
>proc = MyProc; stateP->nextMsgNum = segP->maxMsgNum; stateP-
>resetState = false; stateP->signaled = false; *************** ***
341,347 **** CleanupInvalidationState(int status, Datum arg) stateP-
>nextLXID = nextLocalTransactionId; /* Mark myself inactive */ !
stateP->procPid = 0; stateP->nextMsgNum = 0; stateP->resetState =
false; stateP->signaled = false; --- 342,348 ---- stateP->nextLXID =
nextLocalTransactionId; /* Mark myself inactive */ ! stateP-
>proc = NULL; stateP->nextMsgNum = 0; stateP->resetState = false;
stateP->signaled = false; *************** *** 349,355 ****
CleanupInvalidationState(int status, Datum arg) /* Recompute index
of last active backend */ for (i = segP->lastBackend; i > 0; i--)
{ ! if (segP->procState[i - 1].procPid != 0) break; } segP-
>lastBackend = i; --- 350,356 ---- /* Recompute index of last active
backend */ for (i = segP->lastBackend; i > 0; i--) { ! if (segP-
>procState[i - 1].proc != NULL) break; } segP->lastBackend = i;
*************** *** 374,380 **** BackendIdIsActive(int backendID)
{ ProcState *stateP = &segP->procState[backendID - 1]; ! result =
(stateP->procPid != 0); } else result = false; --- 375,381 ----
{ ProcState *stateP = &segP->procState[backendID - 1]; ! result =
(stateP->proc != NULL); } else result = false; *************** ***
590,596 **** SICleanupQueue(bool callerHasWriteLock, int minFree)
int n = stateP->nextMsgNum; /* Ignore if inactive or already in
reset state */ ! if (stateP->procPid == 0 || stateP->resetState)
continue; /* --- 591,597 ---- int n = stateP->nextMsgNum; /* Ignore
if inactive or already in reset state */ ! if (stateP->proc == NULL
|| stateP->resetState) continue; /* *************** *** 644,661 ****
SICleanupQueue(bool callerHasWriteLock, int minFree) segP-
>nextThreshold = (numMsgs / CLEANUP_QUANTUM + 1) * CLEANUP_QUANTUM; /
* ! * Lastly, signal anyone who needs a catchup interrupt. Since
kill() ! * might not be fast, we don't want to hold locks while
executing it. */ if (needSig) { ! pid_t his_pid = needSig->procPid;
needSig->signaled = true; LWLockRelease(SInvalReadLock);
LWLockRelease(SInvalWriteLock); ! elog(DEBUG4, "sending sinval
catchup signal to PID %d", (int) his_pid); ! kill(his_pid, SIGUSR1);
if (callerHasWriteLock) LWLockAcquire(SInvalWriteLock,
LW_EXCLUSIVE); } --- 645,664 ---- segP->nextThreshold = (numMsgs /
CLEANUP_QUANTUM + 1) * CLEANUP_QUANTUM; /* ! * Lastly, signal anyone
who needs a catchup interrupt. Since ! * SendProcSignal() might not
be fast, we don't want to hold locks while ! * executing it. */ if
(needSig) { ! PGPROC *his_proc = needSig->proc; needSig->signaled =
true; LWLockRelease(SInvalReadLock);
LWLockRelease(SInvalWriteLock); ! elog(DEBUG4, "sending sinval
catchup signal to PID %d", ! (int) his_proc->pid); !
SendProcSignal(his_proc, PROCSIG_CATCHUP_INTERRUPT); if
(callerHasWriteLock) LWLockAcquire(SInvalWriteLock, LW_EXCLUSIVE); }
*** a/src/backend/storage/lmgr/proc.c --- b/src/backend/storage/lmgr/
proc.c *************** *** 289,294 **** InitProcess(void) ---
289,295 ---- MyProc->databaseId = InvalidOid; MyProc->roleId =
InvalidOid; MyProc->inCommit = false; + MyProc->signalFlags = 0;
MyProc->vacuumFlags = 0; if (IsAutoVacuumWorkerProcess()) MyProc-
>vacuumFlags |= PROC_IS_AUTOVACUUM; *************** *** 428,433 ****
InitAuxiliaryProcess(void) --- 429,435 ---- MyProc->databaseId =
InvalidOid; MyProc->roleId = InvalidOid; MyProc->inCommit = false; +
MyProc->signalFlags = 0; /* we don't set the "is autovacuum" flag in
the launcher */ MyProc->vacuumFlags = 0; MyProc->lwWaiting = false;
*************** *** 1277,1282 **** ProcSendSignal(int pid) ---
1279,1330 ---- PGSemaphoreUnlock(&proc->sem); } + /* + *
SendProcSignal - send the signal with the reason to the process + *
(such as backend, autovacuum worker and auxiliary process) + *
identified by proc. + */ + void + SendProcSignal(PGPROC *proc, uint8
reason) + { + int pid; + + if (proc == NULL) + return; + +
LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE); + pid = proc->pid; + if
(pid != 0) + proc->signalFlags |= reason; +
LWLockRelease(ProcArrayLock); + + /* Send SIGUSR1 to the process */
+ kill(pid, SIGUSR1); + } + + /* + * CheckProcSignal - check to see
if the particular reason has been + * signaled, and clear the signal
flag. Should be called after + * receiving SIGUSR1. + */ + bool +
CheckProcSignal(uint8 reason) + { + LWLockAcquire(ProcArrayLock,
LW_EXCLUSIVE); + + /* Careful here --- don't clear flag if we
haven't seen it set */ + if (MyProc->signalFlags & reason) + { +
MyProc->signalFlags &= ~reason; + LWLockRelease(ProcArrayLock); +
return true; + } + + LWLockRelease(ProcArrayLock); + + return
false; + } + /
***
***
***
********************************************************************
* SIGALRM interrupt support *** a/src/backend/tcop/postgres.c --- b/
src/backend/tcop/postgres.c *************** *** 2437,2442 ****
drop_unnamed_stmt(void) --- 2437,2464 ---- */ /* + *
proc_sigusr1_handler - handle SIGUSR1 signal. + * + * SIGUSR1 is
multiplexed to handle multiple different events. The signalFlags + *
bitmask in PGPROC indicates which events have been signaled. + */ +
void + proc_sigusr1_handler(SIGNAL_ARGS) + { + int save_errno =
errno; + + if (CheckProcSignal(PROCSIG_CATCHUP_INTERRUPT)) + { + /*
+ * Catchup interrupt has been sent. + */ +
HandleCatchupInterrupt(); + } + + errno = save_errno; + } + + /* *
quickdie() occurs when signalled SIGQUIT by the postmaster. * * Some
backend has bought the farm, *************** *** 3180,3186 ****
PostgresMain(int argc, char *argv[], const char *username) * of
output during who-knows-what operation... */ pqsignal(SIGPIPE,
SIG_IGN); ! pqsignal(SIGUSR1, CatchupInterruptHandler);
pqsignal(SIGUSR2, NotifyInterruptHandler); pqsignal(SIGFPE,
FloatExceptionHandler); --- 3202,3208 ---- * of output during who-
knows-what operation... */ pqsignal(SIGPIPE, SIG_IGN); !
pqsignal(SIGUSR1, proc_sigusr1_handler); pqsignal(SIGUSR2,
NotifyInterruptHandler); pqsignal(SIGFPE, FloatExceptionHandler);
*** a/src/include/storage/proc.h --- b/src/include/storage/proc.h
*************** *** 38,43 **** struct XidCache --- 38,46 ----
TransactionId xids[PGPROC_MAX_CACHED_SUBXIDS]; }; + /* Signals for
PGPROC->signalFlags */ + #define PROCSIG_CATCHUP_INTERRUPT 0x01 /*
catchup interrupt */ + /* Flags for PGPROC->vacuumFlags */ #define
PROC_IS_AUTOVACUUM 0x01 /* is it an autovac worker? */ #define
PROC_IN_VACUUM 0x02 /* currently running lazy vacuum */
*************** *** 91,96 **** struct PGPROC --- 94,100 ---- bool
inCommit; /* true if within commit critical section */ + uint8
signalFlags; /* bitmask of signals raised, see above */ uint8
vacuumFlags; /* vacuum-related flags, see above */ /* Info about
LWLock the process is currently waiting for, if any. */
*************** *** 171,176 **** extern void LockWaitCancel(void);
--- 175,183 ---- extern void ProcWaitForSignal(void); extern void
ProcSendSignal(int pid); + extern void SendProcSignal(PGPROC *proc,
uint8 reason); + extern bool CheckProcSignal(uint8 reason); + extern
bool enable_sig_alarm(int delayms, bool is_statement_timeout);
extern bool disable_sig_alarm(bool is_statement_timeout); extern
void handle_sig_alarm(SIGNAL_ARGS); *** a/src/include/storage/
sinval.h --- b/src/include/storage/sinval.h *************** ***
90,96 **** extern void ReceiveSharedInvalidMessages( void
(*resetFunction) (void)); /* signal handler for catchup events
(SIGUSR1) */ ! extern void CatchupInterruptHandler(SIGNAL_ARGS); /*
* enable/disable processing of catchup events directly from signal
handler. --- 90,96 ---- void (*resetFunction) (void)); /* signal
handler for catchup events (SIGUSR1) */ ! extern void
HandleCatchupInterrupt(void); /* * enable/disable processing of
catchup events directly from signal handler. *** a/src/include/tcop/
tcopprot.h --- b/src/include/tcop/tcopprot.h *************** ***
56,61 **** extern List *pg_plan_queries(List *querytrees, int
cursorOptions, --- 56,62 ---- extern bool assign_max_stack_depth(int
newval, bool doit, GucSource source); + extern void
proc_sigusr1_handler(SIGNAL_ARGS); extern void die(SIGNAL_ARGS);
extern void quickdie(SIGNAL_ARGS); extern void authdie(SIGNAL_ARGS);
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
