On Fri, Mar 22, 2024 at 5:30 PM Bertrand Drouvot <bertranddrouvot...@gmail.com> wrote: > > On Fri, Mar 22, 2024 at 03:56:23PM +0530, Amit Kapila wrote: > > On Fri, Mar 22, 2024 at 3:15 PM Bertrand Drouvot > > <bertranddrouvot...@gmail.com> wrote: > > > > > > On Fri, Mar 22, 2024 at 01:45:01PM +0530, Bharath Rupireddy wrote: > > > > > > 1 === > > > > > > @@ -691,6 +699,13 @@ ReplicationSlotRelease(void) > > > ConditionVariableBroadcast(&slot->active_cv); > > > } > > > > > > + if (slot->data.persistency == RS_PERSISTENT) > > > + { > > > + SpinLockAcquire(&slot->mutex); > > > + slot->last_inactive_at = GetCurrentTimestamp(); > > > + SpinLockRelease(&slot->mutex); > > > + } > > > > > > I'm not sure we should do system calls while we're holding a spinlock. > > > Assign a variable before? > > > > > > 2 === > > > > > > Also, what about moving this here? > > > > > > " > > > if (slot->data.persistency == RS_PERSISTENT) > > > { > > > /* > > > * Mark persistent slot inactive. We're not freeing it, just > > > * disconnecting, but wake up others that may be waiting for it. > > > */ > > > SpinLockAcquire(&slot->mutex); > > > slot->active_pid = 0; > > > SpinLockRelease(&slot->mutex); > > > ConditionVariableBroadcast(&slot->active_cv); > > > } > > > " > > > > > > That would avoid testing twice "slot->data.persistency == RS_PERSISTENT". > > > > > > > That sounds like a good idea. Also, don't we need to consider physical > > slots where we don't reserve WAL during slot creation? I don't think > > there is a need to set inactive_at for such slots. > > If the slot is not active, why shouldn't we set inactive_at? I can understand > that such a slots do not present "any risks" but I think we should still set > inactive_at (also to not give the false impression that the slot is active). >
But OTOH, there is a chance that we will invalidate such slots even though they have never reserved WAL in the first place which doesn't appear to be a good thing. > > > 5 === > > > > > > Most of the fields that reflect a time (not duration) in the system views > > > are > > > xxxx_time, so I'm wondering if instead of "last_inactive_at" we should use > > > something like "last_inactive_time"? > > > > > > > How about naming it as last_active_time? This will indicate the time > > at which the slot was last active. > > I thought about it too but I think it could be missleading as one could think > that > it should be updated each time WAL record decoding is happening. > Fair enough. -- With Regards, Amit Kapila.