Skip to content

Commit db0f6ca

Browse files
robertmhaasrobertmhaas
committed
Remove set_latch_on_sigusr1 flag.
This flag has proven to be a recipe for bugs, and it doesn't seem like it can really buy anything in terms of performance. So let's just *always* set the process latch when we receive SIGUSR1 instead of trying to do it only when needed. Per my recent proposal on pgsql-hackers.
1 parent b7aac36 commit db0f6ca

File tree

6 files changed

+86
-153
lines changed

6 files changed

+86
-153
lines changed

‎src/backend/postmaster/bgworker.c

+27-55
Original file line numberDiff line numberDiff line change
@@ -954,45 +954,31 @@ WaitForBackgroundWorkerStartup(BackgroundWorkerHandle *handle, pid_t *pidp)
954954
{
955955
BgwHandleStatus status;
956956
int rc;
957-
bool save_set_latch_on_sigusr1;
958957

959-
save_set_latch_on_sigusr1 = set_latch_on_sigusr1;
960-
set_latch_on_sigusr1 = true;
961-
962-
PG_TRY();
958+
for (;;)
963959
{
964-
for (;;)
965-
{
966-
pid_t pid;
960+
pid_t pid;
967961

968-
CHECK_FOR_INTERRUPTS();
962+
CHECK_FOR_INTERRUPTS();
969963

970-
status = GetBackgroundWorkerPid(handle, &pid);
971-
if (status == BGWH_STARTED)
972-
*pidp = pid;
973-
if (status != BGWH_NOT_YET_STARTED)
974-
break;
975-
976-
rc = WaitLatch(MyLatch,
977-
WL_LATCH_SET | WL_POSTMASTER_DEATH, 0);
964+
status = GetBackgroundWorkerPid(handle, &pid);
965+
if (status == BGWH_STARTED)
966+
*pidp = pid;
967+
if (status != BGWH_NOT_YET_STARTED)
968+
break;
978969

979-
if (rc & WL_POSTMASTER_DEATH)
980-
{
981-
status = BGWH_POSTMASTER_DIED;
982-
break;
983-
}
970+
rc = WaitLatch(MyLatch,
971+
WL_LATCH_SET | WL_POSTMASTER_DEATH, 0);
984972

985-
ResetLatch(MyLatch);
973+
if (rc & WL_POSTMASTER_DEATH)
974+
{
975+
status = BGWH_POSTMASTER_DIED;
976+
break;
986977
}
978+
979+
ResetLatch(MyLatch);
987980
}
988-
PG_CATCH();
989-
{
990-
set_latch_on_sigusr1 = save_set_latch_on_sigusr1;
991-
PG_RE_THROW();
992-
}
993-
PG_END_TRY();
994981

995-
set_latch_on_sigusr1 = save_set_latch_on_sigusr1;
996982
return status;
997983
}
998984

@@ -1009,40 +995,26 @@ WaitForBackgroundWorkerShutdown(BackgroundWorkerHandle *handle)
1009995
{
1010996
BgwHandleStatus status;
1011997
int rc;
1012-
bool save_set_latch_on_sigusr1;
1013-
1014-
save_set_latch_on_sigusr1 = set_latch_on_sigusr1;
1015-
set_latch_on_sigusr1 = true;
1016998

1017-
PG_TRY();
999+
for (;;)
10181000
{
1019-
for (;;)
1020-
{
1021-
pid_t pid;
1001+
pid_t pid;
10221002

1023-
CHECK_FOR_INTERRUPTS();
1003+
CHECK_FOR_INTERRUPTS();
10241004

1025-
status = GetBackgroundWorkerPid(handle, &pid);
1026-
if (status == BGWH_STOPPED)
1027-
return status;
1005+
status = GetBackgroundWorkerPid(handle, &pid);
1006+
if (status == BGWH_STOPPED)
1007+
return status;
10281008

1029-
rc = WaitLatch(&MyProc->procLatch,
1030-
WL_LATCH_SET | WL_POSTMASTER_DEATH, 0);
1009+
rc = WaitLatch(&MyProc->procLatch,
1010+
WL_LATCH_SET | WL_POSTMASTER_DEATH, 0);
10311011

1032-
if (rc & WL_POSTMASTER_DEATH)
1033-
return BGWH_POSTMASTER_DIED;
1012+
if (rc & WL_POSTMASTER_DEATH)
1013+
return BGWH_POSTMASTER_DIED;
10341014

1035-
ResetLatch(&MyProc->procLatch);
1036-
}
1037-
}
1038-
PG_CATCH();
1039-
{
1040-
set_latch_on_sigusr1 = save_set_latch_on_sigusr1;
1041-
PG_RE_THROW();
1015+
ResetLatch(&MyProc->procLatch);
10421016
}
1043-
PG_END_TRY();
10441017

1045-
set_latch_on_sigusr1 = save_set_latch_on_sigusr1;
10461018
return status;
10471019
}
10481020

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

+1-10
Original file line numberDiff line numberDiff line change
@@ -59,14 +59,6 @@ typedef struct
5959
*/
6060
#define NumProcSignalSlots (MaxBackends + NUM_AUXPROCTYPES)
6161

62-
/*
63-
* If this flag is set, the process latch will be set whenever SIGUSR1
64-
* is received. This is useful when waiting for a signal from the postmaster.
65-
* Spurious wakeups must be expected. Make sure that the flag is cleared
66-
* in the error path.
67-
*/
68-
bool set_latch_on_sigusr1;
69-
7062
static ProcSignalSlot *ProcSignalSlots = NULL;
7163
static volatile ProcSignalSlot *MyProcSignalSlot = NULL;
7264

@@ -296,8 +288,7 @@ procsignal_sigusr1_handler(SIGNAL_ARGS)
296288
if (CheckProcSignal(PROCSIG_RECOVERY_CONFLICT_BUFFERPIN))
297289
RecoveryConflictInterrupt(PROCSIG_RECOVERY_CONFLICT_BUFFERPIN);
298290

299-
if (set_latch_on_sigusr1)
300-
SetLatch(MyLatch);
291+
SetLatch(MyLatch);
301292

302293
latch_sigusr1_handler();
303294

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

+31-45
Original file line numberDiff line numberDiff line change
@@ -962,63 +962,49 @@ static bool
962962
shm_mq_wait_internal(volatile shm_mq *mq, PGPROC *volatile * ptr,
963963
BackgroundWorkerHandle *handle)
964964
{
965-
bool save_set_latch_on_sigusr1;
966965
bool result = false;
967966

968-
save_set_latch_on_sigusr1 = set_latch_on_sigusr1;
969-
if (handle != NULL)
970-
set_latch_on_sigusr1 = true;
971-
972-
PG_TRY();
967+
for (;;)
973968
{
974-
for (;;)
969+
BgwHandleStatus status;
970+
pid_t pid;
971+
bool detached;
972+
973+
/* Acquire the lock just long enough to check the pointer. */
974+
SpinLockAcquire(&mq->mq_mutex);
975+
detached = mq->mq_detached;
976+
result = (*ptr != NULL);
977+
SpinLockRelease(&mq->mq_mutex);
978+
979+
/* Fail if detached; else succeed if initialized. */
980+
if (detached)
981+
{
982+
result = false;
983+
break;
984+
}
985+
if (result)
986+
break;
987+
988+
if (handle != NULL)
975989
{
976-
BgwHandleStatus status;
977-
pid_t pid;
978-
bool detached;
979-
980-
/* Acquire the lock just long enough to check the pointer. */
981-
SpinLockAcquire(&mq->mq_mutex);
982-
detached = mq->mq_detached;
983-
result = (*ptr != NULL);
984-
SpinLockRelease(&mq->mq_mutex);
985-
986-
/* Fail if detached; else succeed if initialized. */
987-
if (detached)
990+
/* Check for unexpected worker death. */
991+
status = GetBackgroundWorkerPid(handle, &pid);
992+
if (status != BGWH_STARTED && status != BGWH_NOT_YET_STARTED)
988993
{
989994
result = false;
990995
break;
991996
}
992-
if (result)
993-
break;
994-
995-
if (handle != NULL)
996-
{
997-
/* Check for unexpected worker death. */
998-
status = GetBackgroundWorkerPid(handle, &pid);
999-
if (status != BGWH_STARTED && status != BGWH_NOT_YET_STARTED)
1000-
{
1001-
result = false;
1002-
break;
1003-
}
1004-
}
997+
}
1005998

1006-
/* Wait to be signalled. */
1007-
WaitLatch(MyLatch, WL_LATCH_SET, 0);
999+
/* Wait to be signalled. */
1000+
WaitLatch(MyLatch, WL_LATCH_SET, 0);
10081001

1009-
/* An interrupt may have occurred while we were waiting. */
1010-
CHECK_FOR_INTERRUPTS();
1002+
/* An interrupt may have occurred while we were waiting. */
1003+
CHECK_FOR_INTERRUPTS();
10111004

1012-
/* Reset the latch so we don't spin. */
1013-
ResetLatch(MyLatch);
1014-
}
1015-
}
1016-
PG_CATCH();
1017-
{
1018-
set_latch_on_sigusr1 = save_set_latch_on_sigusr1;
1019-
PG_RE_THROW();
1005+
/* Reset the latch so we don't spin. */
1006+
ResetLatch(MyLatch);
10201007
}
1021-
PG_END_TRY();
10221008

10231009
return result;
10241010
}

‎src/backend/tcop/postgres.c

+1-3
Original file line numberDiff line numberDiff line change
@@ -2825,9 +2825,7 @@ RecoveryConflictInterrupt(ProcSignalReason reason)
28252825
/*
28262826
* Set the process latch. This function essentially emulates signal
28272827
* handlers like die() and StatementCancelHandler() and it seems prudent
2828-
* to behave similarly as they do. Alternatively all plain backend code
2829-
* waiting on that latch, expecting to get interrupted by query cancels et
2830-
* al., would also need to set set_latch_on_sigusr1.
2828+
* to behave similarly as they do.
28312829
*/
28322830
SetLatch(MyLatch);
28332831

‎src/include/storage/procsignal.h

-1
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,5 @@ extern int SendProcSignal(pid_t pid, ProcSignalReason reason,
5555
BackendId backendId);
5656

5757
extern void procsignal_sigusr1_handler(SIGNAL_ARGS);
58-
extern PGDLLIMPORT bool set_latch_on_sigusr1;
5958

6059
#endif /* PROCSIGNAL_H */

‎src/test/modules/test_shm_mq/setup.c

+26-39
Original file line numberDiff line numberDiff line change
@@ -255,51 +255,38 @@ static void
255255
wait_for_workers_to_become_ready(worker_state *wstate,
256256
volatile test_shm_mq_header *hdr)
257257
{
258-
bool save_set_latch_on_sigusr1;
259258
bool result = false;
260259

261-
save_set_latch_on_sigusr1 = set_latch_on_sigusr1;
262-
set_latch_on_sigusr1 = true;
263-
264-
PG_TRY();
260+
for (;;)
265261
{
266-
for (;;)
262+
int workers_ready;
263+
264+
/* If all the workers are ready, we have succeeded. */
265+
SpinLockAcquire(&hdr->mutex);
266+
workers_ready = hdr->workers_ready;
267+
SpinLockRelease(&hdr->mutex);
268+
if (workers_ready >= wstate->nworkers)
267269
{
268-
int workers_ready;
269-
270-
/* If all the workers are ready, we have succeeded. */
271-
SpinLockAcquire(&hdr->mutex);
272-
workers_ready = hdr->workers_ready;
273-
SpinLockRelease(&hdr->mutex);
274-
if (workers_ready >= wstate->nworkers)
275-
{
276-
result = true;
277-
break;
278-
}
279-
280-
/* If any workers (or the postmaster) have died, we have failed. */
281-
if (!check_worker_status(wstate))
282-
{
283-
result = false;
284-
break;
285-
}
286-
287-
/* Wait to be signalled. */
288-
WaitLatch(MyLatch, WL_LATCH_SET, 0);
289-
290-
/* An interrupt may have occurred while we were waiting. */
291-
CHECK_FOR_INTERRUPTS();
292-
293-
/* Reset the latch so we don't spin. */
294-
ResetLatch(MyLatch);
270+
result = true;
271+
break;
295272
}
273+
274+
/* If any workers (or the postmaster) have died, we have failed. */
275+
if (!check_worker_status(wstate))
276+
{
277+
result = false;
278+
break;
279+
}
280+
281+
/* Wait to be signalled. */
282+
WaitLatch(MyLatch, WL_LATCH_SET, 0);
283+
284+
/* An interrupt may have occurred while we were waiting. */
285+
CHECK_FOR_INTERRUPTS();
286+
287+
/* Reset the latch so we don't spin. */
288+
ResetLatch(MyLatch);
296289
}
297-
PG_CATCH();
298-
{
299-
set_latch_on_sigusr1 = save_set_latch_on_sigusr1;
300-
PG_RE_THROW();
301-
}
302-
PG_END_TRY();
303290

304291
if (!result)
305292
ereport(ERROR,

0 commit comments

Comments
 (0)