8000 Prevent excessive delays before launching new logrep workers. · postgres/postgres@87c8ed3 · GitHub
[go: up one dir, main page]

Skip to content

Commit 87c8ed3

Browse files
committed
Prevent excessive delays before launching new logrep workers.
The logical replication launcher process would sometimes sleep for as much as 3 minutes before noticing that it is supposed to launch a new worker. This could happen if (1) WaitForReplicationWorkerAttach absorbed a process latch wakeup that was meant to cause ApplyLauncherMain to do work, or (2) logicalrep_worker_launch reported failure, either because of resource limits or because the new worker terminated immediately. In case (2), the expected behavior is that we retry the launch after wal_retrieve_retry_interval, but that didn't reliably happen. It's not clear how often such conditions would occur in the field, but in our subscription test suite they are somewhat common, especially in tests that exercise cases that cause quick worker failure. That causes the tests to take substantially longer than they ought to do on typical setups. To fix (1), make WaitForReplicationWorkerAttach re-set the latch before returning if it cleared it while looping. To fix (2), ensure that we reduce wait_time to no more than wal_retrieve_retry_interval when logicalrep_worker_launch reports failure. In passing, fix a couple of perhaps-hypothetical race conditions, e.g. examining worker->in_use without a lock. Backpatch to v16. Problem (2) didn't exist before commit 5a3a953 because the previous code always set wait_time to wal_retrieve_retry_interval when launching a worker, regardless of success or failure of the launch. That behavior also greatly mitigated problem (1), so I'm not excited about adapting the remainder of the patch to the substantially-different code in older branches. Author: Tom Lane <tgl@sss.pgh.pa.us> Reviewed-by: Amit Kapila <amit.kapila16@gmail.com> Reviewed-by: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> Discussion: https://postgr.es/m/817604.1750723007@sss.pgh.pa.us Backpatch-through: 16
1 parent abb487a commit 87c8ed3

File tree

2 files changed

+42
-13
lines changed

2 files changed

+42
-13
lines changed

src/backend/replication/logical/launcher.c

Lines changed: 31 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -190,12 +190,14 @@ WaitForReplicationWorkerAttach(LogicalRepWorker *worker,
190190
uint16 generation,
191191
BackgroundWorkerHandle *handle)
192192
{
193-
BgwHandleStatus status;
194-
int rc;
193+
bool result = false;
194+
bool dropped_latch = false;
195195

196196
for (;;)
197197
{
198+
BgwHandleStatus status;
198199
pid_t pid;
200+
int rc;
199201

200202
CHECK_FOR_INTERRUPTS();
201203

@@ -204,8 +206,9 @@ WaitForReplicationWorkerAttach(LogicalRepWorker *worker,
204206
/* Worker either died or has started. Return false if died. */
205207
if (!worker->in_use || worker->proc)
206208
{
209+
result = worker->in_use;
207210
LWLockRelease(LogicalRepWorkerLock);
208-
return worker->in_use;
211+
break;
209212
}
210213

211214
LWLockRelease(LogicalRepWorkerLock);
@@ -220,7 +223,7 @@ WaitForReplicationWorkerAttach(LogicalRepWorker *worker,
220223
if (generation == worker->generation)
221224
logicalrep_worker_cleanup(worker);
222225
LWLockRelease(LogicalRepWorkerLock);
223-
return false;
226+
break; /* result is already false */
224227
}
225228

226229
/*
@@ -235,8 +238,18 @@ WaitForReplicationWorkerAttach(LogicalRepWorker *worker,
235238
{
236239
ResetLatch(MyLatch);
237240
CHECK_FOR_INTERRUPTS();
241+
dropped_latch = true;
238242
}
239243
}
244+
245+
/*
246+
* If we had to clear a latch event in order to wait, be sure to restore
247+
* it before exiting. Otherwise caller may miss events.
248+
*/
249+
if (dropped_latch)
250+
SetLatch(MyLatch);
251+
252+
return result;
240253
}
241254

242255
/*
@@ -1174,9 +1187,20 @@ ApplyLauncherMain(Datum main_arg)
11741187
(elapsed = TimestampDifferenceMilliseconds(last_start, now)) >= wal_retrieve_retry_interval)
11751188
{
11761189
ApplyLauncherSetWorkerStartTime(sub->oid, now);
1177-
logicalrep_worker_launch(sub->dbid, sub->oid, sub->name,
1178-
sub->owner, InvalidOid,
1179-
DSM_HANDLE_INVALID);
1190+
if (!logicalrep_worker_launch(sub->dbid, sub->oid, sub->name,
1191+
sub->owner, InvalidOid,
1192+
DSM_HANDLE_INVALID))
1193+
{
1194+
/*
1195+
* We get here either if we failed to launch a worker
1196+
* (perhaps for resource-exhaustion reasons) or if we
1197+
* launched one but it immediately quit. Either way, it
1198+
* seems appropriate to try again after
1199+
* wal_retrieve_retry_interval.
1200+
*/
1201+
wait_time = Min(wait_time,
1202+
wal_retrieve_retry_interval);
1203+
}
11801204
}
11811205
else
11821206
{

src/backend/replication/logical/tablesync.c

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -603,13 +603,18 @@ process_syncing_tables_for_apply(XLogRecPtr current_lsn)
603603
TimestampDifferenceExceeds(hentry->last_start_time, now,
604604
wal_retrieve_retry_interval))
605605
{
606-
logicalrep_worker_launch(MyLogicalRepWorker->dbid,
607-
MySubscription->oid,
608-
MySubscription->name,
609-
MyLogicalRepWorker->userid,
610-
rstate->relid,
611-
DSM_HANDLE_INVALID);
606+
/*
607+
* Set the last_start_time even if we fail to start
608+
* the worker, so that we won't retry until
609+
* wal_retrieve_retry_interval has elapsed.
610+
*/
612611
hentry->last_start_time = now;
612+
(void) logicalrep_worker_launch(MyLogicalRepWorker->dbid,
613+
MySubscription->oid,
614+
MySubscription->name,
615+
MyLogicalRepWorker->userid,
616+
rstate->relid,
617+
DSM_HANDLE_INVALID);
613618
}
614619
}
615620
}

0 commit comments

Comments
 (0)
0