On Thu, Sep 24, 2026 at 3:11 PM Bertrand Drouvot
<[email protected]> wrote:
>
> Hi,
>
> On Thu, Sep 24, 2026 at 10:55:03AM +0530, shveta malik wrote:
> > I had a look at patch001 as well. I have 2 questions:
> >
> > 1)
> > Would it be better to use a PG_TRY/PG_CATCH block in patch 001,
> > similar to patch 002? Currently, the slot is released via
> > PG_ENSURE_ERROR_CLEANUP, while we rely on top-level error cleanup for
> > lock-release. Using TRY/CATCH would let us explicitly release both the
> > slot and I/O lock together, making the error handling consistent
> > across both patches. We could even reuse persist_slot_invalidation()
> > with a small change to pass update_inactive_since from the caller.
>
> Yeah, makes sense. I changed 0001 to use PG_TRY/PG_CATCH and moved the common
> cleanup into ReplicationSlotPersistInvalidation(), with update_inactive_since
> passed by the caller.
>
> The I/O lock is still acquired by each caller because 0001 must hold it before
> claiming the inactive slot to serialize concurrent internal invalidators.
Yes, makes sense.
> > 2)
> > + /* Let caller know */
> > + invalidated = true;
> > + LWLockRelease(&s->io_in_progress_lock);
> > ReplicationSlotRelease();
> >
> > Wouldn't it be better (and safer) to release the slot before releasing
> > the I/O lock?
> >
> > Currently, concurrent invalidators are protected by the
> > 'invalidation_cause == RS_INVAL_NONE' check after acquiring the lock.
> > But releasing the slot first would close this race window entirely. It
> > would also make the order consistent with Patch 002 and the
> > error-handling flow in Patch 001 itself.
>
> The current ordering should be safe because the invalidation has already been
> published, so a concurrent invalidator exits before considering active_proc.
Oh I see. I missed this point earlier.
> That said, I agree that releasing the slot first means this ordering no longer
> relies on that check and makes the success and error paths consistent. So,
> done
> in the attached.
>
> It also adds the check you suggested for the synchronized slot's shared memory
> state after a successful synchronization.
>
Thanks for addressing comments. A few concerns on 001:
1)
In SaveSlotToPath(), should we add an 'Assert(cp.slotdata.restart_lsn
== InvalidXLogRecPtr)' at the end for the 'if (clear_restart_lsn)'
case?
slot->data.invalidated = invalidation_cause;
if (clear_restart_lsn)
+ {
+ Assert(cp.slotdata.restart_lsn == InvalidXLogRecPtr);
slot->data.restart_lsn = InvalidXLogRecPtr;
+ }
While slot->last_saved_restart_lsn correctly inherits
cp.slotdata.restart_lsn on the next line, adding this Assert
guarantees that the removed logic from
InvalidatePossiblyObsoleteSlot() was successfully compensated for in
the on-disk struct before we propagate it to shared memory. It is not
mandatory, but it would be good to have.
2)
+ Assert(update_inactive_since || slot->data.persistency == RS_PERSISTENT);
In ReplicationSlotReleaseInternal(), I didn’t quite understand the
reasoning behind above Assert. Does this mean that when the caller
passes update_inactive_since=true, the slot can even be temporary,
whereas if we are not updating inactive_since, the slot must be
persistent?
Is this based on the fact that slotsync passes update_inactive_since =
true and can have a temporary slot here? If so, that is not very clear
from the Assert itself. If we want to retain this check, should we
instead move the 'update_inactive_since ||' part to patch 002? IIUC,
patch 001 seems to strictly require RS_PERSISTENT. Let me know if I
understand it wrong.
I’m not sure what else we can do to make this clearer, but I think at
least a comment explaining why a temporary slot is allowed in this
case (update_inactive_since=true) would help.
3)
Another doubt I have is that with above Assert, when
update_inactive_since is TRUE, we are even allowing RS_EPHEMERAL
slots. However, ReplicationSlotPersistInvalidation() explicitly
disallows them in patch002 with:
Assert(slot->data.persistency != RS_EPHEMERAL);
Both checks are not in sync.
thanks
Shveta
