On 2020/06/17 3:50, Alvaro Herrera wrote:
On 2020-Jun-17, Fujii Masao wrote:
While reading InvalidateObsoleteReplicationSlots() code, I found another issue.
ereport(LOG,
(errmsg("terminating walsender %d because replication
slot \"%s\" is too far behind",
wspid,
NameStr(slotname))));
(void) kill(wspid, SIGTERM);
wspid indicates the PID of process using the slot. That process can be
a backend, for example, executing pg_replication_slot_advance().
So "walsender" in the above log message is not always correct.
Good point.
So InvalidateObsoleteReplicationSlots() can terminate normal backends.
But do we want to do this? If we want, we should add the note about this
case into the docs? Otherwise the users would be surprised at termination
of backends by max_slot_wal_keep_size. I guess that it's basically rarely
happen, though.
int wspid =
ReplicationSlotAcquire(NameStr(slotname),
SAB_Inquire);
Why do we need to call ReplicationSlotAcquire() here and mark the slot as
used by the checkpointer? Isn't it enough to check directly the slot's
active_pid, instead?
Maybe ReplicationSlotAcquire() is necessary because
ReplicationSlotRelease() is called later? If so, why do we need to call
ReplicationSlotRelease()? ISTM that we don't need to do that if the slot's
active_pid is zero. No?
I think the point here was that in order to modify the slot you have to
acquire it -- it's not valid to modify a slot you don't own.
Understood. Thanks!
+ /*
+ * Signal to terminate the process using the replication slot.
+ *
+ * Try to signal every 100ms until it succeeds.
+ */
+ if (!killed && kill(active_pid, SIGTERM) == 0)
+ killed = true;
+ ConditionVariableTimedSleep(&slot->active_cv, 100,
+
WAIT_EVENT_REPLICATION_SLOT_DROP);
+ } while (ReplicationSlotIsActive(slot, NULL));
Note that here you're signalling only once and then sleeping many times
in increments of 100ms -- you're not signalling every 100ms as the
comment claims -- unless the signal fails, but you don't really expect
that. On the contrary, I'd claim that the logic is reversed: if the
signal fails, *then* you should stop signalling.
You mean; in this code path, signaling fails only when the target process
disappears just before signaling. So if it fails, slot->active_pid is
expected to become 0 even without signaling more. Right?
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
