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

On Thu, Jan 2, 2025 at 5:44 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> Hi Nisha.
>
> My review comments for patch v58-0001.
>
> ======
> src/backend/replication/slot.c
>
> InvalidatePossiblyObsoleteSlot:
>
> 1.
> /*
> - * If the slot can be acquired, do so and mark it invalidated
> - * immediately. Otherwise we'll signal the owning process, below, and
> - * retry.
> + * If the slot can be acquired, do so and mark it as invalidated. If
> + * the slot is already ours, mark it as invalidated. Otherwise, we'll
> + * signal the owning process below and retry.
> */
> - if (active_pid == 0)
> + if (active_pid == 0 ||
> + (MyReplicationSlot == s && active_pid == MyProcPid))
> {
>
> As you previously explained [1] "This change applies to all types of
> invalidation, not just inactive_timeout case [...] It's a general
> optimization for the case when the current process is the active PID
> for the slot."
>
> In that case, should this be in a separate patch that can be pushed to
> master by itself, i.e. independent of anything else in this thread
> that is being done for the purpose of implementing the timeout
> feature?

The patch-001 has additional general optimizations similar to the one
you mentioned, which are not strictly required for this feature.
Let’s wait for input from others on splitting the patches or
addressing it in a separate thread.

--
Thanks,
Nisha