From ebfc56d3fb41d914c799555a1f4c9d1a72379e9f Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Sun, 23 May 2004 03:50:45 +0000 Subject: [PATCH] Handle impending sinval queue overflow by means of a separate signal (SIGUSR1, which we have not been using recently) instead of piggybacking on SIGUSR2-driven NOTIFY processing. This has several good results: the processing needed to drain the sinval queue is a lot less than the processing needed to answer a NOTIFY; there's less contention since we don't have a bunch of backends all trying to acquire exclusive lock on pg_listener; backends that are sitting inside a transaction block can still drain the queue, whereas NOTIFY processing can't run if there's an open transaction block. (This last is a fairly serious issue that I don't think we ever recognized before --- with clients like JDBC that tend to sit with open transaction blocks, the sinval queue draining mechanism never really worked as intended, probably resulting in a lot of useless cache-reset overhead.) This is the last of several proposed changes in response to Philip Warner's recent report of sinval-induced performance problems. --- src/backend/commands/async.c | 34 ++-- src/backend/postmaster/postmaster.c | 8 +- src/backend/storage/ipc/sinval.c | 231 +++++++++++++++++++++++++++- src/backend/storage/ipc/sinvaladt.c | 10 +- src/backend/tcop/postgres.c | 15 +- src/include/commands/async.h | 9 +- src/include/storage/pmsignal.h | 4 +- src/include/storage/sinval.h | 15 +- 8 files changed, 289 insertions(+), 37 deletions(-) diff --git a/src/backend/commands/async.c b/src/backend/commands/async.c index 5233e78178..15d7c0aae5 100644 --- a/src/backend/commands/async.c +++ b/src/backend/commands/async.c @@ -7,7 +7,7 @@ * Portions Copyright (c) 1994, Regents of the University of California * * IDENTIFICATION - * $PostgreSQL: pgsql/src/backend/commands/async.c,v 1.110 2004/05/22 21:58:24 tgl Exp $ + * $PostgreSQL: pgsql/src/backend/commands/async.c,v 1.111 2004/05/23 03:50:45 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -86,6 +86,7 @@ #include "libpq/pqformat.h" #include "miscadmin.h" #include "storage/ipc.h" +#include "storage/sinval.h" #include "tcop/tcopprot.h" #include "utils/fmgroids.h" #include "utils/ps_status.h" @@ -607,7 +608,7 @@ AtAbort_Notify(void) /* *-------------------------------------------------------------- - * Async_NotifyHandler + * NotifyInterruptHandler * * This is the signal handler for SIGUSR2. * @@ -623,7 +624,7 @@ AtAbort_Notify(void) *-------------------------------------------------------------- */ void -Async_NotifyHandler(SIGNAL_ARGS) +NotifyInterruptHandler(SIGNAL_ARGS) { int save_errno = errno; @@ -669,12 +670,12 @@ Async_NotifyHandler(SIGNAL_ARGS) { /* Here, it is finally safe to do stuff. */ if (Trace_notify) - elog(DEBUG1, "Async_NotifyHandler: perform async notify"); + elog(DEBUG1, "NotifyInterruptHandler: perform async notify"); ProcessIncomingNotify(); if (Trace_notify) - elog(DEBUG1, "Async_NotifyHandler: done"); + elog(DEBUG1, "NotifyInterruptHandler: done"); } } @@ -766,12 +767,20 @@ EnableNotifyInterrupt(void) * This is called by the PostgresMain main loop just after receiving * 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. * -------------------------------------------------------------- */ -void +bool DisableNotifyInterrupt(void) { + bool result = (notifyInterruptEnabled != 0); + notifyInterruptEnabled = 0; + + return result; } /* @@ -785,10 +794,6 @@ DisableNotifyInterrupt(void) * and clear the notification field in pg_listener until next time. * * NOTE: since we are outside any transaction, we must create our own. - * - * Results: - * XXX - * * -------------------------------------------------------------- */ static void @@ -803,11 +808,15 @@ ProcessIncomingNotify(void) Datum value[Natts_pg_listener]; char repl[Natts_pg_listener], nulls[Natts_pg_listener]; + bool catchup_enabled; + + /* Must prevent SIGUSR1 interrupt while I am running */ + catchup_enabled = DisableCatchupInterrupt(); if (Trace_notify) elog(DEBUG1, "ProcessIncomingNotify"); - set_ps_display("async_notify"); + set_ps_display("notify interrupt"); notifyInterruptOccurred = 0; @@ -883,6 +892,9 @@ ProcessIncomingNotify(void) if (Trace_notify) elog(DEBUG1, "ProcessIncomingNotify: done"); + + if (catchup_enabled) + EnableCatchupInterrupt(); } /* diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c index c141ca6787..01c2ed3eaf 100644 --- a/src/backend/postmaster/postmaster.c +++ b/src/backend/postmaster/postmaster.c @@ -37,7 +37,7 @@ * * * IDENTIFICATION - * $PostgreSQL: pgsql/src/backend/postmaster/postmaster.c,v 1.393 2004/05/21 05:07:57 tgl Exp $ + * $PostgreSQL: pgsql/src/backend/postmaster/postmaster.c,v 1.394 2004/05/23 03:50:45 tgl Exp $ * * NOTES * @@ -2861,11 +2861,11 @@ sigusr1_handler(SIGNAL_ARGS) if (CheckPostmasterSignal(PMSIGNAL_WAKEN_CHILDREN)) { /* - * Send SIGUSR2 to all children (triggers AsyncNotifyHandler). See - * storage/ipc/sinvaladt.c for the use of this. + * Send SIGUSR1 to all children (triggers CatchupInterruptHandler). + * See storage/ipc/sinval[adt].c for the use of this. */ if (Shutdown == NoShutdown) - SignalChildren(SIGUSR2); + SignalChildren(SIGUSR1); } PG_SETMASK(&UnBlockSig); diff --git a/src/backend/storage/ipc/sinval.c b/src/backend/storage/ipc/sinval.c index f13084850b..570feab25e 100644 --- a/src/backend/storage/ipc/sinval.c +++ b/src/backend/storage/ipc/sinval.c @@ -8,20 +8,46 @@ * * * IDENTIFICATION - * $PostgreSQL: pgsql/src/backend/storage/ipc/sinval.c,v 1.62 2003/11/29 19:51:56 pgsql Exp $ + * $PostgreSQL: pgsql/src/backend/storage/ipc/sinval.c,v 1.63 2004/05/23 03:50:45 tgl Exp $ * *------------------------------------------------------------------------- */ #include "postgres.h" +#include +#include "commands/async.h" +#include "storage/ipc.h" #include "storage/proc.h" #include "storage/sinval.h" #include "storage/sinvaladt.h" +#include "utils/inval.h" #include "utils/tqual.h" #include "miscadmin.h" +/* + * 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 everyone + * through a cache reset exercise. This is done by broadcasting SIGUSR1 + * to all backends when the queue is threatening to become full. + * + * State for catchup events consists of two flags: one saying whether + * the signal handler is currently allowed to call ProcessCatchupEvent + * directly, and one saying whether the signal has occurred but the handler + * was not allowed to call ProcessCatchupEvent at the time. + * + * NB: the "volatile" on these declarations is critical! If your compiler + * does not grok "volatile", you'd be best advised to compile this file + * with all optimization turned off. + */ +static volatile int catchupInterruptEnabled = 0; +static volatile int catchupInterruptOccurred = 0; + +static void ProcessCatchupEvent(void); + + /****************************************************************************/ /* CreateSharedInvalidationState() Initialize SI buffer */ /* */ @@ -91,6 +117,12 @@ ReceiveSharedInvalidMessages( for (;;) { + /* + * We can discard any pending catchup event, since we will not exit + * this loop until we're fully caught up. + */ + catchupInterruptOccurred = 0; + /* * We can run SIGetDataEntry in parallel with other backends * running SIGetDataEntry for themselves, since each instance will @@ -137,6 +169,203 @@ 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 + * to do it later. (Note that it's quite possible for normal processing + * of the current transaction to cause ReceiveSharedInvalidMessages() + * to be run later on; in that case the flag will get cleared again, + * 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 */ + if (proc_exit_inprogress) + return; + + if (catchupInterruptEnabled) + { + bool save_ImmediateInterruptOK = ImmediateInterruptOK; + + /* + * We may be called while ImmediateInterruptOK is true; turn it + * off while messing with the catchup state. (We would have to + * save and restore it anyway, because PGSemaphore operations + * inside ProcessCatchupEvent() might reset it.) + */ + ImmediateInterruptOK = false; + + /* + * I'm not sure whether some flavors of Unix might allow another + * SIGUSR1 occurrence to recursively interrupt this routine. To + * cope with the possibility, we do the same sort of dance that + * EnableCatchupInterrupt must do --- see that routine for + * comments. + */ + catchupInterruptEnabled = 0; /* disable any recursive signal */ + catchupInterruptOccurred = 1; /* do at least one iteration */ + for (;;) + { + catchupInterruptEnabled = 1; + if (!catchupInterruptOccurred) + break; + catchupInterruptEnabled = 0; + if (catchupInterruptOccurred) + { + /* Here, it is finally safe to do stuff. */ + ProcessCatchupEvent(); + } + } + + /* + * Restore ImmediateInterruptOK, and check for interrupts if + * needed. + */ + ImmediateInterruptOK = save_ImmediateInterruptOK; + if (save_ImmediateInterruptOK) + CHECK_FOR_INTERRUPTS(); + } + else + { + /* + * In this path it is NOT SAFE to do much of anything, except + * this: + */ + catchupInterruptOccurred = 1; + } + + errno = save_errno; +} + +/* + * EnableCatchupInterrupt + * + * This is called by the PostgresMain main loop just before waiting + * for a frontend command. We process any pending catchup events, + * and enable the signal handler to process future events directly. + * + * NOTE: the signal handler starts out disabled, and stays so until + * PostgresMain calls this the first time. + */ +void +EnableCatchupInterrupt(void) +{ + /* + * This code is tricky because we are communicating with a signal + * handler that could interrupt us at any point. If we just checked + * catchupInterruptOccurred and then set catchupInterruptEnabled, we + * could fail to respond promptly to a signal that happens in between + * those two steps. (A very small time window, perhaps, but Murphy's + * Law says you can hit it...) Instead, we first set the enable flag, + * then test the occurred flag. If we see an unserviced interrupt has + * occurred, we re-clear the enable flag before going off to do the + * service work. (That prevents re-entrant invocation of + * ProcessCatchupEvent() if another interrupt occurs.) If an + * interrupt comes in between the setting and clearing of + * catchupInterruptEnabled, then it will have done the service work and + * left catchupInterruptOccurred zero, so we have to check again after + * clearing enable. The whole thing has to be in a loop in case + * another interrupt occurs while we're servicing the first. Once we + * get out of the loop, enable is set and we know there is no + * unserviced interrupt. + * + * NB: an overenthusiastic optimizing compiler could easily break this + * code. Hopefully, they all understand what "volatile" means these + * days. + */ + for (;;) + { + catchupInterruptEnabled = 1; + if (!catchupInterruptOccurred) + break; + catchupInterruptEnabled = 0; + if (catchupInterruptOccurred) + { + ProcessCatchupEvent(); + } + } +} + +/* + * DisableCatchupInterrupt + * + * This is called by the PostgresMain main loop just after receiving + * a frontend command. Signal handler execution of catchup events + * is disabled until the next EnableCatchupInterrupt call. + * + * The SIGUSR2 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 +DisableCatchupInterrupt(void) +{ + bool result = (catchupInterruptEnabled != 0); + + catchupInterruptEnabled = 0; + + return result; +} + +/* + * 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 + * there's still anything to do by then). + */ +static void +ProcessCatchupEvent(void) +{ + bool notify_enabled; + + /* Must prevent SIGUSR2 interrupt while I am running */ + notify_enabled = DisableNotifyInterrupt(); + + /* + * What we need to do here is cause ReceiveSharedInvalidMessages() + * to run, which will do the necessary work and also reset the + * catchupInterruptOccurred flag. If we are inside a transaction + * we can just call AcceptInvalidationMessages() to do this. If we + * aren't, we start and immediately end a transaction; the call to + * AcceptInvalidationMessages() happens down inside transaction start. + * + * It is awfully tempting to just call AcceptInvalidationMessages() + * without the rest of the xact start/stop overhead, and I think that + * would actually work in the normal case; but I am not sure that things + * would clean up nicely if we got an error partway through. + */ + if (IsTransactionOrTransactionBlock()) + { + elog(DEBUG4, "ProcessCatchupEvent inside transaction"); + AcceptInvalidationMessages(); + } + else + { + elog(DEBUG4, "ProcessCatchupEvent outside transaction"); + StartTransactionCommand(); + CommitTransactionCommand(); + } + + if (notify_enabled) + EnableNotifyInterrupt(); +} + + /****************************************************************************/ /* Functions that need to scan the PGPROC structures of all running backends. */ /* It's a bit strange to keep these in sinval.c, since they don't have any */ diff --git a/src/backend/storage/ipc/sinvaladt.c b/src/backend/storage/ipc/sinvaladt.c index a50cc4601e..de56b2d1b0 100644 --- a/src/backend/storage/ipc/sinvaladt.c +++ b/src/backend/storage/ipc/sinvaladt.c @@ -8,7 +8,7 @@ * * * IDENTIFICATION - * $PostgreSQL: pgsql/src/backend/storage/ipc/sinvaladt.c,v 1.54 2003/12/20 17:31:21 momjian Exp $ + * $PostgreSQL: pgsql/src/backend/storage/ipc/sinvaladt.c,v 1.55 2004/05/23 03:50:45 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -215,16 +215,12 @@ SIInsertDataEntry(SISeg *segP, SharedInvalidationMessage *data) /* * Try to prevent table overflow. When the table is 70% full send a * WAKEN_CHILDREN request to the postmaster. The postmaster will send - * a SIGUSR2 signal (ordinarily a NOTIFY signal) to all the backends. - * This will force idle backends to execute a transaction to look - * through pg_listener for NOTIFY messages, and as a byproduct of the - * transaction start they will read SI entries. + * a SIGUSR1 signal to all the backends, which will cause sinval.c + * to read any pending SI entries. * * This should never happen if all the backends are actively executing * queries, but if a backend is sitting idle then it won't be starting * transactions and so won't be reading SI entries. - * - * dz - 27 Jan 1998 */ if (numMsgs == (MAXNUMMESSAGES * 70 / 100) && IsUnderPostmaster) diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c index d097612ba3..604dd9819e 100644 --- a/src/backend/tcop/postgres.c +++ b/src/backend/tcop/postgres.c @@ -8,7 +8,7 @@ * * * IDENTIFICATION - * $PostgreSQL: pgsql/src/backend/tcop/postgres.c,v 1.413 2004/05/21 05:07:58 tgl Exp $ + * $PostgreSQL: pgsql/src/backend/tcop/postgres.c,v 1.414 2004/05/23 03:50:45 tgl Exp $ * * NOTES * this is the "main" module of the postgres backend and @@ -52,6 +52,7 @@ #include "storage/ipc.h" #include "storage/pg_shmem.h" #include "storage/proc.h" +#include "storage/sinval.h" #include "tcop/fastpath.h" #include "tcop/pquery.h" #include "tcop/tcopprot.h" @@ -1899,6 +1900,7 @@ die(SIGNAL_ARGS) /* until we are done getting ready for it */ InterruptHoldoffCount++; DisableNotifyInterrupt(); + DisableCatchupInterrupt(); /* Make sure CheckDeadLock won't run while shutting down... */ LockWaitCancel(); InterruptHoldoffCount--; @@ -1955,6 +1957,7 @@ StatementCancelHandler(SIGNAL_ARGS) if (LockWaitCancel()) { DisableNotifyInterrupt(); + DisableCatchupInterrupt(); InterruptHoldoffCount--; ProcessInterrupts(); } @@ -2006,6 +2009,7 @@ ProcessInterrupts(void) QueryCancelPending = false; /* ProcDie trumps QueryCancel */ ImmediateInterruptOK = false; /* not idle anymore */ DisableNotifyInterrupt(); + DisableCatchupInterrupt(); ereport(FATAL, (errcode(ERRCODE_ADMIN_SHUTDOWN), errmsg("terminating connection due to administrator command"))); @@ -2015,6 +2019,7 @@ ProcessInterrupts(void) QueryCancelPending = false; ImmediateInterruptOK = false; /* not idle anymore */ DisableNotifyInterrupt(); + DisableCatchupInterrupt(); ereport(ERROR, (errcode(ERRCODE_QUERY_CANCELED), errmsg("canceling query due to user request"))); @@ -2595,9 +2600,8 @@ PostgresMain(int argc, char *argv[], const char *username) * midst of output during who-knows-what operation... */ pqsignal(SIGPIPE, SIG_IGN); - pqsignal(SIGUSR1, SIG_IGN); /* this signal available for use */ - - pqsignal(SIGUSR2, Async_NotifyHandler); /* flush also sinval cache */ + pqsignal(SIGUSR1, CatchupInterruptHandler); + pqsignal(SIGUSR2, NotifyInterruptHandler); pqsignal(SIGFPE, FloatExceptionHandler); /* @@ -2761,6 +2765,7 @@ PostgresMain(int argc, char *argv[], const char *username) disable_sig_alarm(true); QueryCancelPending = false; /* again in case timeout occurred */ DisableNotifyInterrupt(); + DisableCatchupInterrupt(); debug_query_string = NULL; /* @@ -2879,6 +2884,7 @@ PostgresMain(int argc, char *argv[], const char *username) * signal */ EnableNotifyInterrupt(); + EnableCatchupInterrupt(); /* Allow "die" interrupt to be processed while waiting */ ImmediateInterruptOK = true; @@ -2901,6 +2907,7 @@ PostgresMain(int argc, char *argv[], const char *username) QueryCancelPending = false; /* forget any CANCEL signal */ DisableNotifyInterrupt(); + DisableCatchupInterrupt(); /* * (5) check for any other interesting events that happened while diff --git a/src/include/commands/async.h b/src/include/commands/async.h index bbfb24086e..6429895fbd 100644 --- a/src/include/commands/async.h +++ b/src/include/commands/async.h @@ -6,7 +6,7 @@ * Portions Copyright (c) 1996-2003, PostgreSQL Global Development Group * Portions Copyright (c) 1994, Regents of the University of California * - * $PostgreSQL: pgsql/src/include/commands/async.h,v 1.23 2003/11/29 22:40:59 pgsql Exp $ + * $PostgreSQL: pgsql/src/include/commands/async.h,v 1.24 2004/05/23 03:50:45 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -25,15 +25,14 @@ extern void AtCommit_Notify(void); extern void AtAbort_Notify(void); /* signal handler for inbound notifies (SIGUSR2) */ -extern void Async_NotifyHandler(SIGNAL_ARGS); +extern void NotifyInterruptHandler(SIGNAL_ARGS); /* * enable/disable processing of inbound notifies directly from signal handler. * The enable routine first performs processing of any inbound notifies that - * have occurred since the last disable. These are meant to be called ONLY - * from the appropriate places in PostgresMain(). + * have occurred since the last disable. */ extern void EnableNotifyInterrupt(void); -extern void DisableNotifyInterrupt(void); +extern bool DisableNotifyInterrupt(void); #endif /* ASYNC_H */ diff --git a/src/include/storage/pmsignal.h b/src/include/storage/pmsignal.h index e8ba72a253..688b83adab 100644 --- a/src/include/storage/pmsignal.h +++ b/src/include/storage/pmsignal.h @@ -7,7 +7,7 @@ * Portions Copyright (c) 1996-2003, PostgreSQL Global Development Group * Portions Copyright (c) 1994, Regents of the University of California * - * $PostgreSQL: pgsql/src/include/storage/pmsignal.h,v 1.6 2003/11/29 22:41:13 pgsql Exp $ + * $PostgreSQL: pgsql/src/include/storage/pmsignal.h,v 1.7 2004/05/23 03:50:45 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -24,7 +24,7 @@ typedef enum { PMSIGNAL_DO_CHECKPOINT, /* request to start a checkpoint */ PMSIGNAL_PASSWORD_CHANGE, /* pg_pwd file has changed */ - PMSIGNAL_WAKEN_CHILDREN, /* send a NOTIFY signal to all backends */ + PMSIGNAL_WAKEN_CHILDREN, /* send a SIGUSR1 signal to all backends */ NUM_PMSIGNALS /* Must be last value of enum! */ } PMSignalReason; diff --git a/src/include/storage/sinval.h b/src/include/storage/sinval.h index 84706272de..a1c731024c 100644 --- a/src/include/storage/sinval.h +++ b/src/include/storage/sinval.h @@ -7,7 +7,7 @@ * Portions Copyright (c) 1996-2003, PostgreSQL Global Development Group * Portions Copyright (c) 1994, Regents of the University of California * - * $PostgreSQL: pgsql/src/include/storage/sinval.h,v 1.33 2004/02/10 01:55:26 tgl Exp $ + * $PostgreSQL: pgsql/src/include/storage/sinval.h,v 1.34 2004/05/23 03:50:45 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -99,10 +99,19 @@ extern bool DatabaseHasActiveBackends(Oid databaseId, bool ignoreMyself); extern bool TransactionIdIsInProgress(TransactionId xid); extern TransactionId GetOldestXmin(bool allDbs); extern int CountActiveBackends(void); - +extern int CountEmptyBackendSlots(void); /* Use "struct PGPROC", not PGPROC, to avoid including proc.h here */ extern struct PGPROC *BackendIdGetProc(BackendId procId); -extern int CountEmptyBackendSlots(void); +/* signal handler for catchup events (SIGUSR1) */ +extern void CatchupInterruptHandler(SIGNAL_ARGS); + +/* + * enable/disable processing of catchup events directly from signal handler. + * The enable routine first performs processing of any catchup events that + * have occurred since the last disable. + */ +extern void EnableCatchupInterrupt(void); +extern bool DisableCatchupInterrupt(void); #endif /* SINVAL_H */