Skip to content

Commit ebfc56d

Browse files
tglsfdctglsfdc
committed
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.
1 parent 4d86ae4 commit ebfc56d

File tree

8 files changed

+289
-37
lines changed

8 files changed

+289
-37
lines changed

‎src/backend/commands/async.c

Lines changed: 23 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77
* Portions Copyright (c) 1994, Regents of the University of California
88
*
99
* IDENTIFICATION
10-
* $PostgreSQL: pgsql/src/backend/commands/async.c,v 1.110 2004/05/22 21:58:24 tgl Exp $
10+
* $PostgreSQL: pgsql/src/backend/commands/async.c,v 1.111 2004/05/23 03:50:45 tgl Exp $
1111
*
1212
*-------------------------------------------------------------------------
1313
*/
@@ -86,6 +86,7 @@
8686
#include "libpq/pqformat.h"
8787
#include "miscadmin.h"
8888
#include "storage/ipc.h"
89+
#include "storage/sinval.h"
8990
#include "tcop/tcopprot.h"
9091
#include "utils/fmgroids.h"
9192
#include "utils/ps_status.h"
@@ -607,7 +608,7 @@ AtAbort_Notify(void)
607608

608609
/*
609610
*--------------------------------------------------------------
610-
* Async_NotifyHandler
611+
* NotifyInterruptHandler
611612
*
612613
* This is the signal handler for SIGUSR2.
613614
*
@@ -623,7 +624,7 @@ AtAbort_Notify(void)
623624
*--------------------------------------------------------------
624625
*/
625626
void
626-
Async_NotifyHandler(SIGNAL_ARGS)
627+
NotifyInterruptHandler(SIGNAL_ARGS)
627628
{
628629
int save_errno = errno;
629630

@@ -669,12 +670,12 @@ Async_NotifyHandler(SIGNAL_ARGS)
669670
{
670671
/* Here, it is finally safe to do stuff. */
671672
if (Trace_notify)
672-
elog(DEBUG1, "Async_NotifyHandler: perform async notify");
673+
elog(DEBUG1, "NotifyInterruptHandler: perform async notify");
673674

674675
ProcessIncomingNotify();
675676

676677
if (Trace_notify)
677-
elog(DEBUG1, "Async_NotifyHandler: done");
678+
elog(DEBUG1, "NotifyInterruptHandler: done");
678679
}
679680
}
680681

@@ -766,12 +767,20 @@ EnableNotifyInterrupt(void)
766767
* This is called by the PostgresMain main loop just after receiving
767768
* a frontend command. Signal handler execution of inbound notifies
768769
* is disabled until the next EnableNotifyInterrupt call.
770+
*
771+
* The SIGUSR1 signal handler also needs to call this, so as to
772+
* prevent conflicts if one signal interrupts the other. So we
773+
* must return the previous state of the flag.
769774
* --------------------------------------------------------------
770775
*/
771-
void
776+
bool
772777
DisableNotifyInterrupt(void)
773778
{
779+
bool result = (notifyInterruptEnabled != 0);
780+
774781
notifyInterruptEnabled = 0;
782+
783+
return result;
775784
}
776785

777786
/*
@@ -785,10 +794,6 @@ DisableNotifyInterrupt(void)
785794
* and clear the notification field in pg_listener until next time.
786795
*
787796
* NOTE: since we are outside any transaction, we must create our own.
788-
*
789-
* Results:
790-
* XXX
791-
*
792797
* --------------------------------------------------------------
793798
*/
794799
static void
@@ -803,11 +808,15 @@ ProcessIncomingNotify(void)
803808
Datum value[Natts_pg_listener];
804809
char repl[Natts_pg_listener],
805810
nulls[Natts_pg_listener];
811+
bool catchup_enabled;
812+
813+
/* Must prevent SIGUSR1 interrupt while I am running */
814+
catchup_enabled = DisableCatchupInterrupt();
806815

807816
if (Trace_notify)
808817
elog(DEBUG1, "ProcessIncomingNotify");
809818

810-
set_ps_display("async_notify");
819+
set_ps_display("notify interrupt");
811820

812821
notifyInterruptOccurred = 0;
813822

@@ -883,6 +892,9 @@ ProcessIncomingNotify(void)
883892

884893
if (Trace_notify)
885894
elog(DEBUG1, "ProcessIncomingNotify: done");
895+
896+
if (catchup_enabled)
897+
EnableCatchupInterrupt();
886898
}
887899

888900
/*

‎src/backend/postmaster/postmaster.c

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@
3737
*
3838
*
3939
* IDENTIFICATION
40-
* $PostgreSQL: pgsql/src/backend/postmaster/postmaster.c,v 1.393 2004/05/21 05:07:57 tgl Exp $
40+
* $PostgreSQL: pgsql/src/backend/postmaster/postmaster.c,v 1.394 2004/05/23 03:50:45 tgl Exp $
4141
*
4242
* NOTES
4343
*
@@ -2861,11 +2861,11 @@ sigusr1_handler(SIGNAL_ARGS)
28612861
if (CheckPostmasterSignal(PMSIGNAL_WAKEN_CHILDREN))
28622862
{
28632863
/*
2864-
* Send SIGUSR2 to all children (triggers AsyncNotifyHandler). See
2865-
* storage/ipc/sinvaladt.c for the use of this.
2864+
* Send SIGUSR1 to all children (triggers CatchupInterruptHandler).
2865+
* See storage/ipc/sinval[adt].c for the use of this.
28662866
*/
28672867
if (Shutdown == NoShutdown)
2868-
SignalChildren(SIGUSR2);
2868+
SignalChildren(SIGUSR1);
28692869
}
28702870

28712871
PG_SETMASK(&UnBlockSig);

‎src/backend/storage/ipc/sinval.c

Lines changed: 230 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,20 +8,46 @@
88
*
99
*
1010
* IDENTIFICATION
11-
* $PostgreSQL: pgsql/src/backend/storage/ipc/sinval.c,v 1.62 2003/11/29 19:51:56 pgsql Exp $
11+
* $PostgreSQL: pgsql/src/backend/storage/ipc/sinval.c,v 1.63 2004/05/23 03:50:45 tgl Exp $
1212
*
1313
*-------------------------------------------------------------------------
1414
*/
1515
#include "postgres.h"
1616

17+
#include <signal.h>
1718

19+
#include "commands/async.h"
20+
#include "storage/ipc.h"
1821
#include "storage/proc.h"
1922
#include "storage/sinval.h"
2023
#include "storage/sinvaladt.h"
24+
#include "utils/inval.h"
2125
#include "utils/tqual.h"
2226
#include "miscadmin.h"
2327

2428

29+
/*
30+
* Because backends sitting idle will not be reading sinval events, we
31+
* need a way to give an idle backend a swift kick in the rear and make
32+
* it catch up before the sinval queue overflows and forces everyone
33+
* through a cache reset exercise. This is done by broadcasting SIGUSR1
34+
* to all backends when the queue is threatening to become full.
35+
*
36+
* State for catchup events consists of two flags: one saying whether
37+
* the signal handler is currently allowed to call ProcessCatchupEvent
38+
* directly, and one saying whether the signal has occurred but the handler
39+
* was not allowed to call ProcessCatchupEvent at the time.
40+
*
41+
* NB: the "volatile" on these declarations is critical! If your compiler
42+
* does not grok "volatile", you'd be best advised to compile this file
43+
* with all optimization turned off.
44+
*/
45+
static volatile int catchupInterruptEnabled = 0;
46+
static volatile int catchupInterruptOccurred = 0;
47+
48+
static void ProcessCatchupEvent(void);
49+
50+
2551
/****************************************************************************/
2652
/* CreateSharedInvalidationState() Initialize SI buffer */
2753
/* */
@@ -91,6 +117,12 @@ ReceiveSharedInvalidMessages(
91117

92118
for (;;)
93119
{
120+
/*
121+
* We can discard any pending catchup event, since we will not exit
122+
* this loop until we're fully caught up.
123+
*/
124+
catchupInterruptOccurred = 0;
125+
94126
/*
95127
* We can run SIGetDataEntry in parallel with other backends
96128
* running SIGetDataEntry for themselves, since each instance will
@@ -137,6 +169,203 @@ ReceiveSharedInvalidMessages(
137169
}
138170

139171

172+
/*
173+
* CatchupInterruptHandler
174+
*
175+
* This is the signal handler for SIGUSR1.
176+
*
177+
* If we are idle (catchupInterruptEnabled is set), we can safely
178+
* invoke ProcessCatchupEvent directly. Otherwise, just set a flag
179+
* to do it later. (Note that it's quite possible for normal processing
180+
* of the current transaction to cause ReceiveSharedInvalidMessages()
181+
* to be run later on; in that case the flag will get cleared again,
182+
* since there's no longer any reason to do anything.)
183+
*/
184+
void
185+
CatchupInterruptHandler(SIGNAL_ARGS)
186+
{
187+
int save_errno = errno;
188+
189+
/*
190+
* Note: this is a SIGNAL HANDLER. You must be very wary what you do
191+
* here.
192+
*/
193+
194+
/* Don't joggle the elbow of proc_exit */
195+
if (proc_exit_inprogress)
196+
return;
197+
198+
if (catchupInterruptEnabled)
199+
{
200+
bool save_ImmediateInterruptOK = ImmediateInterruptOK;
201+
202+
/*
203+
* We may be called while ImmediateInterruptOK is true; turn it
204+
* off while messing with the catchup state. (We would have to
205+
* save and restore it anyway, because PGSemaphore operations
206+
* inside ProcessCatchupEvent() might reset it.)
207+
*/
208+
ImmediateInterruptOK = false;
209+
210+
/*
211+
* I'm not sure whether some flavors of Unix might allow another
212+
* SIGUSR1 occurrence to recursively interrupt this routine. To
213+
* cope with the possibility, we do the same sort of dance that
214+
* EnableCatchupInterrupt must do --- see that routine for
215+
* comments.
216+
*/
217+
catchupInterruptEnabled = 0; /* disable any recursive signal */
218+
catchupInterruptOccurred = 1; /* do at least one iteration */
219+
for (;;)
220+
{
221+
catchupInterruptEnabled = 1;
222+
if (!catchupInterruptOccurred)
223+
break;
224+
catchupInterruptEnabled = 0;
225+
if (catchupInterruptOccurred)
226+
{
227+
/* Here, it is finally safe to do stuff. */
228+
ProcessCatchupEvent();
229+
}
230+
}
231+
232+
/*
233+
* Restore ImmediateInterruptOK, and check for interrupts if
234+
* needed.
235+
*/
236+
ImmediateInterruptOK = save_ImmediateInterruptOK;
237+
if (save_ImmediateInterruptOK)
238+
CHECK_FOR_INTERRUPTS();
239+
}
240+
else
241+
{
242+
/*
243+
* In this path it is NOT SAFE to do much of anything, except
244+
* this:
245+
*/
246+
catchupInterruptOccurred = 1;
247+
}
248+
249+
errno = save_errno;
250+
}
251+
252+
/*
253+
* EnableCatchupInterrupt
254+
*
255+
* This is called by the PostgresMain main loop just before waiting
256+
* for a frontend command. We process any pending catchup events,
257+
* and enable the signal handler to process future events directly.
258+
*
259+
* NOTE: the signal handler starts out disabled, and stays so until
260+
* PostgresMain calls this the first time.
261+
*/
262+
void
263+
EnableCatchupInterrupt(void)
264+
{
265+
/*
266+
* This code is tricky because we are communicating with a signal
267+
* handler that could interrupt us at any point. If we just checked
268+
* catchupInterruptOccurred and then set catchupInterruptEnabled, we
269+
* could fail to respond promptly to a signal that happens in between
270+
* those two steps. (A very small time window, perhaps, but Murphy's
271+
* Law says you can hit it...) Instead, we first set the enable flag,
272+
* then test the occurred flag. If we see an unserviced interrupt has
273+
* occurred, we re-clear the enable flag before going off to do the
274+
* service work. (That prevents re-entrant invocation of
275+
* ProcessCatchupEvent() if another interrupt occurs.) If an
276+
* interrupt comes in between the setting and clearing of
277+
* catchupInterruptEnabled, then it will have done the service work and
278+
* left catchupInterruptOccurred zero, so we have to check again after
279+
* clearing enable. The whole thing has to be in a loop in case
280+
* another interrupt occurs while we're servicing the first. Once we
281+
* get out of the loop, enable is set and we know there is no
282+
* unserviced interrupt.
283+
*
284+
* NB: an overenthusiastic optimizing compiler could easily break this
285+
* code. Hopefully, they all understand what "volatile" means these
286+
* days.
287+
*/
288+
for (;;)
289+
{
290+
catchupInterruptEnabled = 1;
291+
if (!catchupInterruptOccurred)
292+
break;
293+
catchupInterruptEnabled = 0;
294+
if (catchupInterruptOccurred)
295+
{
296+
ProcessCatchupEvent();
297+
}
298+
}
299+
}
300+
301+
/*
302+
* DisableCatchupInterrupt
303+
*
304+
* This is called by the PostgresMain main loop just after receiving
305+
* a frontend command. Signal handler execution of catchup events
306+
* is disabled until the next EnableCatchupInterrupt call.
307+
*
308+
* The SIGUSR2 signal handler also needs to call this, so as to
309+
* prevent conflicts if one signal interrupts the other. So we
310+
* must return the previous state of the flag.
311+
*/
312+
bool
313+
DisableCatchupInterrupt(void)
314+
{
315+
bool result = (catchupInterruptEnabled != 0);
316+
317+
catchupInterruptEnabled = 0;
318+
319+
return result;
320+
}
321+
322+
/*
323+
* ProcessCatchupEvent
324+
*
325+
* Respond to a catchup event (SIGUSR1) from another backend.
326+
*
327+
* This is called either directly from the SIGUSR1 signal handler,
328+
* or the next time control reaches the outer idle loop (assuming
329+
* there's still anything to do by then).
330+
*/
331+
static void
332+
ProcessCatchupEvent(void)
333+
{
334+
bool notify_enabled;
335+
336+
/* Must prevent SIGUSR2 interrupt while I am running */
337+
notify_enabled = DisableNotifyInterrupt();
338+
339+
/*
340+
* What we need to do here is cause ReceiveSharedInvalidMessages()
341+
* to run, which will do the necessary work and also reset the
342+
* catchupInterruptOccurred flag. If we are inside a transaction
343+
* we can just call AcceptInvalidationMessages() to do this. If we
344+
* aren't, we start and immediately end a transaction; the call to
345+
* AcceptInvalidationMessages() happens down inside transaction start.
346+
*
347+
* It is awfully tempting to just call AcceptInvalidationMessages()
348+
* without the rest of the xact start/stop overhead, and I think that
349+
* would actually work in the normal case; but I am not sure that things
350+
* would clean up nicely if we got an error partway through.
351+
*/
352+
if (IsTransactionOrTransactionBlock())
353+
{
354+
elog(DEBUG4, "ProcessCatchupEvent inside transaction");
355+
AcceptInvalidationMessages();
356+
}
357+
else
358+
{
359+
elog(DEBUG4, "ProcessCatchupEvent outside transaction");
360+
StartTransactionCommand();
361+
CommitTransactionCommand();
362+
}
363+
364+
if (notify_enabled)
365+
EnableNotifyInterrupt();
366+
}
367+
368+
140369
/****************************************************************************/
141370
/* Functions that need to scan the PGPROC structures of all running backends. */
142371
/* It's a bit strange to keep these in sinval.c, since they don't have any */

0 commit comments

Comments
 (0)