Re: Introduce XID age and inactive timeout based replication slot invalidation

On Fri, Mar 22, 2024 at 3:15 PM Bertrand Drouvot
<bertranddrouvot(dot)pg(at)gmail(dot)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 we agree,
probably checking restart_lsn should suffice the need to know whether
the WAL is reserved or not.

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

--
With Regards,
Amit Kapila.