Re: Review for GetWALAvailability()

On 2020-Jun-17, Fujii Masao wrote:
> On 2020/06/17 3:50, Alvaro Herrera wrote:

> 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.

Well, if we could distinguish a walsender from a non-walsender process,
then maybe it would make sense to leave backends alive. But do we want
that? I admit I don't know what would be the reason to have a
non-walsender process with an active slot, so I don't have a good
opinion on what to do in this case.

> > > + /*
> > > + * 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?

I guess kill() can also fail if the PID now belongs to a process owned
by a different user. I think we've disregarded very quick reuse of
PIDs, so we needn't concern ourselves with it.

--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services