Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
Skip to content

Commit f88bd31

Browse files
committed
Don't call palloc() while holding a spinlock, either.
Fix some more violations of the "only straight-line code inside a spinlock" rule. These are hazardous not only because they risk holding the lock for an excessively long time, but because it's possible for palloc to throw elog(ERROR), leaving a stuck spinlock behind. copy_replication_slot() had two separate places that did pallocs while holding a spinlock. We can make the code simpler and safer by copying the whole ReplicationSlot struct into a local variable while holding the spinlock, and then referencing that copy. (While that's arguably more cycles than we really need to spend holding the lock, the struct isn't all that big, and this way seems far more maintainable than copying fields piecemeal. Anyway this is surely much cheaper than a palloc.) That bug goes back to v12. InvalidateObsoleteReplicationSlots() not only did a palloc while holding a spinlock, but for extra sloppiness then leaked the memory --- probably for the lifetime of the checkpointer process, though I didn't try to verify that. Fortunately that silliness is new in HEAD. pg_get_replication_slots() had a cosmetic violation of the rule, in that it only assumed it's safe to call namecpy() while holding a spinlock. Still, that's a hazard waiting to bite somebody, and there were some other cosmetic coding-rule violations in the same function, so clean it up. I back-patched this as far as v10; the code exists before that but it looks different, and this didn't seem important enough to adapt the patch further back. Discussion: https://postgr.es/m/20200602.161518.1399689010416646074.horikyota.ntt@gmail.com
1 parent 4d685f6 commit f88bd31

File tree

3 files changed

+57
-61
lines changed

3 files changed

+57
-61
lines changed

src/backend/replication/slot.c

+6-5
Original file line numberDiff line numberDiff line change
@@ -1099,7 +1099,7 @@ InvalidateObsoleteReplicationSlots(XLogSegNo oldestSegno)
10991099
{
11001100
ReplicationSlot *s = &ReplicationSlotCtl->replication_slots[i];
11011101
XLogRecPtr restart_lsn = InvalidXLogRecPtr;
1102-
char *slotname;
1102+
NameData slotname;
11031103

11041104
if (!s->in_use)
11051105
continue;
@@ -1112,23 +1112,24 @@ InvalidateObsoleteReplicationSlots(XLogSegNo oldestSegno)
11121112
continue;
11131113
}
11141114

1115-
slotname = pstrdup(NameStr(s->data.name));
1115+
slotname = s->data.name;
11161116
restart_lsn = s->data.restart_lsn;
11171117

11181118
SpinLockRelease(&s->mutex);
11191119
LWLockRelease(ReplicationSlotControlLock);
11201120

11211121
for (;;)
11221122
{
1123-
int wspid = ReplicationSlotAcquire(slotname, SAB_Inquire);
1123+
int wspid = ReplicationSlotAcquire(NameStr(slotname),
1124+
SAB_Inquire);
11241125

11251126
/* no walsender? success! */
11261127
if (wspid == 0)
11271128
break;
11281129

11291130
ereport(LOG,
11301131
(errmsg("terminating walsender %d because replication slot \"%s\" is too far behind",
1131-
wspid, slotname)));
1132+
wspid, NameStr(slotname))));
11321133
(void) kill(wspid, SIGTERM);
11331134

11341135
ConditionVariableTimedSleep(&s->active_cv, 10,
@@ -1138,7 +1139,7 @@ InvalidateObsoleteReplicationSlots(XLogSegNo oldestSegno)
11381139

11391140
ereport(LOG,
11401141
(errmsg("invalidating slot \"%s\" because its restart_lsn %X/%X exceeds max_slot_wal_keep_size",
1141-
slotname,
1142+
NameStr(slotname),
11421143
(uint32) (restart_lsn >> 32),
11431144
(uint32) restart_lsn)));
11441145

src/backend/replication/slotfuncs.c

+49-54
Original file line numberDiff line numberDiff line change
@@ -278,88 +278,71 @@ pg_get_replication_slots(PG_FUNCTION_ARGS)
278278
for (slotno = 0; slotno < max_replication_slots; slotno++)
279279
{
280280
ReplicationSlot *slot = &ReplicationSlotCtl->replication_slots[slotno];
281+
ReplicationSlot slot_contents;
281282
Datum values[PG_GET_REPLICATION_SLOTS_COLS];
282283
bool nulls[PG_GET_REPLICATION_SLOTS_COLS];
283-
284-
ReplicationSlotPersistency persistency;
285-
TransactionId xmin;
286-
TransactionId catalog_xmin;
287-
XLogRecPtr restart_lsn;
288-
XLogRecPtr confirmed_flush_lsn;
289-
pid_t active_pid;
290-
Oid database;
291-
NameData slot_name;
292-
NameData plugin;
293284
WALAvailability walstate;
294285
XLogSegNo last_removed_seg;
295286
int i;
296287

297288
if (!slot->in_use)
298289
continue;
299290

291+
/* Copy slot contents while holding spinlock, then examine at leisure */
300292
SpinLockAcquire(&slot->mutex);
301-
302-
xmin = slot->data.xmin;
303-
catalog_xmin = slot->data.catalog_xmin;
304-
database = slot->data.database;
305-
restart_lsn = slot->data.restart_lsn;
306-
confirmed_flush_lsn = slot->data.confirmed_flush;
307-
namecpy(&slot_name, &slot->data.name);
308-
namecpy(&plugin, &slot->data.plugin);
309-
active_pid = slot->active_pid;
310-
persistency = slot->data.persistency;
311-
293+
slot_contents = *slot;
312294
SpinLockRelease(&slot->mutex);
313295

296+
memset(values, 0, sizeof(values));
314297
memset(nulls, 0, sizeof(nulls));
315298

316299
i = 0;
317-
values[i++] = NameGetDatum(&slot_name);
300+
values[i++] = NameGetDatum(&slot_contents.data.name);
318301

319-
if (database == InvalidOid)
302+
if (slot_contents.data.database == InvalidOid)
320303
nulls[i++] = true;
321304
else
322-
values[i++] = NameGetDatum(&plugin);
305+
values[i++] = NameGetDatum(&slot_contents.data.plugin);
323306

324-
if (database == InvalidOid)
307+
if (slot_contents.data.database == InvalidOid)
325308
values[i++] = CStringGetTextDatum("physical");
326309
else
327310
values[i++] = CStringGetTextDatum("logical");
328311

329-
if (database == InvalidOid)
312+
if (slot_contents.data.database == InvalidOid)
330313
nulls[i++] = true;
331314
else
332-
values[i++] = database;
315+
values[i++] = ObjectIdGetDatum(slot_contents.data.database);
333316

334-
values[i++] = BoolGetDatum(persistency == RS_TEMPORARY);
335-
values[i++] = BoolGetDatum(active_pid != 0);
317+
values[i++] = BoolGetDatum(slot_contents.data.persistency == RS_TEMPORARY);
318+
values[i++] = BoolGetDatum(slot_contents.active_pid != 0);
336319

337-
if (active_pid != 0)
338-
values[i++] = Int32GetDatum(active_pid);
320+
if (slot_contents.active_pid != 0)
321+
values[i++] = Int32GetDatum(slot_contents.active_pid);
339322
else
340323
nulls[i++] = true;
341324

342-
if (xmin != InvalidTransactionId)
343-
values[i++] = TransactionIdGetDatum(xmin);
325+
if (slot_contents.data.xmin != InvalidTransactionId)
326+
values[i++] = TransactionIdGetDatum(slot_contents.data.xmin);
344327
else
345328
nulls[i++] = true;
346329

347-
if (catalog_xmin != InvalidTransactionId)
348-
values[i++] = TransactionIdGetDatum(catalog_xmin);
330+
if (slot_contents.data.catalog_xmin != InvalidTransactionId)
331+
values[i++] = TransactionIdGetDatum(slot_contents.data.catalog_xmin);
349332
else
350333
nulls[i++] = true;
351334

352-
if (restart_lsn != InvalidXLogRecPtr)
353-
values[i++] = LSNGetDatum(restart_lsn);
335+
if (slot_contents.data.restart_lsn != InvalidXLogRecPtr)
336+
values[i++] = LSNGetDatum(slot_contents.data.restart_lsn);
354337
else
355338
nulls[i++] = true;
356339

357-
if (confirmed_flush_lsn != InvalidXLogRecPtr)
358-
values[i++] = LSNGetDatum(confirmed_flush_lsn);
340+
if (slot_contents.data.confirmed_flush != InvalidXLogRecPtr)
341+
values[i++] = LSNGetDatum(slot_contents.data.confirmed_flush);
359342
else
360343
nulls[i++] = true;
361344

362-
walstate = GetWALAvailability(restart_lsn);
345+
walstate = GetWALAvailability(slot_contents.data.restart_lsn);
363346

364347
switch (walstate)
365348
{
@@ -378,6 +361,9 @@ pg_get_replication_slots(PG_FUNCTION_ARGS)
378361
case WALAVAIL_REMOVED:
379362
values[i++] = CStringGetTextDatum("lost");
380363
break;
364+
365+
default:
366+
elog(ERROR, "invalid walstate: %d", (int) walstate);
381367
}
382368

383369
if (max_slot_wal_keep_size_mb >= 0 &&
@@ -393,8 +379,11 @@ pg_get_replication_slots(PG_FUNCTION_ARGS)
393379
else
394380
nulls[i++] = true;
395381

382+
Assert(i == PG_GET_REPLICATION_SLOTS_COLS);
383+
396384
tuplestore_putvalues(tupstore, tupdesc, values, nulls);
397385
}
386+
398387
LWLockRelease(ReplicationSlotControlLock);
399388

400389
tuplestore_donestoring(tupstore);
@@ -653,6 +642,8 @@ copy_replication_slot(FunctionCallInfo fcinfo, bool logical_slot)
653642
Name src_name = PG_GETARG_NAME(0);
654643
Name dst_name = PG_GETARG_NAME(1);
655644
ReplicationSlot *src = NULL;
645+
ReplicationSlot first_slot_contents;
646+
ReplicationSlot second_slot_contents;
656647
XLogRecPtr src_restart_lsn;
657648
bool src_islogical;
658649
bool temporary;
@@ -692,13 +683,10 @@ copy_replication_slot(FunctionCallInfo fcinfo, bool logical_slot)
692683

693684
if (s->in_use && strcmp(NameStr(s->data.name), NameStr(*src_name)) == 0)
694685
{
686+
/* Copy the slot contents while holding spinlock */
695687
SpinLockAcquire(&s->mutex);
696-
src_islogical = SlotIsLogical(s);
697-
src_restart_lsn = s->data.restart_lsn;
698-
temporary = s->data.persistency == RS_TEMPORARY;
699-
plugin = logical_slot ? pstrdup(NameStr(s->data.plugin)) : NULL;
688+
first_slot_contents = *s;
700689
SpinLockRelease(&s->mutex);
701-
702690
src = s;
703691
break;
704692
}
@@ -711,6 +699,11 @@ copy_replication_slot(FunctionCallInfo fcinfo, bool logical_slot)
711699
(errcode(ERRCODE_UNDEFINED_OBJECT),
712700
errmsg("replication slot \"%s\" does not exist", NameStr(*src_name))));
713701

702+
src_islogical = SlotIsLogical(&first_slot_contents);
703+
src_restart_lsn = first_slot_contents.data.restart_lsn;
704+
temporary = (first_slot_contents.data.persistency == RS_TEMPORARY);
705+
plugin = logical_slot ? NameStr(first_slot_contents.data.plugin) : NULL;
706+
714707
/* Check type of replication slot */
715708
if (src_islogical != logical_slot)
716709
ereport(ERROR,
@@ -775,18 +768,20 @@ copy_replication_slot(FunctionCallInfo fcinfo, bool logical_slot)
775768

776769
/* Copy data of source slot again */
777770
SpinLockAcquire(&src->mutex);
778-
copy_effective_xmin = src->effective_xmin;
779-
copy_effective_catalog_xmin = src->effective_catalog_xmin;
771+
second_slot_contents = *src;
772+
SpinLockRelease(&src->mutex);
780773

781-
copy_xmin = src->data.xmin;
782-
copy_catalog_xmin = src->data.catalog_xmin;
783-
copy_restart_lsn = src->data.restart_lsn;
784-
copy_confirmed_flush = src->data.confirmed_flush;
774+
copy_effective_xmin = second_slot_contents.effective_xmin;
775+
copy_effective_catalog_xmin = second_slot_contents.effective_catalog_xmin;
776+
777+
copy_xmin = second_slot_contents.data.xmin;
778+
copy_catalog_xmin = second_slot_contents.data.catalog_xmin;
779+
copy_restart_lsn = second_slot_contents.data.restart_lsn;
780+
copy_confirmed_flush = second_slot_contents.data.confirmed_flush;
785781

786782
/* for existence check */
787-
copy_name = pstrdup(NameStr(src->data.name));
788-
copy_islogical = SlotIsLogical(src);
789-
SpinLockRelease(&src->mutex);
783+
copy_name = NameStr(second_slot_contents.data.name);
784+
copy_islogical = SlotIsLogical(&second_slot_contents);
790785

791786
/*
792787
* Check if the source slot still exists and is valid. We regard it as

src/include/replication/slot.h

+2-2
Original file line numberDiff line numberDiff line change
@@ -158,8 +158,8 @@ typedef struct ReplicationSlot
158158
XLogRecPtr candidate_restart_lsn;
159159
} ReplicationSlot;
160160

161-
#define SlotIsPhysical(slot) (slot->data.database == InvalidOid)
162-
#define SlotIsLogical(slot) (slot->data.database != InvalidOid)
161+
#define SlotIsPhysical(slot) ((slot)->data.database == InvalidOid)
162+
#define SlotIsLogical(slot) ((slot)->data.database != InvalidOid)
163163

164164
/*
165165
* Shared memory control area for all of replication slots.

0 commit comments

Comments
 (0)