Re: Persist slot invalidations before publishing them

Hi,

On Mon, Sep 28, 2026 at 9:16 PM Bertrand Drouvot
<bertranddrouvot(dot)pg(at)gmail(dot)com> wrote:
>
> The patch needed a rebase, so at the same time I went ahead with the proposed
> changes above (plus the one in the commit message suggested by Rui in [1]).
>

Thanks for reporting the problem and providing a patch for it. The
approach looks good to me, but I have a few comments to share:

1)

The normal replication slot update pattern is:

SpinLockAcquire(&slot->mutex);
slot->data.some_field = new_value;
SpinLockRelease(&slot->mutex);

ReplicationSlotMarkDirty();
ReplicationSlotSave();

i.e. we first update the slot in memory and then persist the changes
to disk. This is an exception where we need to persist the change to
disk before publishing it in memory.

I think it would be good to add a comment above
InvalidatePossiblyObsoleteSlot() explaining the reason for this change
in the usual slot update pattern.

--

2)

+ {
+ SaveSlotToPath(slot, path, ERROR, cause, clear_restart_lsn);
+ }

Do we need to pass clear_restart_lsn here? Cause can be used to
determine the restart lsn value later, no?

--

3)

The current patch changes the interface for SaveSlotToPath() (), isn't
it possible to keep it unchanged, that would probably also reduce some
amount of complexity. How about renaming the extended implementation
to SaveSlotToPathInternal() and have SaveSlotToPath() call it with
RS_INVAL_NONE and false. This avoids exposing invalidation specific
arguments to normal save callers. See below:

static void
SaveSlotToPath(ReplicationSlot *slot, const char *dir, int elevel)
{
SaveSlotToPathInternal(slot, dir, elevel, RS_INVAL_NONE, false);
}

For slot invalidation, add a dedicated SaveInvalidatedSlotToPath()
wrapper that calls SaveSlotToPathInternal() with the required
invalidation state.

static void
SaveInvalidatedSlotToPath(ReplicationSlot *slot, const char *dir,
ReplicationSlotInvalidationCause cause,
bool clear_restart_lsn)
{
Assert(cause != RS_INVAL_NONE);
Assert(LWLockHeldByMeInMode(&slot->io_in_progress_lock, LW_EXCLUSIVE));

SaveSlotToPathInternal(slot, dir, ERROR, cause, clear_restart_lsn);
}

--
With Regards,
Ashutosh Sharma.