Persist slot invalidations before publishing them

Hi Bertrand

3/ I think 0001 should be backpatched down to 14. 14 and 15 would probably need
some adaptations though (I did not look in detail yet).

4/ 0002 should be backpatched down to 17, where failover slot synchronization was
introduced.

I did a brief review and tried to test these changes locally.
I ran test for PG19 and master, and they passed, but couldn't
apply patches for PG18 and lower.

I'll try dig deeper into this patch later.

Kind regards,
Miłosz Bieniek

Hi Miłosz,

On Wed, Aug 26, 2026 at 07:24:07PM +0000, Miłosz Bieniek wrote:

Hi Bertrand

3/ I think 0001 should be backpatched down to 14. 14 and 15 would probably need
some adaptations though (I did not look in detail yet).

4/ 0002 should be backpatched down to 17, where failover slot synchronization was
introduced.

I did a brief review and tried to test these changes locally.

Thanks for looking at it!

I ran test for PG19 and master, and they passed, but couldn't
apply patches for PG18 and lower.

Yeah, the back patch versions are not done. I think it's better to discuss the
issue first and agree on what the fix should be on master. Once done the
back patch versions will be prepared based on the agreement.

Regards,

--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

Hello,

At Wed, 26 Aug 2026 13:49:24 +0000, Bertrand Drouvot <bertranddrouvot.pg@gmail.com> wrote in

If ReplicationSlotSave() errors before replacing the state file, the slot is
invalid in shared memory but still valid on disk. That sounds problematic as the
resource horizon computations could stop accounting for the slot, remove required
WAL or rows, and then an immediate restart would restore the old valid slot image.

I've spent some time looking through the related discussions and
patches, and I think I now have a better understanding of the
problem. I have a question about the persistence mechanism.

For the InvalidatePossiblyObsoleteSlot() case, at least for
RS_INVAL_XID_AGE, if the server crashes after the slot is invalidated
but before the invalidation is persisted, it seems that the restored
slot would still satisfy the same XID-age condition and would
eventually be invalidated again by vacuum or checkpoint. Is the main
reason for making the invalidation durable here that we don't want to
leave the slot valid until that next opportunity?

If so, I'm a little uncomfortable with persisting a modified copy of
the normal slot state before that state has actually been published in
shared memory. It seems to make the state transition somewhat harder
to follow, since the slot state file no longer necessarily represents
the current slot state.

Would it be simpler to persist the invalidation separately? For
example, we could write the invalidation cause to a small file such as
pg_replslot/<slotname>/invalidated and make it durable before
publishing the invalidation in shared memory. On restart, that file
would cause the slot to be restored as invalidated with the recorded
cause. This would keep the normal slot state file as a representation
of the actual slot state, and would also naturally avoid the race with
concurrent slot saves.

Regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

Hi Horiguchi-san,

On Thu, Aug 27, 2026 at 05:06:47PM +0900, Kyotaro Horiguchi wrote:

Hello,

At Wed, 26 Aug 2026 13:49:24 +0000, Bertrand Drouvot <bertranddrouvot.pg@gmail.com> wrote in

If ReplicationSlotSave() errors before replacing the state file, the slot is
invalid in shared memory but still valid on disk. That sounds problematic as the
resource horizon computations could stop accounting for the slot, remove required
WAL or rows, and then an immediate restart would restore the old valid slot image.

I've spent some time looking through the related discussions and
patches,

Thanks for looking at it!

For the InvalidatePossiblyObsoleteSlot() case, at least for
RS_INVAL_XID_AGE, if the server crashes after the slot is invalidated
but before the invalidation is persisted, it seems that the restored
slot would still satisfy the same XID-age condition and would
eventually be invalidated again by vacuum or checkpoint. Is the main
reason for making the invalidation durable here that we don't want to
leave the slot valid until that next opportunity?

Yeah, for XID age the condition should still hold after restart. On a primary,
the end of recovery checkpoint should detect it before connections are accepted.

On a hot standby, however, connections can be accepted before the next successful
restartpoint, so a restored slot could be used while valid although rows it
needed may already have been removed.

Also, other causes are not necessarily rediscovered immediately. For example,
inactive_since is reset at startup, so an idle timeout invalidation would not
be detected again until the timeout has elapsed again.

If so, I'm a little uncomfortable with persisting a modified copy of
the normal slot state before that state has actually been published in
shared memory. It seems to make the state transition somewhat harder
to follow, since the slot state file no longer necessarily represents
the current slot state.

Would it be simpler to persist the invalidation separately?

example, we could write the invalidation cause to a small file such as
pg_replslot/<slotname>/invalidated and make it durable before
publishing the invalidation in shared memory. On restart, that file
would cause the slot to be restored as invalidated with the recorded
cause. This would keep the normal slot state file as a representation
of the actual slot state, and would also naturally avoid the race with
concurrent slot saves.

Your proposal could probably work too. I’m not sure it would be simpler though,
as it would add another on disk state and startup handling. I also could not find
a precedent for introducing such a persistent file in back branches, but I may
have missed one. The proposed patch reuses the existing slot state and format,
which probably makes it more suitable for backpatching.

Regards,

--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

Thanks for the detailed discussion.

I'm following the reasoning around the durability requirement, particularly
the hot standby case and invalidation causes such as 'inactive_since'.

The separate invalidation file approach makes sense to me as an
alternative, while I also see the advantage of reusing the existing slot
state for backpatching.

I'll hold off on further changes for now and follow the discussion on which
approach is preferred for master implementation.

Regards,
Osama Abdul Qader

On Thu, Aug 27, 2026 at 3:57 PM Bertrand Drouvot <
bertranddrouvot.pg@gmail.com> wrote:

On Wed, Aug 26, 2026 at 7:19 PM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:

while reviewing [1], I hit an issue due to the fact that an inactive replication
slot is marked invalid in shared memory before its new state is persisted.

If ReplicationSlotSave() errors before replacing the state file, the slot is
invalid in shared memory but still valid on disk. That sounds problematic as the
resource horizon computations could stop accounting for the slot, remove required
WAL or rows, and then an immediate restart would restore the old valid slot image.

The same issue exists in synchronize_one_slot(): it copies the invalidation from
the remote slot into the local synchronized slot before saving it. In that case,
a save error also prevents a direct retry because the next synchronization sees
the local slot as already invalid and skips it.

Won't the drop_local_obsolete_slots() drop the locally invalidated
slot before trying to synchronize the remote_slot in the next slot?

--
With Regards,
Amit Kapila.

Hi,

On Fri, Aug 28, 2026 at 03:59:32PM +0530, Amit Kapila wrote:

On Wed, Aug 26, 2026 at 7:19 PM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:

while reviewing [1], I hit an issue due to the fact that an inactive replication
slot is marked invalid in shared memory before its new state is persisted.

If ReplicationSlotSave() errors before replacing the state file, the slot is
invalid in shared memory but still valid on disk. That sounds problematic as the
resource horizon computations could stop accounting for the slot, remove required
WAL or rows, and then an immediate restart would restore the old valid slot image.

The same issue exists in synchronize_one_slot(): it copies the invalidation from
the remote slot into the local synchronized slot before saving it. In that case,
a save error also prevents a direct retry because the next synchronization sees
the local slot as already invalid and skips it.

Won't the drop_local_obsolete_slots() drop the locally invalidated
slot before trying to synchronize the remote_slot in the next slot?

It would be if the remote slot were valid and only the local slot invalidated.

Here both are invalidated, so locally_invalidated is false and local_sync_slot_required()
returns true. Thus, drop_local_obsolete_slots() keeps the local slot.
synchronize_one_slot() then sees it already invalidated and skips the save.

The test added by 0002 is meant to cover this. I noticed that v1 restarts
before synchronizing again though, so I changed it in v2 to retry before the
immediate restart and make this case explicit.

Regards,

--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

Hi Bertrand,

I read v2 and have a question about the window the new ordering opens
between two invalidators.

As I understand it, in v2 the first invalidator acquires the slot,
writes the state file with the invalidation, and only after the fsync
sets data.invalidated in shared memory and releases the slot. While the
write is in progress the slot looks valid to everyone else, with the
invalidator's PID as active_pid. If a second invalidator reaches the
same slot in that window (say a restartpoint enforcing
max_slot_wal_keep_size while startup is replaying a wal_level change, or
the other way round), it seems it would take the "slot is in use" path,
so startup might send a recovery conflict to active_pid, or another
process a SIGTERM. What do you think?

--
JoongHyuk Shin

Hi,

Thanks for v2. I made the save fail with the injection point of 0001,
and the slot comes out of it fine: still valid, released by the
checkpointer, and I can drop it. On master the same failure leaves it
invalid but still owned by the checkpointer, and the next checkpoint
does not fix that:

active|active_pid|invalidation_reason|restart_lsn|wal_status
t|259023|wal_removed||lost

SELECT pg_drop_replication_slot('target_slot');
ERROR: replication slot "target_slot" is active for PID 259023

If a second invalidator reaches the same slot in that window (say a
restartpoint enforcing max_slot_wal_keep_size while startup is replaying
a wal_level change, or the other way round), it seems it would take the
"slot is in use" path, so startup might send a recovery conflict to
active_pid, or another process a SIGTERM. What do you think?

Both happen, and the SIGTERM shuts the standby down. 0003 makes the
second invalidator wait for the first one instead, and adds a test with
the two processes in both orders.

The standby has max_slot_wal_keep_size = 1MB, a logical slot 8 segments
of 1MB behind, and the injection point of 0001 attached with 'wait' for
that slot. The primary is restarted with wal_level = replica, so that the
startup process invalidates the slot, and once it waits at the injection
point, CHECKPOINT on the standby:

checkpointer LOG: restartpoint starting: fast wait
checkpointer LOG: terminating process 378422 to release replication
slot "startup_first"
checkpointer DETAIL: The slot's restart_lsn 0/012002D0 exceeds the
limit by 7339312 bytes.
startup LOG: invalidating obsolete replication slot "startup_first"
postmaster LOG: startup process (PID 378422) exited with exit code 1
postmaster LOG: terminating any other active server processes
postmaster LOG: shutting down due to startup process failure

The other way round, the startup process finds the slot held by the
checkpointer and sends it a recovery conflict. The checkpointer errors
out at its next CHECK_FOR_INTERRUPTS(), and the startup process then
invalidates the slot itself:

startup LOG: terminating process 378420 to release replication slot
"checkpointer_first"
checkpointer ERROR: canceling statement due to conflict with recovery
client backend ERROR: checkpoint request failed
startup LOG: invalidating obsolete replication slot "checkpointer_first"

On master the slot is marked invalid under the spinlock at the moment it
is claimed, so the second invalidator sees the invalidation and does
nothing.

0003 takes the slot's io_in_progress_lock before looking at the slot,
and holds it from claiming the slot until the invalidation is published;
0001 holds it for the write only. Whoever comes second then sees either
an invalidated slot or one nobody is working on, and only signals a
process that really uses the slot. If the lock is not free, the function
drops ReplicationSlotControlLock, waits for it, and starts over, the same
way it does around the condition variable sleep. The test fails on v2 as
above and passes with 0003.

The side effects of 0003, as far as I can see:

(a) The second invalidator now waits (wait event ReplicationSlotIO)
instead of signalling; on master it never waited. The wait is one write
and fsync of the slot's state file, and only when two processes go for
the same slot at the same time.

(b) Every pass over the slots now takes and releases each in-use slot's
io_in_progress_lock once. A checkpoint's pass over N valid physical
slots, median of 60 checkpoints:

N v2 0003
10 1.5 us 1.6 us
100 7.2 us 9.0 us

A pass that finds a slot being written waits for that write, without
ReplicationSlotControlLock.

(c) The two locks cannot deadlock: no holder of an io_in_progress_lock
takes ReplicationSlotControlLock, since the callers of SaveSlotToPath()
hold ReplicationSlotAllocationLock at most, and the slot is not in use
yet when CreateSlotOnDisk() writes it. The second invalidator holds
nothing while it waits. Nothing changes for a slot that is really in use:
a walsender's slot, or a temporary slot of an idle session, is signalled
as before.

(d) Not covered: synchronize_one_slot() acquires the slot before it
takes the lock, so a restartpoint that looks at the slot in between still
terminates the slot sync worker. Master has the same window, between
acquiring the slot and publishing; closing it would take a flag in the
slot's shared memory.

I think 0003 is small enough to be backpatched with 0001.

Regards,
Rui

Hi JoongHyuk,

On Sun, Sep 13, 2026 at 07:31:57PM +0900, JoongHyuk Shin wrote:

Hi Bertrand,

I read v2 and have a question about the window the new ordering opens
between two invalidators.

Thanks for looking at it!

If a second invalidator reaches the
same slot in that window (say a restartpoint enforcing
max_slot_wal_keep_size while startup is replaying a wal_level change, or
the other way round), it seems it would take the "slot is in use" path,
so startup might send a recovery conflict to active_pid, or another
process a SIGTERM. What do you think?

You're right, v2 could treat the first invalidator as a regular slot user and
terminate it.

Rui, thanks for the patch! I've incorporated its locking approach and test coverage
in v3, with some adjustments around error cleanup.

Please find v3 attached.

Regards,

--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

On Tue, Sep 22, 2026 at 1:58 PM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:

Hi JoongHyuk,

On Sun, Sep 13, 2026 at 07:31:57PM +0900, JoongHyuk Shin wrote:

Hi Bertrand,

I read v2 and have a question about the window the new ordering opens
between two invalidators.

Thanks for looking at it!

If a second invalidator reaches the
same slot in that window (say a restartpoint enforcing
max_slot_wal_keep_size while startup is replaying a wal_level change, or
the other way round), it seems it would take the "slot is in use" path,
so startup might send a recovery conflict to active_pid, or another
process a SIGTERM. What do you think?

You're right, v2 could treat the first invalidator as a regular slot user and
terminate it.

Rui, thanks for the patch! I've incorporated its locking approach and test coverage
in v3, with some adjustments around error cleanup.

Please find v3 attached.

I had a look at 002 to review slotsync path, I had one concern:

+ /*
+ * A failed invalidation can still hold the slot's I/O lock. Release it
+ * before slot cleanup acquires ReplicationSlotAllocationLock, which
+ * checkpoints hold while acquiring slot I/O locks.
+ */
+ LWLockReleaseAll();
+

Could it be problematic to call LWLockReleaseAll() inside a localized
error cleanup callback (PG_ENSURE_ERROR_CLEANUP) rather than waiting
for AbortTransaction or proc_exit? Since the goal is just to avoid
deadlock with the Checkpointer, shouldn't we explicitly release that
one specific lock?
if (MyReplicationSlot != NULL &&
LWLockHeldByMe(&MyReplicationSlot->io_in_progress_lock))
{
LWLockRelease(&MyReplicationSlot->io_in_progress_lock);
}

I don't have an exact scenario to worry about, but it seems like
overkill. Thoughts?

thanks
Shveta

Hi,

On Tue, Sep 22, 2026 at 03:46:55PM +0530, shveta malik wrote:

I had a look at 002 to review slotsync path,

Thanks for looking at it!

+ /*
+ * A failed invalidation can still hold the slot's I/O lock. Release it
+ * before slot cleanup acquires ReplicationSlotAllocationLock, which
+ * checkpoints hold while acquiring slot I/O locks.
+ */
+ LWLockReleaseAll();
+

Could it be problematic to call LWLockReleaseAll() inside a localized
error cleanup callback (PG_ENSURE_ERROR_CLEANUP) rather than waiting
for AbortTransaction or proc_exit? Since the goal is just to avoid
deadlock with the Checkpointer, shouldn't we explicitly release that
one specific lock?
if (MyReplicationSlot != NULL &&
LWLockHeldByMe(&MyReplicationSlot->io_in_progress_lock))
{
LWLockRelease(&MyReplicationSlot->io_in_progress_lock);
}

I don't have an exact scenario to worry about, but it seems like
overkill. Thoughts?

Yeah, it's probably better to be specific here.

One concern with the proposed check is that all existing uses of LWLockHeldByMe()
appear to be for assertions or debugging (as documented on top of LWLockHeldByMe()).

Also, releasing an LWLock after ERROR requires restoring the interrupt holdoff
expected by LWLockRelease().

Another possibility would be to make ReplicationSlotPersistInvalidation() always
leave the caller acquired I/O lock held. Slotsync could then release that specific
lock in a PG_CATCH() block, something like:

"
PG_CATCH();
{
HOLD_INTERRUPTS();
LWLockRelease(&slot->io_in_progress_lock);
PG_RE_THROW();
}
PG_END_TRY();

LWLockRelease(&slot->io_in_progress_lock);
"

This would avoid both LWLockReleaseAll() and using LWLockHeldByMe() for normal
control flow. Does that sound preferable?

Regards,

--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

On Wed, Sep 23, 2026 at 11:37 AM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:

Hi,

On Tue, Sep 22, 2026 at 03:46:55PM +0530, shveta malik wrote:

I had a look at 002 to review slotsync path,

Thanks for looking at it!

+ /*
+ * A failed invalidation can still hold the slot's I/O lock. Release it
+ * before slot cleanup acquires ReplicationSlotAllocationLock, which
+ * checkpoints hold while acquiring slot I/O locks.
+ */
+ LWLockReleaseAll();
+

Could it be problematic to call LWLockReleaseAll() inside a localized
error cleanup callback (PG_ENSURE_ERROR_CLEANUP) rather than waiting
for AbortTransaction or proc_exit? Since the goal is just to avoid
deadlock with the Checkpointer, shouldn't we explicitly release that
one specific lock?
if (MyReplicationSlot != NULL &&
LWLockHeldByMe(&MyReplicationSlot->io_in_progress_lock))
{
LWLockRelease(&MyReplicationSlot->io_in_progress_lock);
}

I don't have an exact scenario to worry about, but it seems like
overkill. Thoughts?

Yeah, it's probably better to be specific here.

One concern with the proposed check is that all existing uses of LWLockHeldByMe()
appear to be for assertions or debugging (as documented on top of LWLockHeldByMe()).

Oh Okay, I missed this point earlier.

Also, releasing an LWLock after ERROR requires restoring the interrupt holdoff
expected by LWLockRelease().

Okay.

Another possibility would be to make ReplicationSlotPersistInvalidation() always
leave the caller acquired I/O lock held. Slotsync could then release that specific
lock in a PG_CATCH() block, something like:

Yes, I agree. I find this approach much better for 2 reasons:

1) The caller has better control over the lock, which makes sense
since it is the one acquiring it.
2) It makes the code much more understandable. Earlier, it took me a
while to figure out exactly where the io_in_progress_lock was getting
released, especially looking at the slotsync patch where it was
acquired right before calling ReplicationSlotPersistInvalidation().

"
PG_CATCH();
{
HOLD_INTERRUPTS();
LWLockRelease(&slot->io_in_progress_lock);
PG_RE_THROW();
}
PG_END_TRY();

LWLockRelease(&slot->io_in_progress_lock);
"

This would avoid both LWLockReleaseAll() and using LWLockHeldByMe() for normal
control flow. Does that sound preferable?

Yes.

thanks
Shveta

Hi,

On Wed, Sep 23, 2026 at 12:00:39PM +0530, shveta malik wrote:

On Wed, Sep 23, 2026 at 11:37 AM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:

Another possibility would be to make ReplicationSlotPersistInvalidation() always
leave the caller acquired I/O lock held. Slotsync could then release that specific
lock in a PG_CATCH() block, something like:

Yes, I agree. I find this approach much better for 2 reasons:

1) The caller has better control over the lock, which makes sense
since it is the one acquiring it.
2) It makes the code much more understandable. Earlier, it took me a
while to figure out exactly where the io_in_progress_lock was getting
released, especially looking at the slotsync patch where it was
acquired right before calling ReplicationSlotPersistInvalidation().

"
PG_CATCH();
{
HOLD_INTERRUPTS();
LWLockRelease(&slot->io_in_progress_lock);
PG_RE_THROW();
}
PG_END_TRY();

LWLockRelease(&slot->io_in_progress_lock);
"

This would avoid both LWLockReleaseAll() and using LWLockHeldByMe() for normal
control flow. Does that sound preferable?

Yes.

Thanks! Done that way in v4 attached.

Regards,

--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

On Wed, Sep 23, 2026 at 2:08 PM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:

Hi,

On Wed, Sep 23, 2026 at 12:00:39PM +0530, shveta malik wrote:

On Wed, Sep 23, 2026 at 11:37 AM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:

Another possibility would be to make ReplicationSlotPersistInvalidation() always
leave the caller acquired I/O lock held. Slotsync could then release that specific
lock in a PG_CATCH() block, something like:

Yes, I agree. I find this approach much better for 2 reasons:

1) The caller has better control over the lock, which makes sense
since it is the one acquiring it.
2) It makes the code much more understandable. Earlier, it took me a
while to figure out exactly where the io_in_progress_lock was getting
released, especially looking at the slotsync patch where it was
acquired right before calling ReplicationSlotPersistInvalidation().

"
PG_CATCH();
{
HOLD_INTERRUPTS();
LWLockRelease(&slot->io_in_progress_lock);
PG_RE_THROW();
}
PG_END_TRY();

LWLockRelease(&slot->io_in_progress_lock);
"

This would avoid both LWLockReleaseAll() and using LWLockHeldByMe() for normal
control flow. Does that sound preferable?

Yes.

Thanks! Done that way in v4 attached.

Can we create the helper function for this logic as it is not core to
the sync-function. Please use the attached patch if you agree, and
feel free to change comments as you see apt.

This approach is better for one more reason: v3 was only releasing the
I/O lock locally during the API call (via slotsync_failure_callback),
leaving the slot-sync worker to rely on top-level proc_exit perhaps.
Current approach ensures consistent lock lifecycle handling for both
the worker and the API.

thanks
Shhveta

Hi,

On Wed, Sep 23, 2026 at 04:07:02PM +0530, shveta malik wrote:

Can we create the helper function for this logic as it is not core to
the sync-function. Please use the attached patch if you agree, and
feel free to change comments as you see apt.

I was initially a bit skeptical about adding a helper with only one caller, as it
seemed to just move the PG_TRY() block elsewhere without reducing duplication.

On second thought, I agree it makes sense here, as it keeps the lock acquisition
and required ERROR cleanup ordering together. There are also similar "one caller"
helpers wrapping PG_TRY() blocks, such as start_table_sync() and start_sequence_sync().

+static void
+persist_slot_invalidation(ReplicationSlot *slot, ReplicationSlotInvalidationCause cause)
+{

I changed it slightly to derive slot from MyReplicationSlot, ensuring that the I/O
lock belongs to the same slot that ReplicationSlotPersistInvalidation() and
and ReplicationSlotRelease() are using. This is also consistent with other operations
on the currently acquired slot.

Please find v5 attached.

Regards,

--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

On Wed, Sep 23, 2026 at 9:35 PM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:

Hi,

On Wed, Sep 23, 2026 at 04:07:02PM +0530, shveta malik wrote:

Can we create the helper function for this logic as it is not core to
the sync-function. Please use the attached patch if you agree, and
feel free to change comments as you see apt.

I was initially a bit skeptical about adding a helper with only one caller, as it
seemed to just move the PG_TRY() block elsewhere without reducing duplication.

On second thought, I agree it makes sense here, as it keeps the lock acquisition
and required ERROR cleanup ordering together. There are also similar "one caller"
helpers wrapping PG_TRY() blocks, such as start_table_sync() and start_sequence_sync().

+static void
+persist_slot_invalidation(ReplicationSlot *slot, ReplicationSlotInvalidationCause cause)
+{

I changed it slightly to derive slot from MyReplicationSlot, ensuring that the I/O
lock belongs to the same slot that ReplicationSlotPersistInvalidation() and
and ReplicationSlotRelease() are using. This is also consistent with other operations
on the currently acquired slot.

Yes, that makes sense.

Please find v5 attached.

Code changes looks good to me. Regarding the test, I have one trivial
comment: I think we should cover all the cases:

a) Failed sync: Memory is NOT updated.
b) Successful sync: Memory IS updated.
c) Server restart: Memory is STILL updated (because it was recovered
from disk).

I think b) is not covered. It will be good to check the memory state
before standby-stop here:

$standby->safe_psql('postgres', 'SELECT pg_sync_replication_slots()');

$standby->stop('immediate');
$standby->start;

thanks
Shveta

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.

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.

thanks
Shveta

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.

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.

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.

Regards,

--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com