Review: support for multiplexing SIGUSR1
Hi,
I'm reviewing this patch:
http://archives.postgresql.org/message-id/3f0b79eb0907022341m1d36a841x19c3e2a5a6906b5b@mail.gmail.com
This one applies almost cleanly, except for a minor hunk in elog.c and
postinit.c
Compiles and pass regression tests (i tried both steps in a debian
lenny amd turion x2 64bits and in a windows xp sp2)
about the patch itself:
Tom objects to a previous patch for this here:
http://archives.postgresql.org/message-id/14969.1228835521@sss.pgh.pa.us
This new patch doesn't use PGPROC struct anymore, instead it uses a
ProcSignalSlot struct defined as:
typedef struct {
pid_t pss_pid;
sig_atomic_t pss_signalFlags[NUM_PROCSIGNALS];
} ProcSignalSlot;
which, AFAIU, seems to be in sync with Tom's advice here:
http://archives.postgresql.org/pgsql-hackers/2008-12/msg00556.php
something that make me nervous is this:
/*
* 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.
*/
can we signal a wrong process and still "be fine"?
besides, seems like SendProcSignal is still attached to SIGUSR1 only,
is this fine?
the rest of the patch (or better ways of testing it) is beyond my knowledge...
i think a reviewer should take a look on it, specially Tom because he
rejected the other one...
--
Atentamente,
Jaime Casanova
Soporte y capacitación de PostgreSQL
Asesoría y desarrollo de sistemas
Guayaquil - Ecuador
Cel. +59387171157
On Thu, Jul 16, 2009 at 2:57 AM, Jaime
Casanova<jcasanov@systemguards.com.ec> wrote:
Hi,
I'm reviewing this patch:
http://archives.postgresql.org/message-id/3f0b79eb0907022341m1d36a841x19c3e2a5a6906b5b@mail.gmail.com
Another thing that took my attention, i don't think this is safe (it
assumes only one auxiliary process of any type, don't know if we have
various of the same kind but...):
+ /*
+ * Assign backend ID to auxiliary processes like backends, in order to
+ * allow multiplexing signal to auxiliary processes. Since backends use
+ * ID in the range from 1 to MaxBackends (inclusive), we assign
+ * auxiliary processes with MaxBackends + AuxProcType + 1 as
an unique ID.
+ */
+ MyBackendId = MaxBackends + auxType + 1;
+ MyProc->backendId = MyBackendId;--
Atentamente,
Jaime Casanova
Soporte y capacitación de PostgreSQL
Asesoría y desarrollo de sistemas
Guayaquil - Ecuador
Cel. +59387171157
Hi Jaime,
On Fri, Jul 17, 2009 at 6:31 AM, Jaime
Casanova<jcasanov@systemguards.com.ec> wrote:
I'm reviewing this patch:
http://archives.postgresql.org/message-id/3f0b79eb0907022341m1d36a841x19c3e2a5a6906b5b@mail.gmail.com
Thanks for reviewing the patch!
something that make me nervous is this:
/*
* 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.
*/can we signal a wrong process and still "be fine"?
Umm... the old flag might be set to a new process wongly as follows.
1. The target process exits right after SendProcSignal passed that test.
2. A new process is assigned to the same ProcSignalSlot, and reset it.
3. SendProcSignal sets the old flag to the slot.
I think that locking is required here to get around this problem. How about
introducing a new slock variable "slock_t pss_lck" into ProcSignalSlot strust,
which protects pss_pid and pss_signalFlags? SendProcSignal and
ProcSignalInit should use the slock.
besides, seems like SendProcSignal is still attached to SIGUSR1 only,
is this fine?
Yes, I think that multiplexing of one signal is enough. Why do you think
that SendProcSignal should be attached to also the other signals?
Another thing that took my attention, i don't think this is safe (it
assumes only one auxiliary process of any type, don't know if we have
various of the same kind but...):+ /* + * Assign backend ID to auxiliary processes like backends, in order to + * allow multiplexing signal to auxiliary processes. Since backends use + * ID in the range from 1 to MaxBackends (inclusive), we assign + * auxiliary processes with MaxBackends + AuxProcType + 1 as an unique ID. + */ + MyBackendId = MaxBackends + auxType + 1; + MyProc->backendId = MyBackendId;
That assumption is OK for now, but might be unacceptable in the near future.
So, I'll use the index of AuxiliaryProcs instead of auxType, which is assigned
in InitAuxiliaryProcess. Is this OK?
Regards,
--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center
Hi,
On Fri, Jul 17, 2009 at 5:41 PM, Fujii Masao<masao.fujii@gmail.com> wrote:
I'm reviewing this patch:
http://archives.postgresql.org/message-id/3f0b79eb0907022341m1d36a841x19c3e2a5a6906b5b@mail.gmail.com
I updated the patch to solve two problems which you pointed.
Here is the changes:
* Prevented the obsolete flag to being set to a new process, by using
newly-introduced spinlock.
* Used the index of AuxiliaryProcs instead of auxType, to assign
backend ID to an auxiliary process.
Regards,
--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center
Attachments:
signal_multiplexer_0717.patchapplication/octet-stream; name=signal_multiplexer_0717.patchDownload
diff -rcN base/src/backend/bootstrap/bootstrap.c new/src/backend/bootstrap/bootstrap.c
*** base/src/backend/bootstrap/bootstrap.c 2009-07-17 21:32:03.000000000 +0900
--- new/src/backend/bootstrap/bootstrap.c 2009-07-17 21:48:33.000000000 +0900
***************
*** 35,40 ****
--- 35,41 ----
#include "storage/bufmgr.h"
#include "storage/ipc.h"
#include "storage/proc.h"
+ #include "storage/procsignal.h"
#include "tcop/tcopprot.h"
#include "utils/builtins.h"
#include "utils/fmgroids.h"
***************
*** 388,393 ****
--- 389,397 ----
InitAuxiliaryProcess();
#endif
+ /* register a process to the procsignal array */
+ ProcSignalInit();
+
/* finish setting up bufmgr.c */
InitBufferPoolBackend();
diff -rcN base/src/backend/postmaster/autovacuum.c new/src/backend/postmaster/autovacuum.c
*** base/src/backend/postmaster/autovacuum.c 2009-07-17 21:32:03.000000000 +0900
--- new/src/backend/postmaster/autovacuum.c 2009-07-17 21:32:08.000000000 +0900
***************
*** 91,96 ****
--- 91,97 ----
#include "storage/pmsignal.h"
#include "storage/proc.h"
#include "storage/procarray.h"
+ #include "storage/procsignal.h"
#include "storage/sinvaladt.h"
#include "tcop/tcopprot.h"
#include "utils/dynahash.h"
***************
*** 1501,1507 ****
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);
--- 1502,1508 ----
pqsignal(SIGALRM, handle_sig_alarm);
pqsignal(SIGPIPE, SIG_IGN);
! pqsignal(SIGUSR1, procsignal_sigusr1_handler);
/* We don't listen for async notifies */
pqsignal(SIGUSR2, SIG_IGN);
pqsignal(SIGFPE, FloatExceptionHandler);
diff -rcN base/src/backend/storage/ipc/Makefile new/src/backend/storage/ipc/Makefile
*** base/src/backend/storage/ipc/Makefile 2009-07-17 21:32:03.000000000 +0900
--- new/src/backend/storage/ipc/Makefile 2009-07-17 21:32:08.000000000 +0900
***************
*** 15,21 ****
endif
endif
! OBJS = ipc.o ipci.o pmsignal.o procarray.o shmem.o shmqueue.o \
sinval.o sinvaladt.o
include $(top_srcdir)/src/backend/common.mk
--- 15,21 ----
endif
endif
! OBJS = ipc.o ipci.o pmsignal.o procarray.o procsignal.o shmem.o shmqueue.o \
sinval.o sinvaladt.o
include $(top_srcdir)/src/backend/common.mk
diff -rcN base/src/backend/storage/ipc/ipci.c new/src/backend/storage/ipc/ipci.c
*** base/src/backend/storage/ipc/ipci.c 2009-07-17 21:32:03.000000000 +0900
--- new/src/backend/storage/ipc/ipci.c 2009-07-17 21:32:08.000000000 +0900
***************
*** 30,35 ****
--- 30,36 ----
#include "storage/pg_shmem.h"
#include "storage/pmsignal.h"
#include "storage/procarray.h"
+ #include "storage/procsignal.h"
#include "storage/sinvaladt.h"
#include "storage/spin.h"
***************
*** 114,119 ****
--- 115,121 ----
size = add_size(size, PMSignalShmemSize());
size = add_size(size, BgWriterShmemSize());
size = add_size(size, AutoVacuumShmemSize());
+ size = add_size(size, ProcSignalShmemSize());
size = add_size(size, BTreeShmemSize());
size = add_size(size, SyncScanShmemSize());
#ifdef EXEC_BACKEND
***************
*** 210,215 ****
--- 212,218 ----
PMSignalShmemInit();
BgWriterShmemInit();
AutoVacuumShmemInit();
+ ProcSignalShmemInit();
/*
* Set up other modules that need some shared memory space
diff -rcN base/src/backend/storage/ipc/procsignal.c new/src/backend/storage/ipc/procsignal.c
*** base/src/backend/storage/ipc/procsignal.c 1970-01-01 09:00:00.000000000 +0900
--- new/src/backend/storage/ipc/procsignal.c 2009-07-17 22:28:08.000000000 +0900
***************
*** 0 ****
--- 1,233 ----
+ /*-------------------------------------------------------------------------
+ *
+ * procsignal.c
+ * routines for signaling processes
+ *
+ *
+ * Portions Copyright (c) 1996-2008, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
+ *
+ * IDENTIFICATION
+ * $PostgreSQL$
+ *
+ *-------------------------------------------------------------------------
+ */
+ #include "postgres.h"
+
+ #include <signal.h>
+ #include <unistd.h>
+
+ #include "bootstrap/bootstrap.h"
+ #include "miscadmin.h"
+ #include "storage/backendid.h"
+ #include "storage/ipc.h"
+ #include "storage/proc.h"
+ #include "storage/procsignal.h"
+ #include "storage/shmem.h"
+ #include "storage/sinval.h"
+ #include "storage/spin.h"
+
+ /*
+ * SIGUSR1 signal is multiplexed for multiple events. The specific reason
+ * is communicated via flags in shared memory. We keep a boolean flag for
+ * each possible "reason", so that different reasons can be signaled by
+ * different backends at the same time. (However, if the same reason is
+ * signaled more than once simultaneously, the process will observe it only
+ * once.)
+ *
+ * Each process that wants to receive signals registers its process ID
+ * in the ProcSignalSlots array. The array is indexed by backend ID to make
+ * slot allocation simple, and to avoid having to search the array when you
+ * know the backend ID of the process you're signaling.
+ *
+ * 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.
+ */
+ typedef struct {
+ pid_t pss_pid;
+ sig_atomic_t pss_signalFlags[NUM_PROCSIGNALS];
+
+ /* protects shared variables shown above */
+ slock_t pss_lck;
+ } ProcSignalSlot;
+
+ static ProcSignalSlot *ProcSignalSlots;
+
+ static bool CheckProcSignal(ProcSignalReason reason);
+ static void CleanupProcSignalState(int status, Datum arg);
+
+ /*
+ * ProcSignalShmemInit - initialize during shared-memory creation
+ */
+ Size
+ ProcSignalShmemSize(void)
+ {
+ return (MaxBackends + NUM_AUXILIARY_PROCS) * sizeof(ProcSignalSlot);
+ }
+
+
+ /*
+ * ProcSignalShmemInit - initialize during shared-memory creation
+ */
+ void
+ ProcSignalShmemInit(void)
+ {
+ bool found;
+ int i;
+
+ ProcSignalSlots = (ProcSignalSlot *)
+ ShmemInitStruct("ProcSignalSlots", ProcSignalShmemSize(),
+ &found);
+
+ if (found)
+ return;
+
+ MemSet(ProcSignalSlots, 0, ProcSignalShmemSize());
+ for (i = 0; i < MaxBackends + NUM_AUXILIARY_PROCS; i++)
+ {
+ volatile ProcSignalSlot *slot;
+
+ slot = &ProcSignalSlots[i];
+ SpinLockInit(&slot->pss_lck);
+ }
+ }
+
+ /*
+ * ProcSignalInit - register a process to the procsignal array
+ */
+ void
+ ProcSignalInit(void)
+ {
+ volatile ProcSignalSlot *slot;
+
+ Assert(MyBackendId >= 1 &&
+ MyBackendId <= MaxBackends + NUM_AUXILIARY_PROCS);
+
+ slot = &ProcSignalSlots[MyBackendId - 1];
+ SpinLockAcquire(&slot->pss_lck);
+ MemSet(slot->pss_signalFlags, 0, NUM_PROCSIGNALS * sizeof(sig_atomic_t));
+ SpinLockRelease(&slot->pss_lck);
+ slot->pss_pid = MyProcPid;
+
+ on_shmem_exit(CleanupProcSignalState, Int32GetDatum(MyBackendId));
+ }
+
+
+ /*
+ * CleanupProcSignalState
+ * Remove current process from ProcSignalSlots
+ *
+ * This function is called via on_shmem_exit() during backend shutdown.
+ *
+ * arg is really of type "BackendId".
+ */
+ static void
+ CleanupProcSignalState(int status, Datum arg)
+ {
+ BackendId backendId = (BackendId) DatumGetInt32(arg);
+ volatile ProcSignalSlot *slot;
+
+ slot = &ProcSignalSlots[backendId - 1];
+
+ /* sanity check */
+ if (slot->pss_pid != MyProcPid)
+ {
+ /*
+ * don't ERROR here. We're exiting anyway, and don't want to
+ * get into infinite loop trying to exit
+ */
+ elog(WARNING, "current process not in ProcSignalSlots array");
+ return;
+ }
+ slot->pss_pid = 0;
+ }
+
+ /*
+ * SendProcSignal - signal a process with a reason code
+ *
+ * Providing backendId is optional, but it will speed up the operation.
+ *
+ * Not to be confused with ProcSendSignal
+ */
+ void
+ SendProcSignal(pid_t pid, ProcSignalReason reason, BackendId backendId)
+ {
+ volatile ProcSignalSlot *slot;
+
+ if (backendId != InvalidBackendId)
+ {
+ slot = &ProcSignalSlots[backendId - 1];
+
+ SpinLockAcquire(&slot->pss_lck);
+ if (slot->pss_pid == pid)
+ {
+ /* Atomically set the proper flag */
+ slot->pss_signalFlags[reason] = true;
+ SpinLockRelease(&slot->pss_lck);
+
+ /* Send signal */
+ kill(pid, SIGUSR1);
+ }
+ else
+ SpinLockRelease(&slot->pss_lck);
+ }
+ else
+ {
+ /* same as above, but linear search the array using pid */
+ int i;
+ for (i = 0; i < MaxBackends + NUM_AUXILIARY_PROCS; i++)
+ {
+ slot = &ProcSignalSlots[i];
+
+ SpinLockAcquire(&slot->pss_lck);
+ if (slot->pss_pid == pid)
+ {
+ /* Atomically set the proper flag */
+ slot->pss_signalFlags[reason] = true;
+ SpinLockRelease(&slot->pss_lck);
+
+ /* Send signal */
+ kill(pid, SIGUSR1);
+
+ break;
+ }
+ SpinLockRelease(&slot->pss_lck);
+ }
+ }
+ }
+
+ /*
+ * CheckProcSignal - check to see if a particular reason has been
+ * signaled, and clear the signal flag. Should be called after receiving
+ * SIGUSR1.
+ */
+ static bool
+ CheckProcSignal(ProcSignalReason reason)
+ {
+ volatile ProcSignalSlot *slot;
+
+ slot = &ProcSignalSlots[MyBackendId - 1];
+
+ /* Careful here --- don't clear flag if we haven't seen it set */
+ if (slot->pss_signalFlags[reason])
+ {
+ slot->pss_signalFlags[reason] = false;
+ return true;
+ }
+ return false;
+ }
+
+ /*
+ * procsignal_sigusr1_handler - handle SIGUSR1 signal.
+ */
+ void
+ procsignal_sigusr1_handler(SIGNAL_ARGS)
+ {
+ int save_errno = errno;
+
+ if (CheckProcSignal(PROCSIG_CATCHUP_INTERRUPT))
+ HandleCatchupInterrupt();
+
+ errno = save_errno;
+ }
diff -rcN base/src/backend/storage/ipc/sinval.c new/src/backend/storage/ipc/sinval.c
*** base/src/backend/storage/ipc/sinval.c 2009-07-17 21:32:03.000000000 +0900
--- new/src/backend/storage/ipc/sinval.c 2009-07-17 21:32:08.000000000 +0900
***************
*** 26,33 ****
* Because backends sitting idle will not be reading sinval events, we
* 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
--- 26,33 ----
* Because backends sitting idle will not be reading sinval events, we
* 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
! * PROCSIG_CATHCUP_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
***************
*** 145,153 ****
/*
! * 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,153 ----
/*
! * HandleCatchupInterrupt
*
! * This is called when PROCSIG_CATCHUP_INTERRUPT signal is received.
*
* If we are idle (catchupInterruptEnabled is set), we can safely
* invoke ProcessCatchupEvent directly. Otherwise, just set a flag
***************
*** 157,169 ****
* 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 */
--- 157,167 ----
* since there's no longer any reason to do anything.)
*/
void
! HandleCatchupInterrupt(void)
{
/*
! * Note: this called by a SIGNAL HANDLER. You must be very wary what
! * you do here.
*/
/* Don't joggle the elbow of proc_exit */
***************
*** 217,224 ****
*/
catchupInterruptOccurred = 1;
}
-
- errno = save_errno;
}
/*
--- 215,220 ----
***************
*** 290,296 ****
/*
* 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
--- 286,293 ----
/*
* ProcessCatchupEvent
*
! * Respond to a catchup event (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
diff -rcN base/src/backend/storage/ipc/sinvaladt.c new/src/backend/storage/ipc/sinvaladt.c
*** base/src/backend/storage/ipc/sinvaladt.c 2009-07-17 21:32:03.000000000 +0900
--- new/src/backend/storage/ipc/sinvaladt.c 2009-07-17 21:32:08.000000000 +0900
***************
*** 21,26 ****
--- 21,27 ----
#include "storage/backendid.h"
#include "storage/ipc.h"
#include "storage/proc.h"
+ #include "storage/procsignal.h"
#include "storage/shmem.h"
#include "storage/sinvaladt.h"
#include "storage/spin.h"
***************
*** 138,143 ****
--- 139,145 ----
{
/* procPid is zero in an inactive ProcState array entry. */
pid_t procPid; /* PID of backend, for signaling */
+ BackendId backendId; /* backend ID, 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 */
***************
*** 236,241 ****
--- 238,244 ----
for (i = 0; i < shmInvalBuffer->maxBackends; i++)
{
shmInvalBuffer->procState[i].procPid = 0; /* inactive */
+ shmInvalBuffer->procState[i].backendId = InvalidBackendId;
shmInvalBuffer->procState[i].nextMsgNum = 0; /* meaningless */
shmInvalBuffer->procState[i].resetState = false;
shmInvalBuffer->procState[i].signaled = false;
***************
*** 304,309 ****
--- 307,313 ----
/* mark myself active, with all extant messages already read */
stateP->procPid = MyProcPid;
+ stateP->backendId = MyBackendId;
stateP->nextMsgNum = segP->maxMsgNum;
stateP->resetState = false;
stateP->signaled = false;
***************
*** 342,347 ****
--- 346,352 ----
/* Mark myself inactive */
stateP->procPid = 0;
+ stateP->backendId = InvalidBackendId;
stateP->nextMsgNum = 0;
stateP->resetState = false;
stateP->signaled = false;
***************
*** 644,661 ****
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);
}
--- 649,668 ----
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)
{
! pid_t his_pid = needSig->procPid;
! BackendId his_backendId = needSig->backendId;
needSig->signaled = true;
LWLockRelease(SInvalReadLock);
LWLockRelease(SInvalWriteLock);
elog(DEBUG4, "sending sinval catchup signal to PID %d", (int) his_pid);
! SendProcSignal(his_pid, PROCSIG_CATCHUP_INTERRUPT, his_backendId);
if (callerHasWriteLock)
LWLockAcquire(SInvalWriteLock, LW_EXCLUSIVE);
}
diff -rcN base/src/backend/storage/lmgr/proc.c new/src/backend/storage/lmgr/proc.c
*** base/src/backend/storage/lmgr/proc.c 2009-07-17 21:32:03.000000000 +0900
--- new/src/backend/storage/lmgr/proc.c 2009-07-17 21:46:58.000000000 +0900
***************
*** 425,430 ****
--- 425,438 ----
SpinLockRelease(ProcStructLock);
/*
+ * Assign backend ID to auxiliary processes like backends, in order to
+ * allow multiplexing signal to auxiliary processes. Since backends use
+ * ID in the range from 1 to MaxBackends (inclusive), we assign
+ * auxiliary processes with MaxBackends + proctype + 1 as an unique ID.
+ */
+ MyBackendId = MaxBackends + proctype + 1;
+
+ /*
* Initialize all fields of MyProc, except for the semaphore which was
* prepared for us by InitProcGlobal.
*/
***************
*** 433,439 ****
MyProc->lxid = InvalidLocalTransactionId;
MyProc->xid = InvalidTransactionId;
MyProc->xmin = InvalidTransactionId;
! MyProc->backendId = InvalidBackendId;
MyProc->databaseId = InvalidOid;
MyProc->roleId = InvalidOid;
MyProc->inCommit = false;
--- 441,447 ----
MyProc->lxid = InvalidLocalTransactionId;
MyProc->xid = InvalidTransactionId;
MyProc->xmin = InvalidTransactionId;
! MyProc->backendId = MyBackendId;
MyProc->databaseId = InvalidOid;
MyProc->roleId = InvalidOid;
MyProc->inCommit = false;
diff -rcN base/src/backend/tcop/postgres.c new/src/backend/tcop/postgres.c
*** base/src/backend/tcop/postgres.c 2009-07-17 21:32:03.000000000 +0900
--- new/src/backend/tcop/postgres.c 2009-07-17 21:32:08.000000000 +0900
***************
*** 59,64 ****
--- 59,65 ----
#include "storage/bufmgr.h"
#include "storage/ipc.h"
#include "storage/proc.h"
+ #include "storage/procsignal.h"
#include "storage/sinval.h"
#include "tcop/fastpath.h"
#include "tcop/pquery.h"
***************
*** 3221,3227 ****
* of output during who-knows-what operation...
*/
pqsignal(SIGPIPE, SIG_IGN);
! pqsignal(SIGUSR1, CatchupInterruptHandler);
pqsignal(SIGUSR2, NotifyInterruptHandler);
pqsignal(SIGFPE, FloatExceptionHandler);
--- 3222,3228 ----
* of output during who-knows-what operation...
*/
pqsignal(SIGPIPE, SIG_IGN);
! pqsignal(SIGUSR1, procsignal_sigusr1_handler);
pqsignal(SIGUSR2, NotifyInterruptHandler);
pqsignal(SIGFPE, FloatExceptionHandler);
diff -rcN base/src/backend/utils/error/elog.c new/src/backend/utils/error/elog.c
*** base/src/backend/utils/error/elog.c 2009-07-17 21:32:03.000000000 +0900
--- new/src/backend/utils/error/elog.c 2009-07-17 21:32:08.000000000 +0900
***************
*** 1807,1813 ****
break;
case 'v':
/* keep VXID format in sync with lockfuncs.c */
! if (MyProc != NULL && MyProc->backendId != InvalidBackendId)
appendStringInfo(buf, "%d/%u",
MyProc->backendId, MyProc->lxid);
break;
--- 1807,1814 ----
break;
case 'v':
/* keep VXID format in sync with lockfuncs.c */
! if (MyProc != NULL &&
! MyProc->backendId >= 1 && MyProc->backendId <= MaxBackends)
appendStringInfo(buf, "%d/%u",
MyProc->backendId, MyProc->lxid);
break;
***************
*** 1955,1961 ****
/* Virtual transaction id */
/* keep VXID format in sync with lockfuncs.c */
! if (MyProc != NULL && MyProc->backendId != InvalidBackendId)
appendStringInfo(&buf, "%d/%u", MyProc->backendId, MyProc->lxid);
appendStringInfoChar(&buf, ',');
--- 1956,1963 ----
/* Virtual transaction id */
/* keep VXID format in sync with lockfuncs.c */
! if (MyProc != NULL &&
! MyProc->backendId >= 1 && MyProc->backendId <= MaxBackends)
appendStringInfo(&buf, "%d/%u", MyProc->backendId, MyProc->lxid);
appendStringInfoChar(&buf, ',');
diff -rcN base/src/backend/utils/init/postinit.c new/src/backend/utils/init/postinit.c
*** base/src/backend/utils/init/postinit.c 2009-07-17 21:32:03.000000000 +0900
--- new/src/backend/utils/init/postinit.c 2009-07-17 21:32:08.000000000 +0900
***************
*** 39,44 ****
--- 39,45 ----
#include "storage/lmgr.h"
#include "storage/proc.h"
#include "storage/procarray.h"
+ #include "storage/procsignal.h"
#include "storage/sinvaladt.h"
#include "storage/smgr.h"
#include "utils/acl.h"
***************
*** 451,456 ****
--- 452,459 ----
if (MyBackendId > MaxBackends || MyBackendId <= 0)
elog(FATAL, "bad backend id: %d", MyBackendId);
+ ProcSignalInit();
+
/*
* bufmgr needs another initialization call too
*/
diff -rcN base/src/include/storage/lock.h new/src/include/storage/lock.h
*** base/src/include/storage/lock.h 2009-07-17 21:32:03.000000000 +0900
--- new/src/include/storage/lock.h 2009-07-17 21:32:08.000000000 +0900
***************
*** 65,71 ****
#define InvalidLocalTransactionId 0
#define LocalTransactionIdIsValid(lxid) ((lxid) != InvalidLocalTransactionId)
#define VirtualTransactionIdIsValid(vxid) \
! (((vxid).backendId != InvalidBackendId) && \
LocalTransactionIdIsValid((vxid).localTransactionId))
#define VirtualTransactionIdEquals(vxid1, vxid2) \
((vxid1).backendId == (vxid2).backendId && \
--- 65,72 ----
#define InvalidLocalTransactionId 0
#define LocalTransactionIdIsValid(lxid) ((lxid) != InvalidLocalTransactionId)
#define VirtualTransactionIdIsValid(vxid) \
! (((vxid).backendId > InvalidBackendId) && \
! ((vxid).backendId <= MaxBackends) && \
LocalTransactionIdIsValid((vxid).localTransactionId))
#define VirtualTransactionIdEquals(vxid1, vxid2) \
((vxid1).backendId == (vxid2).backendId && \
diff -rcN base/src/include/storage/procsignal.h new/src/include/storage/procsignal.h
*** base/src/include/storage/procsignal.h 1970-01-01 09:00:00.000000000 +0900
--- new/src/include/storage/procsignal.h 2009-07-17 21:32:08.000000000 +0900
***************
*** 0 ****
--- 1,44 ----
+ /*-------------------------------------------------------------------------
+ *
+ * procsignal.h
+ * routines for signaling processes XXX
+ *
+ *
+ * Portions Copyright (c) 1996-2008, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
+ *
+ * $PostgreSQL$
+ *
+ *-------------------------------------------------------------------------
+ */
+ #ifndef PROCSIGNAL_H
+ #define PROCSIGNAL_H
+
+ #include "storage/backendid.h"
+
+
+ /*
+ * Reasons for signaling a process (a backend or an auxiliary process, like
+ * autovacuum worker). We can cope with simultaneous signals for different
+ * reasons. If the same reason is signaled multiple times in quick succession,
+ * however, the process is likely to observe only one notification of it.
+ * This is okay for the present uses.
+ */
+ typedef enum
+ {
+ PROCSIG_CATCHUP_INTERRUPT, /* catchup interrupt */
+ NUM_PROCSIGNALS /* Must be last value of enum! */
+ } ProcSignalReason;
+
+ /*
+ * prototypes for functions in procsignal.c
+ */
+ extern Size ProcSignalShmemSize(void);
+ extern void ProcSignalShmemInit(void);
+ extern void ProcSignalInit(void);
+ extern void SendProcSignal(pid_t pid, ProcSignalReason reason,
+ BackendId backendId);
+
+ extern void procsignal_sigusr1_handler(SIGNAL_ARGS);
+
+ #endif /* PROCSIGNAL_H */
diff -rcN base/src/include/storage/sinval.h new/src/include/storage/sinval.h
*** base/src/include/storage/sinval.h 2009-07-17 21:32:03.000000000 +0900
--- new/src/include/storage/sinval.h 2009-07-17 21:32:08.000000000 +0900
***************
*** 89,96 ****
void (*invalFunction) (SharedInvalidationMessage *msg),
void (*resetFunction) (void));
! /* signal handler for catchup events (SIGUSR1) */
! extern void CatchupInterruptHandler(SIGNAL_ARGS);
/*
* enable/disable processing of catchup events directly from signal handler.
--- 89,96 ----
void (*invalFunction) (SharedInvalidationMessage *msg),
void (*resetFunction) (void));
! /* signal handler for catchup events (PROCSIG_CATCHUP_INTERRUPT) */
! extern void HandleCatchupInterrupt(void);
/*
* enable/disable processing of catchup events directly from signal handler.
On Fri, Jul 17, 2009 at 8:58 AM, Fujii Masao<masao.fujii@gmail.com> wrote:
Hi,
On Fri, Jul 17, 2009 at 5:41 PM, Fujii Masao<masao.fujii@gmail.com> wrote:
I'm reviewing this patch:
http://archives.postgresql.org/message-id/3f0b79eb0907022341m1d36a841x19c3e2a5a6906b5b@mail.gmail.comI updated the patch to solve two problems which you pointed.
Here is the changes:
* Prevented the obsolete flag to being set to a new process, by using
newly-introduced spinlock.
thinking in ways to test the patch i tried this, the test at least try
to see if signals are managed correctly:
- patch, compile, install, initdb and start the service
- open five terminals:
on the first: make installcheck
on the second: pg_dumpall -p
port_to_an_existing_med_size_test_installation | psql
on third: psql -f /home/postgres/a_something_small_database.sql
on fourth: explain analyze with q as (select * from
generate_series(1, 1000000)
select * from q a, q
b, q c, q d, q e, q f;
on fifth: select procpid from pg_start_activity; and
pg_cancel_backend(randomly_choosen_pid);
when cancelling backends i got in a situation where i kill the explain
analyze in fourth session, execute again the pg_cancel_backend for the
same session and if i try to re-execute the same explain analyze it
got cancelled immediatly (seems like something don't get cleaned
appropiately).
once you get in this situation you can repeat that everytime you want;
bad enough, i wasn't able to repeat this on a new instalation and of
course i can't swear this is your patch fault...
--
Atentamente,
Jaime Casanova
Soporte y capacitación de PostgreSQL
Asesoría y desarrollo de sistemas
Guayaquil - Ecuador
Cel. +59387171157
On Fri, Jul 17, 2009 at 1:44 PM, Jaime
Casanova<jcasanov@systemguards.com.ec> wrote:
i wasn't able to repeat this on a new instalation and of
course i can't swear this is your patch fault...
this is not your patch fault but an existing bug, i repeat that
behaviour in an unpatched source tree...
with the steps in the previous mail i canceled 2 or 3 backend before
cancell the one with the explain analyze, i execute a second time that
pg_cancel_backend and if i try to re-run the query it gets cancelled
immediately, next time it runs normally...
pg_stat_activity reports that query as running no mather what...
--
Atentamente,
Jaime Casanova
Soporte y capacitación de PostgreSQL
Asesoría y desarrollo de sistemas
Guayaquil - Ecuador
Cel. +59387171157
Fujii Masao <masao.fujii@gmail.com> writes:
I updated the patch to solve two problems which you pointed.
Here is the changes:
* Prevented the obsolete flag to being set to a new process, by using
newly-introduced spinlock.
* Used the index of AuxiliaryProcs instead of auxType, to assign
backend ID to an auxiliary process.
Neither of these changes seem like a good idea to me. The use of a
spinlock renders the mechanism unsafe for use from the postmaster ---
we do not wish the postmaster to risk getting stuck if the contents of
shared memory have become corrupted, eg, there is a spinlock that looks
taken. And you've completely mangled the concept of BackendId.
MyBackendId is by definition the same as the index of the process's
entry in the sinval ProcState array. This means that (1) storing it in
the ProcState entry is silly, and (2) assigning values that don't
correspond to an actual ProcState entry is dangerous.
If we want to be able to signal processes that don't have a ProcState
entry, it would be better to assign an independent index instead of
overloading BackendId like this. I'm not sure though whether we care
about being able to signal such processes. It's certainly not necessary
for catchup interrupts, but it might be for future applications of the
mechanism. Do we have a clear idea of what the future applications are?
As for the locking issue, I'm inclined to just specify that uses of the
mechanism must be such that receiving a signal that wasn't meant for you
isn't fatal. In the case of catchup interrupts the worst that can
happen is you do a little bit of useless work. Are there any intended
uses where it would be seriously bad to get a misdirected signal?
I agree with Jaime that the patch would be more credible if it covered
more than one signal usage at the start --- these questions make it
clear that the design can't happen in a vacuum without intended usages
in mind.
regards, tom lane
Hi,
Thanks for reviewing the patch!
On Mon, Jul 27, 2009 at 2:43 AM, Tom Lane<tgl@sss.pgh.pa.us> wrote:
Neither of these changes seem like a good idea to me. The use of a
spinlock renders the mechanism unsafe for use from the postmaster ---
we do not wish the postmaster to risk getting stuck if the contents of
shared memory have become corrupted, eg, there is a spinlock that looks
taken. And you've completely mangled the concept of BackendId.
MyBackendId is by definition the same as the index of the process's
entry in the sinval ProcState array. This means that (1) storing it in
the ProcState entry is silly, and (2) assigning values that don't
correspond to an actual ProcState entry is dangerous.If we want to be able to signal processes that don't have a ProcState
entry, it would be better to assign an independent index instead of
overloading BackendId like this.
OK, I'll change the mechanism to assign a ProcSignalSlot to a process,
independently from ProcState, but similarly to ProcState: a process gets
a ProcSignalSlot from newly-introduced freelist at startup, and returns it
to freelist at exit. Since this assignment and return require the lock, I'll
introduce a new LWLock for them.
I'm not sure though whether we care
about being able to signal such processes. It's certainly not necessary
for catchup interrupts, but it might be for future applications of the
mechanism. Do we have a clear idea of what the future applications are?
Yes, I'm planning to use this mechanism for communication between
a process which can generate the WAL records and a WAL sender process,
in Synch Rep. Since not only a backend but also some auxiliary processes
(e.g., bgwriter) can generate the WAL records, processes which don't have
a ProcState need to receive the multiplexed signal, too.
As for the locking issue, I'm inclined to just specify that uses of the
mechanism must be such that receiving a signal that wasn't meant for you
isn't fatal.
This makes sense. I'll get rid of a spinlock from the patch and add the usage
note as above.
In the case of catchup interrupts the worst that can
happen is you do a little bit of useless work. Are there any intended
uses where it would be seriously bad to get a misdirected signal?
In, at least, currently-intended use case (i.e., Synch Rep), such wrong signal
is not fatal because it's only used as a hint.
I agree with Jaime that the patch would be more credible if it covered
more than one signal usage at the start --- these questions make it
clear that the design can't happen in a vacuum without intended usages
in mind.
Umm... the patch should cover a notify interrupt which currently uses
SIGUSR2?
Regards,
--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center
Fujii Masao <masao.fujii@gmail.com> writes:
On Mon, Jul 27, 2009 at 2:43 AM, Tom Lane<tgl@sss.pgh.pa.us> wrote:
If we want to be able to signal processes that don't have a ProcState
entry, it would be better to assign an independent index instead of
overloading BackendId like this.
OK, I'll change the mechanism to assign a ProcSignalSlot to a process,
independently from ProcState, but similarly to ProcState: a process gets
a ProcSignalSlot from newly-introduced freelist at startup, and returns it
to freelist at exit. Since this assignment and return require the lock, I'll
introduce a new LWLock for them.
I think you're making things more complicated when they should be
getting simpler.
It strikes me that the current API of "pass the BackendId if known or
InvalidBackendId if not" still works for processes without a BackendId,
as long as you can tolerate a bit of extra search overhead for them.
(You could reduce the search overhead by searching the array back to
front.) So a new process index may be overkill.
Do we have a clear idea of what the future applications are?
Yes, I'm planning to use this mechanism for communication between
a process which can generate the WAL records and a WAL sender process,
in Synch Rep. Since not only a backend but also some auxiliary processes
(e.g., bgwriter) can generate the WAL records, processes which don't have
a ProcState need to receive the multiplexed signal, too.
Huh? Wouldn't the bgwriter be *sending* the signal not receiving it?
Umm... the patch should cover a notify interrupt which currently uses
SIGUSR2?
Getting rid of the separate SIGUSR2 handler would definitely be a good
proof of concept that the mechanism works for more than one use.
regards, tom lane
Hi,
On Tue, Jul 28, 2009 at 12:09 AM, Tom Lane<tgl@sss.pgh.pa.us> wrote:
I think you're making things more complicated when they should be
getting simpler.It strikes me that the current API of "pass the BackendId if known or
InvalidBackendId if not" still works for processes without a BackendId,
as long as you can tolerate a bit of extra search overhead for them.
(You could reduce the search overhead by searching the array back to
front.) So a new process index may be overkill.
Yeah, this is very simple. I'll change the patch according to your suggestion.
Such extra search wouldn't be problem. Because an auxiliary process doesn't
need a catchup and notify interrupt which are intended uses. Also, because
there are few opportunities to send a multiplexed signal to an auxiliary process
in Synch Rep.
Do we have a clear idea of what the future applications are?
Yes, I'm planning to use this mechanism for communication between
a process which can generate the WAL records and a WAL sender process,
in Synch Rep. Since not only a backend but also some auxiliary processes
(e.g., bgwriter) can generate the WAL records, processes which don't have
a ProcState need to receive the multiplexed signal, too.Huh? Wouldn't the bgwriter be *sending* the signal not receiving it?
The bgwriter sends and receives a signal to/from the walsender process.
The walsender is new introduced process in Synch Rep, and there are
some free signals which aren't assigned yet. So, the signal sent from a
backend or an auxiliary process to the walsender doesn't need to be
multiplexed.
On the other hand, the signal sent to a backend which has no free signal
must be multiplexed. Since I'd like to treat a backend and an auxiliary
process as unified destination of a signal, I'd like to multiplex also a signal
sent to an auxiliary process. Of course, though we can change the method
of signaling according to the destination (e.g., call a native kill instead of
newly-introduced SendProcSignal when the destination is the bgwriter),
it seems to be awkward.
Umm... the patch should cover a notify interrupt which currently uses
SIGUSR2?Getting rid of the separate SIGUSR2 handler would definitely be a good
proof of concept that the mechanism works for more than one use.
OK. I'll change the patch as above.
Regards,
--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center