Обсуждение: Newly created replication slot may be invalidated by checkpoint

Hi suyu.cmj

> The commit 2090edc6f32f652a2c introduced a change that the
> minimal restart_lsn is obtained at the start of checkpoint creation. If a
> replication slot is created and performs a WAL reservation concurrently, the
> WAL segment contains the new slot's restart_lsn could be removed by the ongoing
> checkpoint.

Thank you for reporting this issue. I agree, the issue with slot invalidation
seems to take place in REL_17_STABLE and earlier, but it is not reproducible in
18+ versions because of different implementation. The problem may appear if
the first persistent slot is created during checkpoint, when slot's oldest lsn
is invalid. I'm not sure how it works when some other persistent slots exist.
Probably, invalidation is still possible if the reservation happens with lsn
older than the oldest lsn of existing slots.

In 17 and earlier verions, when checkpoint is started in takes slot's oldest lsn
using XLogGetReplicationSlotMinimumLSN(). This value will be used later in WAL
segments removal. If a new slot reserved the WAL between getting of slots'
oldest lsn and WAL removal, it may be invalidated. It happens because
ReplicationSlotReserveWal() checks XLogCtl->lastRemovedSegNo but the segments
are not yet removed. There is a subtle thing, when the wal reservation completes
at the same time when the checkpointer is between KeepLogSeg and
RemoveOldXlogFiles where XLogCtl->lastRemovedSegNo is updated. The slot will not
be invalidated but the segments, reserved by the new slot, may be removed, I guess.

In 17 and earlier we tried to create a compatible solution, when oldest lsn was
taken before slot syncing to disk. In the master branch we added a new
last_saved_restart_lsn into ReplicationSlot structure which seems to be a better
solution.

I prepared a simple fix [1] for 17 and earlier versions. It seems it fixes the
problem with first persistent slot creation. I also think, it should work as it
was before the patch that added this bug.

I also did some changes in the original test script, for 17 ([2]) and 18 ([3])
versions.

I continue to investigate and test it.

[1] 0001-Fix-invalidation-when-slot-is-created-during-checkpo.patch
[2] v2-17-0001-Newly-created-replication-slot-may-be-invalidated-by.patch
[3] v2-18-0001-Newly-created-replication-slot-may-be-invalidated-by.patch

With best regards,
Vitaly

On Wed, Sep 17, 2025 at 4:19 PM Vitaly Davydov <v.davydov@postgrespro.ru> wrote:
>
> [1] 0001-Fix-invalidation-when-slot-is-created-during-checkpo.patch
>

- /* Calculate how many segments are kept by slots. */
- keep = slotsMinReqLSN;
+ /*
+ * Calculate how many segments are kept by slots. Keep the wal using
+ * the minimal value from the current reserved LSN and the reserved LSN at
+ * the moment of checkpoint start (before CheckPointReplicationSlots).
+ */
+ keep = XLogGetReplicationSlotMinimumLSN();
+ if (!XLogRecPtrIsInvalid(slotsMinReqLSN))
+ keep = Min(keep, slotsMinReqLSN);

Can we add why we need this additional calculation here?

I have one question regarding commit 2090edc6f32f652a2c:
*
    if (InvalidateObsoleteReplicationSlots(RS_INVAL_WAL_REMOVED,
                                           _logSegNo, InvalidOid,
                                           InvalidTransactionId))
    {
+       /*
+        * Recalculate the current minimum LSN to be used in the WAL segment
+        * cleanup.  Then, we must synchronize the replication slots again in
+        * order to make this LSN safe to use.
+        */
+       slotsMinReqLSN = XLogGetReplicationSlotMinimumLSN();
+       CheckPointReplicationSlots(shutdown);
+
        /*
         * Some slots have been invalidated; recalculate the old-segment
         * horizon, starting again from RedoRecPtr.
         */
        XLByteToSeg(RedoRecPtr, _logSegNo, wal_segment_size);
-       KeepLogSeg(recptr, &_logSegNo);
+       KeepLogSeg(recptr, slotsMinReqLSN, &_logSegNo);

After invalidating the slots, we recalculate the slotsMinReqLSN with
the latest value of XLogGetReplicationSlotMinimumLSN(). Can't it
generate a more recent value of slot's restart_lsn which has not been
flushed and we may end up removing the corresponding WAL? We should
probably add some comments as to why such a race doesn't exist.

--
With Regards,
Amit Kapila.

On Mon, Sep 15, 2025 at 8:11 PM suyu.cmj <mengjuan.cmj@alibaba-inc.com> wrote:
>
>
> Additionally, while studying the InvalidatePossiblyObsoleteSlot(), I noticed a behavioral difference between PG15
(andearlier) and PG16 (and later). In PG15 and earlier, while attempting to acquire a slot, if the slot's restart_lsn
advancedto be greater than oldestLSN, the slot would not be marked invalid. Starting in PG16, whether a slot is marked
invalidis determined solely based on initial_restart_lsn, even if the slot's restart_lsn advances above oldestLSN while
waiting,the slot will still be marked invalid. The initial_restart_lsn is recorded to report the correct invalidation
cause(see discussion [2]), but why not decide whether to mark the slot as invalid based on the slot's current
restart_lsn?If a slot's restart_lsn has already advanced sufficiently, shouldn't we refrain from invalidating it? 
>

I haven't tried to reproduce it but I see your point. I agree that if
this is happening then we should not invalidate such slots. This is a
different problem related to a different commit than what is fixd as
2090edc6f32f652a2c. I suggest you to either start a new thread for
this or report in the original thread where this change was made.

--
With Regards,
Amit Kapila.

On Tue, Sep 23, 2025 at 12:19 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Wed, Sep 17, 2025 at 4:19 PM Vitaly Davydov <v.davydov@postgrespro.ru> wrote:
> >
> > [1] 0001-Fix-invalidation-when-slot-is-created-during-checkpo.patch
> >
>
> - /* Calculate how many segments are kept by slots. */
> - keep = slotsMinReqLSN;
> + /*
> + * Calculate how many segments are kept by slots. Keep the wal using
> + * the minimal value from the current reserved LSN and the reserved LSN at
> + * the moment of checkpoint start (before CheckPointReplicationSlots).
> + */
> + keep = XLogGetReplicationSlotMinimumLSN();
> + if (!XLogRecPtrIsInvalid(slotsMinReqLSN))
> + keep = Min(keep, slotsMinReqLSN);
>
> Can we add why we need this additional calculation here?
>

I was thinking some more about this solution. Won't it lead to the
same problem if ReplicationSlotReserveWal() calls
ReplicationSlotsComputeRequiredLSN() after the above calculation of
checkpointer?

--
With Regards,
Amit Kapila.

Dear Amit, Vitaly,

> I was thinking some more about this solution. Won't it lead to the
> same problem if ReplicationSlotReserveWal() calls
> ReplicationSlotsComputeRequiredLSN() after the above calculation of
> checkpointer?

Exactly. I verified that in your patch, the invalidation can still happen if we
cannot finish the LSN computation before the KeepLogSegments().

Attached file can be applied atop 0001-Fix-invalidation-... and
v2-17-0001-Newly-created-replication... patches. It could invalidate the given
slot.

Best regards,
Hayato Kuroda
FUJITSU LIMITED 

Dear Amit, Hayato

On Wednesday, September 24, 2025 14:31 MSK, "Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com> wrote:

>> I was thinking some more about this solution. Won't it lead to the
>> same problem if ReplicationSlotReserveWal() calls
>> ReplicationSlotsComputeRequiredLSN() after the above calculation of
>> checkpointer?

> Exactly. I verified that in your patch, the invalidation can still happen if
> we cannot finish the LSN computation before the KeepLogSegments().

Yes. The moment, when WAL reservation takes place is the call of
ReplicationSlotsComputeRequiredLSN which updates the oldest slots' lsn
(XLogCtl->replicationSlotMinLSN). If it occurs at the moment between KeepLogSeg
and RemoveOldXlogFiles, such reservation will not be taken into account. This
behaviour seems to be before commit 2090edc6f32f652a2c, but the probability of
such race condition was too slow due to the short time period between KeepLogSeg
and RemoveOldXlogFiles. The commit 2090edc6f32f652a2c increased the probability
of such race condition because CheckPointGuts can take greater time to execute.

The attached patch doesn't solve the original problem completely but it
decreases the probability of such race condition, as it was before the commit.
I propose to apply this patch and then to think how to resolve this race
condition, which seems to take place in 18 and master as well.

I updated the patch by improving some comments as suggested by Amit.

With best regards,
Vitaly

Dear Vitaly,

> I propose to apply this patch and then to think how to resolve this race
> condition, which seems to take place in 18 and master as well.

No, I think this invalidation can't happen in PG18/HEAD.
This is because in CheckpointGuts()->CheckPointReplicationSlots()->
ReplicationSlotsComputeRequiredLSN(), slots are examined one by one to determine
whether their restart_lsn has advanced since the last check. If any slot has
advanced, protection is applied starting from the oldest restart_lsn.
Crucially, this check is performed before WAL removal. The function call was
introduced in commit ca307d5cec.
 
Further analysis shows that it is also safe if a slot is being created and WAL
advances after CheckpointGuts() but before the removal segments are determined.
In this case the restart_lsn points the CHECKPOINT_REDO generated by the current
CHECKPOINT. This and later records won't be discarded.

Best regards,
Hayato Kuroda
FUJITSU LIMITED

Dear Vitaly,

Would you have enough time to work on and fix the issue?
One idea is to compute the required LSN by the system at the slot checkpoint. This
partially follows what PG18/HEAD does but seems hacky and difficult to accept.

Best regards,
Hayato Kuroda
FUJITSU LIMITED

Dear Hayato,

> Would you have enough time to work on and fix the issue?
> One idea is to compute the required LSN by the system at the slot checkpoint. This
> partially follows what PG18/HEAD does but seems hacky and difficult to accept.

I'm working on the issue. Give me, please, a couple of days to finalize my work.

In short, I think to call ReplicationSlotsComputeRequiredLSN() right before
slotsMinReqLSN assignment in CreateCheckPoint in 17 and earlier versions. At
this moment, we already have a new redo lsn. I consider, that the WAL
reservation happens when we assign restart_lsn to a slot. Taking into account
this consideration, I distinguish two cases - WAL reservation happens before or
after new redo ptr assignment. If we reserve the WAL after new redo ptr, it will
protect the slot's reservation, as you've mentioned. The problem appears, when
we reserve the WAL before a new redo ptr, but the update of
XLogCtl->replicationSlotMinLSN was not yet hapenned. When we assign
slotsMinReqLSN, we use XLogCtl->replicationSlotMinLSN. The call of
ReplicationSlotsComputeRequiredLSN before slotsMinReqLSN assignment can help.
It will be guaranteed, that those slots with WAL reservation before a new redo
ptr will be protected by slotsMinReqLSN, but slots with wal reservation after
a new redo ptr will be protected by the redo ptr. I think it is about the same
as you proposed.

These reasonings are applied to physical slots, but it seems to be ok for
logical slots. One moment, I'm not sure, when we create a logical slot in
recovery. In this case, GetXLogReplayRecPtr() is used. I'm not sure, that
redo ptr will protect such slot in CreateRestartPoint.

With best regards,
Vitaly

Dear Vitaly,

> > Would you have enough time to work on and fix the issue?
> > One idea is to compute the required LSN by the system at the slot checkpoint.
> This
> > partially follows what PG18/HEAD does but seems hacky and difficult to accept.
> 
> I'm working on the issue. Give me, please, a couple of days to finalize my work.

Oh, sorry. I was rude.

> In short, I think to call ReplicationSlotsComputeRequiredLSN() right before
> slotsMinReqLSN assignment in CreateCheckPoint in 17 and earlier versions. At
> this moment, we already have a new redo lsn. I consider, that the WAL
> reservation happens when we assign restart_lsn to a slot. Taking into account
> this consideration, I distinguish two cases - WAL reservation happens before or
> after new redo ptr assignment. If we reserve the WAL after new redo ptr, it will
> protect the slot's reservation, as you've mentioned. The problem appears, when
> we reserve the WAL before a new redo ptr, but the update of
> XLogCtl->replicationSlotMinLSN was not yet hapenned. When we assign
> slotsMinReqLSN, we use XLogCtl->replicationSlotMinLSN. The call of
> ReplicationSlotsComputeRequiredLSN before slotsMinReqLSN assignment can
> help.
> It will be guaranteed, that those slots with WAL reservation before a new redo
> ptr will be protected by slotsMinReqLSN, but slots with wal reservation after
> a new redo ptr will be protected by the redo ptr. I think it is about the same
> as you proposed.

Per my understanding, this happened because there is a lag that restart_lsn of
the slot is set, and it is protected by the system. Your idea is to ensure the
restart_lsn is protected by the system before obtaining on-memory LSN, right?

> These reasonings are applied to physical slots, but it seems to be ok for
> logical slots. One moment, I'm not sure, when we create a logical slot in
> recovery. In this case, GetXLogReplayRecPtr() is used. I'm not sure, that
> redo ptr will protect such slot in CreateRestartPoint.

I considered a reproducer for the logical slot on the standby instance. Similar
with the physical one, the injection point while reserving the WAL is used, and
it would be discarded by the restartpoint command.
One difference with physical is that invalidated slot does not retain, because
it is the ephemeral at that time.

After adding the fix [1], I confirmed my testcases are passed, but we should
understand more about the standby stuff.

[1]:
```
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -7675,6 +7675,7 @@ CreateRestartPoint(int flags)
        MemSet(&CheckpointStats, 0, sizeof(CheckpointStats));
        CheckpointStats.ckpt_start_t = GetCurrentTimestamp();
 
+       XLogGetReplicationSlotMinimumLSN();
```

Best regards,
Hayato Kuroda
FUJITSU LIMITED

Dear Hayato, All

On Friday, October 03, 2025 14:14 MSK, "Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com> wrote:
>> I'm working on the issue. Give me, please, a couple of days to finalize my work.
> Oh, sorry. I was rude.
It is okay. I very appreciate your help.

> Per my understanding, this happened because there is a lag that restart_lsn of
> the slot is set, and it is protected by the system. Your idea is to ensure the
> restart_lsn is protected by the system before obtaining on-memory LSN, right?

Not sure what you mean by on-memory LSN, but, the issue happens because we have
a lag between restart_lsn assignment and update of
XLogCtl->replicationSlotsMinLSN which is used to protect the WAL. Yes, I propose
to ensure that the protection happens when we assign restart_lsn. It seems to be
wrong that we invalidate slots by its restart_lsn but protect the wal for
slots using XLogCtl->replicationSlotsMinLSN.

Below I tried to write some summary and propose the patch which fixes the problem.

The issue was originally reported at [1] and it seems to appear in 17 and
earlier versions. The issue is not reproducible in 18+ versions.

The issue may appear when we create a persistent slot during checkpoint. The WAL
reservation in slots happens in ReplicationSlotReserveWal and executed in three
steps:
    1. Assignment of slot->data.restart_lsn
    2. Update of XLogCtl->replicationSlotMinLSN
    3. Check if WAL segments at restart_lsn are removed, go to step 1 if removed.

When the checkpointer calculates the oldest lsn which is used as the lsn horizon
when removing old WAL segments, it takes XLogCtl->replicationSlotMinLSN.
There is a race condition may happen when slot's restart_lsn is already assigned
but XLogCtl->replicationSlotMinLSN is not updated yet. Consider the following
scenario with two processes executing in parallel (checkpointer and backend,
where a new slot is creating):

1. Assign of slot.data->restart_lsn in the backend from GetRedoRecPtr()
2. Assign a new redo LSN in the checkpointer
3. Assign slotsMinReqLSN from XLogCtl->replicationSlotMinLSN in the checkpointer
4. Update of XLogCtl->replicationSlotMinLSN in the backend.
5. Calculation of the WAL horizon for old segments cleanup (KeepLogSeg before
   the call of InvalidateObsoleteReplicationSlots) in the checkpointer.
6. Exit from ReplicationSlotReserveWal in the backend, once the reserved WAL
   segments are not removed at this moment (XLogGetLastRemovedSegno() < segno).
7. Call of InvalidateObsoleteReplicationSlots in the checkpointer will invalidate
   the creating slot because its restart_lsn will be less than the calculated
   WAL horizon (the min of slotsMinReqLSN and RedoRecPtr).

To fix the issue I propose to consider the following assumptions:
1. Slots do not cross WAL segment borders backward when moving.
2. Old WAL segments are removed in the checkpointer only.
3. The following LSNs are initially assigned during slot reservation:
    - GetRedoRecPtr() for physical slots
    - GetXLogInsertRecPtr() for logical slots
    - GetXLogReplayRecPtr() for logical slots in recovery

Taking into account these assumptions, I would like to propose the fix [2].

There is an idea to think that the WAL reservation happens when we assign
restart_lsn to the slot. The call of ReplicationSlotsComputeRequiredLSN() is not
required to be executed immediately in the backend where the slot is creating
concurrently. In the checkpointer we have to guarantee that we do WAL horizon
calculations based on actual values of restart_lsn of existing slots. If
we call ReplicationSlotsComputeRequiredLSN() in the checkpointer after a new
REDO assignment and before the calculation of WAL horizon, the value of
XLogCtl->replicationSlotMinLSN will correctly define the oldest LSN for existing
slots. If the WAL reservation by a new slot happens during checkpoint before
a new REDO assignment, it is guaranteed that its restart_lsn will be accounted
when we call ReplicationSlotsComputeRequiredLSN() in the checkpointer. If the
WAL reservation happens after a new redo LSN assignment, the slot's restart_lsn
will be protected by this new redo LSN, because this LSN will be lesser or equal
to initial restart_lsn (see assumption 3).

There is one subtle thing. Once, the operation of restart_lsn assignment is not
an atomic, the following scenario may happen theoretically:
1. Read GetRedoRecPtr() in the backend (ReplicationSlotReserveWal)
2. Assign a new redo LSN in the checkpointer
3. Call ReplicationSlotsComputeRequiredLSN() in the checkpointer
3. Assign the old redo LSN to restart_lsn

In this scenario, the restart_lsn will point to a previous redo LSN and it will
be not protected by the new redo LSN. This scenario is unlikely, but it can
happen theoretically. I have no ideas how to deal with it, except of assigning
restart_lsn under XLogCtl->info_lck lock to avoid concurrent modification of
XLogCtl->RecoRecPtr until it is assigned to restart_lsn of a creating slot.

In case of recovery, when GetXLogReplayRecPtr() is used, the protection by
redo LSN seems to work as well, because a new redo LSN is taken from the latest
replayed checkpoint. Thus, it is guaranteed that GetXLogReplayRecPtr() will not
be less than the new redo LSN, if it is called right after assignment of redo
LSN in CreateRestartPoint().

I also think that the cycle in ReplicationSlotReserveWal which checks for the
current restart_lsn to be greater than the XLogGetLastRemovedSegno() is not
necessary because it is guaranteed that the assigned restart_lsn will be
protected. Lets keep it unchanged until this suggestion will be clarified.

The proposed solution doesn't break the fix in ca307d5cec (unexpected removal of
old WAL segments after checkpoint). Once we call
ReplicationSlotsComputeRequiredLSN() before CheckPointReplicationSlots(), the
saved to disk restart_lsn values of existing slots will be not less than
the previously computed XLogCtl->replicationSlotMinLSN. They just may be
advanced to greater values concurrently. For new slots with restart_lsn
assignment after ReplicationSlotsComputeRequiredLSN(), the current redo LSN will
protect the WAL.

The fix for REL_17_STABLE is in [2]. The regression test is in [3].

I apologize for so long summary.

[1] https://www.postgresql.org/message-id/flat/15922-68ca9280-4f-37de2c40%40245457797#4b7aa7fe7c57b02105a56ecff06f0b67
[2] v3-0002-Fix-invalidation-when-slot-is-created-during-checkpo.patch
[3] v3-0001-Newly-created-replication-slot-may-be-invalidated-by.patch

With best regards,
Vitaly

Dear Vitaly,

> > Per my understanding, this happened because there is a lag that restart_lsn of
> > the slot is set, and it is protected by the system. Your idea is to ensure the
> > restart_lsn is protected by the system before obtaining on-memory LSN, right?
> 
> Not sure what you mean by on-memory LSN, but, the issue happens because we
> have
> a lag between restart_lsn assignment and update of
> XLogCtl->replicationSlotsMinLSN which is used to protect the WAL.

Sorry I should say "before obtaining replicationSlotMinLSN".

> Yes, I
> propose
> to ensure that the protection happens when we assign restart_lsn. It seems to be
> wrong that we invalidate slots by its restart_lsn but protect the wal for
> slots using XLogCtl->replicationSlotsMinLSN.

Seems valid. There is another corner case that another restart_lsn can be set in-between,
but they have larger LSN than RedoRecPtr, right?

> Below I tried to write some summary and propose the patch which fixes the
> problem.

Sorry but it is too long to understand properly for me :-(.

> There is one subtle thing. Once, the operation of restart_lsn assignment is not
> an atomic, the following scenario may happen theoretically:
> 1. Read GetRedoRecPtr() in the backend (ReplicationSlotReserveWal)
> 2. Assign a new redo LSN in the checkpointer
> 3. Call ReplicationSlotsComputeRequiredLSN() in the checkpointer
> 3. Assign the old redo LSN to restart_lsn
> 
> In this scenario, the restart_lsn will point to a previous redo LSN and it will
> be not protected by the new redo LSN. This scenario is unlikely, but it can
> happen theoretically. I have no ideas how to deal with it, except of assigning
> restart_lsn under XLogCtl->info_lck lock to avoid concurrent modification of
> XLogCtl->RecoRecPtr until it is assigned to restart_lsn of a creating slot.

Oh, your point is there is another race condition, right? Do you have the reproducer for it?

Best regards,
Hayato Kuroda
FUJITSU LIMITED