Re: [HACKERS] Multiplexing SUGUSR1
Hi, On Sun, Jan 11, 2009 at 7:29 PM, Fujii Masao masao.fu...@gmail.com wrote: Hi, On Thu, Jan 8, 2009 at 2:14 AM, Bruce Momjian br...@momjian.us wrote: Greg Stark wrote: On 7 Jan 2009, at 09:47, Bruce Momjian br...@momjian.us wrote: Heikki Linnakangas wrote: It's required by the sync replication patch, but has no value otherwise. Well, we have talked about allowing more signalling long-term, and this would accomplish that independent of the sync replication, so we might want to revisit this someday if it isn't included in sync replication. I also needed this for the progress indicator patch. I used SIGQUIT for the proof-of-concept patch but I wouldn't want to lose that signal for real. Yep, we want multiplexed signals independent of sync replication. The updated patch of multiplexing SIGUSR1 is included in v5 of synch-rep patch. (01_signal_handling.patch) http://wiki.postgresql.org/wiki/NTT%27s_Development_Projects#Version_History This patch can be also reviewed independent of synch-rep. Is this patch going to be reviewed and committed for 8.4? Though I think that this is useful also to the patch of those other than replication. Regards, -- Fujii Masao NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Multiplexing SUGUSR1
Hi, On Thu, Jan 8, 2009 at 2:14 AM, Bruce Momjian br...@momjian.us wrote: Greg Stark wrote: On 7 Jan 2009, at 09:47, Bruce Momjian br...@momjian.us wrote: Heikki Linnakangas wrote: It's required by the sync replication patch, but has no value otherwise. Well, we have talked about allowing more signalling long-term, and this would accomplish that independent of the sync replication, so we might want to revisit this someday if it isn't included in sync replication. I also needed this for the progress indicator patch. I used SIGQUIT for the proof-of-concept patch but I wouldn't want to lose that signal for real. Yep, we want multiplexed signals independent of sync replication. The updated patch of multiplexing SIGUSR1 is included in v5 of synch-rep patch. (01_signal_handling.patch) http://wiki.postgresql.org/wiki/NTT%27s_Development_Projects#Version_History This patch can be also reviewed independent of synch-rep. Regards, -- Fujii Masao NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Multiplexing SUGUSR1
Heikki Linnakangas wrote: It's required by the sync replication patch, but has no value otherwise. Well, we have talked about allowing more signalling long-term, and this would accomplish that independent of the sync replication, so we might want to revisit this someday if it isn't included in sync replication. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Multiplexing SUGUSR1
On 7 Jan 2009, at 09:47, Bruce Momjian br...@momjian.us wrote: Heikki Linnakangas wrote: It's required by the sync replication patch, but has no value otherwise. Well, we have talked about allowing more signalling long-term, and this would accomplish that independent of the sync replication, so we might want to revisit this someday if it isn't included in sync replication. I also needed this for the progress indicator patch. I used SIGQUIT for the proof-of-concept patch but I wouldn't want to lose that signal for real. -- Greg -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Multiplexing SUGUSR1
Greg Stark wrote: On 7 Jan 2009, at 09:47, Bruce Momjian br...@momjian.us wrote: Heikki Linnakangas wrote: It's required by the sync replication patch, but has no value otherwise. Well, we have talked about allowing more signalling long-term, and this would accomplish that independent of the sync replication, so we might want to revisit this someday if it isn't included in sync replication. I also needed this for the progress indicator patch. I used SIGQUIT for the proof-of-concept patch but I wouldn't want to lose that signal for real. Yep, we want multiplexed signals independent of sync replication. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Multiplexing SUGUSR1
Is this for 8.4? --- Heikki Linnakangas 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Multiplexing SUGUSR1
It's required by the sync replication patch, but has no value otherwise. Bruce Momjian wrote: Is this for 8.4? --- Heikki Linnakangas 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Multiplexing SUGUSR1
Hi, On Wed, Jan 7, 2009 at 3:12 PM, Heikki Linnakangas heikki.linnakan...@enterprisedb.com wrote: It's required by the sync replication patch, but has no value otherwise. Yes. I'm reworking Synch Rep also including the patch. BTW, attached is the patch. Regards, -- Fujii Masao NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center diff -rc base/src/backend/bootstrap/bootstrap.c new/src/backend/bootstrap/bootstrap.c *** base/src/backend/bootstrap/bootstrap.c 2009-01-06 21:19:07.0 +0900 --- new/src/backend/bootstrap/bootstrap.c 2009-01-06 21:19:23.0 +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/flatfiles.h *** *** 389,394 --- 390,406 InitAuxiliaryProcess(); #endif + /* + * 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; + + ProcSignalInit(); + /* finish setting up bufmgr.c */ InitBufferPoolBackend(); diff -rc base/src/backend/postmaster/autovacuum.c new/src/backend/postmaster/autovacuum.c *** base/src/backend/postmaster/autovacuum.c 2009-01-06 21:19:07.0 +0900 --- new/src/backend/postmaster/autovacuum.c 2009-01-06 21:19:23.0 +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 *** *** 1477,1483 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); --- 1478,1484 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 -rc base/src/backend/storage/ipc/Makefile new/src/backend/storage/ipc/Makefile *** base/src/backend/storage/ipc/Makefile 2009-01-06 21:19:07.0 +0900 --- new/src/backend/storage/ipc/Makefile 2009-01-06 21:19:23.0 +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 -rc base/src/backend/storage/ipc/ipci.c new/src/backend/storage/ipc/ipci.c *** base/src/backend/storage/ipc/ipci.c 2009-01-06 21:19:07.0 +0900 --- new/src/backend/storage/ipc/ipci.c 2009-01-06 21:19:23.0 +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 *** *** 113,118 --- 114,120 size = add_size(size, SInvalShmemSize()); 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 *** *** 209,214 --- 211,217 PMSignalInit(); BgWriterShmemInit(); AutoVacuumShmemInit(); + ProcSignalShmemInit(); /* * Set up other modules that need some shared memory space diff -rc base/src/backend/storage/ipc/procsignal.c new/src/backend/storage/ipc/procsignal.c *** base/src/backend/storage/ipc/procsignal.c 2009-01-06 21:19:07.0 +0900 --- new/src/backend/storage/ipc/procsignal.c 2009-01-06 21:19:23.0 +0900 *** *** 0 --- 1,216 + /*- + * + * 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
Re: [HACKERS] Multiplexing SUGUSR1
Hi, Sorry for this late reply. On Thu, Dec 11, 2008 at 3:12 PM, Heikki Linnakangas heikki.linnakan...@enterprisedb.com wrote: Fujii Masao wrote: On Thu, Dec 11, 2008 at 10:55 AM, Fujii Masao masao.fu...@gmail.com wrote: I will update the patch based on yours, and add the support for auxiliary processes into it. Or, should I leave renewal of the patch to you? Of course, if you don't have time, I will do. I can do it, it's just not a big deal. Heikki, thanks for your offer! I'd like to leave it to you if it's OK with you. Regards, -- Fujii Masao NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Multiplexing SUGUSR1
Hi, Alvaro Herrera wrote: No, the signalling needed here is far simpler than Markus' IMessage stuff. Yup, see also Tom's comment [1]. For Postgres-R I'm currently multiplexing by embedding a message type in the imessage data itself. So this part is certainly overlapping, yes. Some of the messages I'm using do have additional payload data, others don't. Moving this message type out of the body part of the message itself and instead use the upcoming signal multiplexing could save a few imessage types in favor of using these multiplexed signals. Most message types require some additional data to be transferred, though. From my point of view it's hard to understand why one should want to move out exactly 32 or 64 bits (sig_atomic_t) of a message. From the point of view of Postgres, it's certainly better than being bound to the existing Unix signals. Regards Markus Wanner [1]: http://archives.postgresql.org/message-id/28487.1221147...@sss.pgh.pa.us -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Multiplexing SUGUSR1
Fujii Masao wrote: Hi, On Wed, Dec 10, 2008 at 1:43 AM, Tom Lane [EMAIL PROTECTED] wrote: Heikki Linnakangas [EMAIL PROTECTED] writes: I'm surprised you feel that way. You suggested earlier (http://archives.postgresql.org/message-id/[EMAIL PROTECTED]) that a solution that only works for processes attached to shared memory would probably suffice for now. Well, I wasn't complaining about the dependence on being attached to shared memory. What I'm complaining about is the dependence on the rather complex PGPROC data structure. That seems hard, considering that we also want it to work without locking. Hmm, I presume we can use spinlocks in a signal handler? Perhaps some sort of a hash table protected by a spinlock would work. No, locks are right out if the postmaster is supposed to be able to use it. What I was thinking of is a simple linear array of PIDs and sig_atomic_t flags. The slots could be assigned on the basis of backendid, but callers trying to send a signal would have to scan the array looking for the matching PID. (This doesn't seem outlandishly expensive considering that one is about to do a kernel call anyway. You might be able to save a few cycles by having the PID array separate from the flag array, which should improve the cache friendliness of the scan.) Also, for those callers who do have access to a PGPROC, there could be a separate entry point that passes backendid instead of PID to eliminate the search. Thanks for the comment! I updated the patch so. Is this patch ready to apply? Looks like we stepped on each others toes, I was just about to send a similar patch. Attached is my version, it's essentially the same as yours. My version doesn't have support for auxiliary processes. Does the synchronous replication patch need that? This comment is wrong, though the code below it is right: *** base/src/backend/bootstrap/bootstrap.c 2008-12-10 16:29:10.0 +0900 --- new/src/backend/bootstrap/bootstrap.c 2008-12-10 17:16:23.0 +0900 *** *** 389,394 --- 389,403 InitAuxiliaryProcess(); #endif + /* +* 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, we assign auxiliary processes +* with MaxBackends + AuxProcType as an unique ID. +*/ + MyBackendId = MaxBackends + auxType; + MyProc-backendId = MyBackendId; + /* finish setting up bufmgr.c */ InitBufferPoolBackend(); Backends use IDs in the range 0 to MaxBackends-1, inclusive. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com *** src/backend/postmaster/autovacuum.c --- src/backend/postmaster/autovacuum.c *** *** 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 *** *** 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); --- 1478,1484 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); *** src/backend/storage/ipc/Makefile --- src/backend/storage/ipc/Makefile *** *** 15,21 override CFLAGS+= -fno-inline 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 *** src/backend/storage/ipc/ipci.c --- src/backend/storage/ipc/ipci.c *** *** 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 *** *** 111,116 CreateSharedMemoryAndSemaphores(bool makePrivate, int port) --- 112,118 size = add_size(size, SInvalShmemSize()); 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 *** *** 207,212 CreateSharedMemoryAndSemaphores(bool makePrivate,
Re: [HACKERS] Multiplexing SUGUSR1
Hi, On Wed, Dec 10, 2008 at 1:43 AM, Tom Lane [EMAIL PROTECTED] wrote: Heikki Linnakangas [EMAIL PROTECTED] writes: I'm surprised you feel that way. You suggested earlier (http://archives.postgresql.org/message-id/[EMAIL PROTECTED]) that a solution that only works for processes attached to shared memory would probably suffice for now. Well, I wasn't complaining about the dependence on being attached to shared memory. What I'm complaining about is the dependence on the rather complex PGPROC data structure. That seems hard, considering that we also want it to work without locking. Hmm, I presume we can use spinlocks in a signal handler? Perhaps some sort of a hash table protected by a spinlock would work. No, locks are right out if the postmaster is supposed to be able to use it. What I was thinking of is a simple linear array of PIDs and sig_atomic_t flags. The slots could be assigned on the basis of backendid, but callers trying to send a signal would have to scan the array looking for the matching PID. (This doesn't seem outlandishly expensive considering that one is about to do a kernel call anyway. You might be able to save a few cycles by having the PID array separate from the flag array, which should improve the cache friendliness of the scan.) Also, for those callers who do have access to a PGPROC, there could be a separate entry point that passes backendid instead of PID to eliminate the search. Thanks for the comment! I updated the patch so. Is this patch ready to apply? Regards, -- Fujii Masao NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center diff -rc base/src/backend/bootstrap/bootstrap.c new/src/backend/bootstrap/bootstrap.c *** base/src/backend/bootstrap/bootstrap.c 2008-12-10 16:29:10.0 +0900 --- new/src/backend/bootstrap/bootstrap.c 2008-12-10 17:16:23.0 +0900 *** *** 389,394 --- 389,403 InitAuxiliaryProcess(); #endif + /* + * 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, we assign auxiliary processes + * with MaxBackends + AuxProcType as an unique ID. + */ + MyBackendId = MaxBackends + auxType; + MyProc-backendId = MyBackendId; + /* finish setting up bufmgr.c */ InitBufferPoolBackend(); diff -rc base/src/backend/commands/async.c new/src/backend/commands/async.c *** base/src/backend/commands/async.c 2008-12-10 16:29:10.0 +0900 --- new/src/backend/commands/async.c 2008-12-10 17:27:31.0 +0900 *** *** 915,923 * 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 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) diff -rc base/src/backend/postmaster/autovacuum.c new/src/backend/postmaster/autovacuum.c *** base/src/backend/postmaster/autovacuum.c 2008-12-10 16:29:10.0 +0900 --- new/src/backend/postmaster/autovacuum.c 2008-12-10 17:28:05.0 +0900 *** *** 1477,1483 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); diff -rc base/src/backend/storage/ipc/Makefile new/src/backend/storage/ipc/Makefile *** base/src/backend/storage/ipc/Makefile 2008-12-10 16:29:10.0 +0900 --- new/src/backend/storage/ipc/Makefile 2008-12-10 17:24:58.0 +0900 *** *** 15,21 endif endif ! OBJS = ipc.o ipci.o
Re: [HACKERS] Multiplexing SUGUSR1
Fujii Masao wrote: Hi, On Wed, Dec 10, 2008 at 1:43 AM, Tom Lane [EMAIL PROTECTED] wrote: Heikki Linnakangas [EMAIL PROTECTED] writes: I'm surprised you feel that way. You suggested earlier (http://archives.postgresql.org/message-id/[EMAIL PROTECTED]) that a solution that only works for processes attached to shared memory would probably suffice for now. Well, I wasn't complaining about the dependence on being attached to shared memory. What I'm complaining about is the dependence on the rather complex PGPROC data structure. That seems hard, considering that we also want it to work without locking. Hmm, I presume we can use spinlocks in a signal handler? Perhaps some sort of a hash table protected by a spinlock would work. No, locks are right out if the postmaster is supposed to be able to use it. What I was thinking of is a simple linear array of PIDs and sig_atomic_t flags. The slots could be assigned on the basis of backendid, but callers trying to send a signal would have to scan the array looking for the matching PID. (This doesn't seem outlandishly expensive considering that one is about to do a kernel call anyway. You might be able to save a few cycles by having the PID array separate from the flag array, which should improve the cache friendliness of the scan.) Also, for those callers who do have access to a PGPROC, there could be a separate entry point that passes backendid instead of PID to eliminate the search. Thanks for the comment! I updated the patch so. Is this patch ready to apply? On closer look, I don't see anything setting ProcSignalData.pid field. Which make me believe that the patch can't possibly work. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Multiplexing SUGUSR1
Hi, My version doesn't have support for auxiliary processes. Does the synchronous replication patch need that? Yes, the background writer also generates the WAL like a backend, so it has to be able to communicate with walsender. On closer look, I don't see anything setting ProcSignalData.pid field. Which make me believe that the patch can't possibly work. Ooops! My patch has some bugs :( I will update the patch based on yours, and add the support for auxiliary processes into it. Regards, -- Fujii Masao NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Multiplexing SUGUSR1
Hi, On Thu, Dec 11, 2008 at 10:55 AM, Fujii Masao [EMAIL PROTECTED] wrote: Hi, My version doesn't have support for auxiliary processes. Does the synchronous replication patch need that? Yes, the background writer also generates the WAL like a backend, so it has to be able to communicate with walsender. On closer look, I don't see anything setting ProcSignalData.pid field. Which make me believe that the patch can't possibly work. Ooops! My patch has some bugs :( I will update the patch based on yours, and add the support for auxiliary processes into it. Or, should I leave renewal of the patch to you? Of course, if you don't have time, I will do. Regards, -- Fujii Masao NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Multiplexing SUGUSR1
Fujii Masao wrote: On Thu, Dec 11, 2008 at 10:55 AM, Fujii Masao [EMAIL PROTECTED] wrote: I will update the patch based on yours, and add the support for auxiliary processes into it. Or, should I leave renewal of the patch to you? Of course, if you don't have time, I will do. I can do it, it's just not a big deal. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Multiplexing SUGUSR1
Fujii Masao wrote: Hi, On Mon, Dec 8, 2008 at 11:39 PM, Heikki Linnakangas [EMAIL PROTECTED] wrote: Tom Lane wrote: Heikki Linnakangas [EMAIL PROTECTED] writes: 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? No. If it's trying to do that then it's broken. In fact, if it's trying to do much of anything beyond setting a volatile flag variable in a signal handler, it's broken --- unless there are special provisions to limit where the signal trap can occur, which would be pretty much unacceptable for a multiplexed-signal implementation. Ok, I was afraid so. I think we'll need to replace the proposed bitmask with an array of sig_atomic_t flags then, and do without locking. Thanks! I updated the patch so (based on signal_handling_v2-heikki-1.patch). Thank you. Looks good to me, committed with minor changes. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Multiplexing SUGUSR1
Heikki Linnakangas [EMAIL PROTECTED] writes: Thank you. Looks good to me, committed with minor changes. I don't think this patch is anywhere near ready to apply. In the first place, touching the PGPROC like that without any lock seems completely unsafe --- among other things, you're relying on an undocumented assumption that the space occupied by a PGPROC struct will never be recycled for use as anything else. It might be all right for the limited purposes at the moment, but if you are advertising this as a general purpose signal multiplexer then it will very soon not be all right. For the same reason, it seems like a bad design to set this up so that the postmaster can't possibly use the mechanism safely. (Before a couple of months ago, this wouldn't even have worked to replace the existing use of SIGUSR1.) And in the third place, this doesn't work unless one has one's hands on the target backend's PGPROC, and not merely its PID. I object to the changes in sinvaladt.c altogether, and note that this decision makes it impossible to fold SIGUSR2 handling into the multiplex code, which is another simple proof that it fails to qualify as a general-purpose multiplexer. I think we need something closer to the postmaster signal multiplexing mechanism, wherein there is a dedicated shared memory area of static layout that holds the signaling flags. And it needs to be driven off of knowing the target's PID, not anything else. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Multiplexing SUGUSR1
Tom Lane wrote: Heikki Linnakangas [EMAIL PROTECTED] writes: Thank you. Looks good to me, committed with minor changes. I don't think this patch is anywhere near ready to apply. Ok, I'll revert it if you feel that strongly. In the first place, touching the PGPROC like that without any lock seems completely unsafe --- among other things, you're relying on an undocumented assumption that the space occupied by a PGPROC struct will never be recycled for use as anything else. Right, it does depend on that. It might be all right for the limited purposes at the moment, but if you are advertising this as a general purpose signal multiplexer then it will very soon not be all right. For the same reason, it seems like a bad design to set this up so that the postmaster can't possibly use the mechanism safely. (Before a couple of months ago, this wouldn't even have worked to replace the existing use of SIGUSR1.) And in the third place, this doesn't work unless one has one's hands on the target backend's PGPROC, and not merely its PID. I object to the changes in sinvaladt.c altogether, and note that this decision makes it impossible to fold SIGUSR2 handling into the multiplex code, which is another simple proof that it fails to qualify as a general-purpose multiplexer. I'm surprised you feel that way. You suggested earlier (http://archives.postgresql.org/message-id/[EMAIL PROTECTED]) that a solution that only works for processes attached to shared memory would probably suffice for now. I think we need something closer to the postmaster signal multiplexing mechanism, wherein there is a dedicated shared memory area of static layout that holds the signaling flags. And it needs to be driven off of knowing the target's PID, not anything else. That seems hard, considering that we also want it to work without locking. Hmm, I presume we can use spinlocks in a signal handler? Perhaps some sort of a hash table protected by a spinlock would work. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Multiplexing SUGUSR1
Hi, I hope I'm not disturbing hackers at work by talking about completely unrelated things but... Le mardi 09 décembre 2008, Tom Lane a écrit : I think we need something closer to the postmaster signal multiplexing mechanism, wherein there is a dedicated shared memory area of static layout that holds the signaling flags. And it needs to be driven off of knowing the target's PID, not anything else. ...this makes me recall IMessage Queues from Postgres-R, reworked by Markus to follow your advices about postmaster and shared memory. http://archives.postgresql.org/pgsql-hackers/2008-07/msg01420.php Could it be the implementation we need for multiplexing signals from one backend some others? Regards, -- dim signature.asc Description: This is a digitally signed message part.
Re: [HACKERS] Multiplexing SUGUSR1
Dimitri Fontaine escribió: Le mardi 09 décembre 2008, Tom Lane a écrit : I think we need something closer to the postmaster signal multiplexing mechanism, wherein there is a dedicated shared memory area of static layout that holds the signaling flags. And it needs to be driven off of knowing the target's PID, not anything else. ...this makes me recall IMessage Queues from Postgres-R, reworked by Markus to follow your advices about postmaster and shared memory. http://archives.postgresql.org/pgsql-hackers/2008-07/msg01420.php Could it be the implementation we need for multiplexing signals from one backend some others? No, the signalling needed here is far simpler than Markus' IMessage stuff. -- Alvaro Herrerahttp://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Multiplexing SUGUSR1
Heikki Linnakangas [EMAIL PROTECTED] writes: I'm surprised you feel that way. You suggested earlier (http://archives.postgresql.org/message-id/[EMAIL PROTECTED]) that a solution that only works for processes attached to shared memory would probably suffice for now. Well, I wasn't complaining about the dependence on being attached to shared memory. What I'm complaining about is the dependence on the rather complex PGPROC data structure. That seems hard, considering that we also want it to work without locking. Hmm, I presume we can use spinlocks in a signal handler? Perhaps some sort of a hash table protected by a spinlock would work. No, locks are right out if the postmaster is supposed to be able to use it. What I was thinking of is a simple linear array of PIDs and sig_atomic_t flags. The slots could be assigned on the basis of backendid, but callers trying to send a signal would have to scan the array looking for the matching PID. (This doesn't seem outlandishly expensive considering that one is about to do a kernel call anyway. You might be able to save a few cycles by having the PID array separate from the flag array, which should improve the cache friendliness of the scan.) Also, for those callers who do have access to a PGPROC, there could be a separate entry point that passes backendid instead of PID to eliminate the search. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Multiplexing SUGUSR1
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
Re: [HACKERS] Multiplexing SUGUSR1
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
Re: [HACKERS] Multiplexing SUGUSR1
Greg Stark wrote: Does this signal multiplexing solve the out of signals problem we have generally? It's a general solution, but it relies on flags in PGPROC, so it'll only work for backends and auxiliary processes that have a PGPROC entry. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Multiplexing SUGUSR1
Heikki Linnakangas [EMAIL PROTECTED] writes: 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? No. If it's trying to do that then it's broken. In fact, if it's trying to do much of anything beyond setting a volatile flag variable in a signal handler, it's broken --- unless there are special provisions to limit where the signal trap can occur, which would be pretty much unacceptable for a multiplexed-signal implementation. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Multiplexing SUGUSR1
On Mon, 2008-12-08 at 10:04 +0200, Heikki Linnakangas wrote: And is the performance impact of that acceptable? No, but I think we already agreed to change that to a separate lwlock. -- Simon Riggs www.2ndQuadrant.com PostgreSQL Training, Services and Support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Multiplexing SUGUSR1
Tom Lane wrote: Heikki Linnakangas [EMAIL PROTECTED] writes: 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? No. If it's trying to do that then it's broken. In fact, if it's trying to do much of anything beyond setting a volatile flag variable in a signal handler, it's broken --- unless there are special provisions to limit where the signal trap can occur, which would be pretty much unacceptable for a multiplexed-signal implementation. Ok, I was afraid so. I think we'll need to replace the proposed bitmask with an array of sig_atomic_t flags then, and do without locking. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Multiplexing SUGUSR1
Hi, On Mon, Dec 8, 2008 at 11:39 PM, Heikki Linnakangas [EMAIL PROTECTED] wrote: Tom Lane wrote: Heikki Linnakangas [EMAIL PROTECTED] writes: 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? No. If it's trying to do that then it's broken. In fact, if it's trying to do much of anything beyond setting a volatile flag variable in a signal handler, it's broken --- unless there are special provisions to limit where the signal trap can occur, which would be pretty much unacceptable for a multiplexed-signal implementation. Ok, I was afraid so. I think we'll need to replace the proposed bitmask with an array of sig_atomic_t flags then, and do without locking. Thanks! I updated the patch so (based on signal_handling_v2-heikki-1.patch). Regards, -- Fujii Masao NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center diff -rc base/src/backend/access/transam/twophase.c new/src/backend/access/transam/twophase.c *** base/src/backend/access/transam/twophase.c 2008-12-09 11:45:21.0 +0900 --- new/src/backend/access/transam/twophase.c 2008-12-09 12:38:52.0 +0900 *** *** 287,292 --- 287,293 gxact-proc.databaseId = databaseid; gxact-proc.roleId = owner; gxact-proc.inCommit = false; + MemSet(gxact-proc.signalFlags, 0, NUM_PROCSIGNALS * sizeof(sig_atomic_t)); gxact-proc.vacuumFlags = 0; gxact-proc.lwWaiting = false; gxact-proc.lwExclusive = false; diff -rc base/src/backend/commands/async.c new/src/backend/commands/async.c *** base/src/backend/commands/async.c 2008-12-09 11:45:21.0 +0900 --- new/src/backend/commands/async.c 2008-12-09 11:45:43.0 +0900 *** *** 915,923 * 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 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) diff -rc base/src/backend/postmaster/autovacuum.c new/src/backend/postmaster/autovacuum.c *** base/src/backend/postmaster/autovacuum.c 2008-12-09 11:45:21.0 +0900 --- new/src/backend/postmaster/autovacuum.c 2008-12-09 11:45:43.0 +0900 *** *** 1477,1483 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); diff -rc base/src/backend/storage/ipc/sinval.c new/src/backend/storage/ipc/sinval.c *** base/src/backend/storage/ipc/sinval.c 2008-12-09 11:45:21.0 +0900 --- new/src/backend/storage/ipc/sinval.c 2008-12-09 11:45:43.0 +0900 *** *** 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 /* ! *
