Horton

Re: Newly created replication slot may be invalidated by checkpoint

Amit Kapila <amit.kapila16@gmail.com>

View thread
From: Amit Kapila <amit.kapila16@gmail.com>
To: Masahiko Sawada <sawada.mshk@gmail.com>
Cc: "Zhijie Hou (Fujitsu)" <houzj.fnst@fujitsu.com>, Vitaly Davydov <v.davydov@postgrespro.ru>, "pgsql-hackers@lists.postgresql.org" <pgsql-hackers@lists.postgresql.org>, "suyu.cmj" <mengjuan.cmj@alibaba-inc.com>, tomas <tomas@vondra.me>, michael <michael@paquier.xyz>, "bharath.rupireddyforpostgres" <bharath.rupireddyforpostgres@gmail.com>, Alexander Korotkov <aekorotkov@gmail.com>
Date: 2025-12-04T05:58:03Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Prevent invalidation of newly synced replication slots.

  2. Prevent invalidation of newly created replication slots.

  3. Fix re-distributing previously distributed invalidation messages during logical decoding.

  4. Keep WAL segments by the flushed value of the slot's restart LSN

  5. Keep WAL segments by slot's last saved restart LSN

On Thu, Dec 4, 2025 at 2:04 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Tue, Dec 2, 2025 at 10:15 PM Zhijie Hou (Fujitsu)
> <houzj.fnst@fujitsu.com> wrote:
> >
> > I think the invalidation cannot occur when copying because:
> >
> > Currently, there are no CHECK_FOR_INTERRUPTS() calls between the initial
> > restart_lsn copy (first phase) and the latest restart_lsn copy (second phase).
> > As a result, even if a checkpoint attempts to invalidate a slot and sends a
> > SIGTERM to the backend, the backend will first update the restart_lsn during the
> > second phase before responding to the signal. Consequently, during the next
> > cycle of InvalidatePossiblyObsoleteSlot(), the checkpoint will observe the
> > updated restart_lsn and skip the invalidation.
> >
> > For logical slots, although invoking the output plugin startup callback presents
> > a slight chance of processing the signal (when using third-party plugins), slot
> > invalidation in this scenario results in immediate slot dropping, because the
> > slot is in RS_EPHEMERAL state, thus preventing invalidation.
>
> Thank you for the analysis. I agree.
>
> > While theoretically, slot invalidation could occur if the code changes in the
> > future, addressing that possibility could be considered an independent
> > improvement task. What do you think ?
>
> Okay. I find that it also might make sense for HEAD to use
> RS_EPHEMERAL state for physical slots too to avoid being invalidated
> during creation, which probably can be discussed later. For back
> branches, the proposed idea of acquiring ReplicationSlotAllocationLock
> in an exclusive mode would be better. I think we might want to have a
> comment in CheckPointReplicationSlots() too that refers to
> ReplicationSlotReserveWal().
>
> Regarding whether we revert the original fix 2090edc6f32 and make it
> the same as we did in HEAD ca307d5cec90a4f, we need to change the size
> of ReplicationSlot struct. I'm concerned that it's really safe to
> change it because the data resides on the shared memory. For example,
> we typically iterate over all replication slots as follow:
>
> for (i = 0; i < max_replication_slots; i++)
> {
>     ReplicationSlot *s = &ReplicationSlotCtl->replication_slots[i];
>
> I'm concerned that the arithmetic for calculating the slot address is
> affected by the size of ReplicationSlot change.
>

Yes, this is a valid concern. I think we can go-ahead with fixing the
0001's-fix in HEAD and 18. We can discuss separately the fix for
back-branches prior to 18.

-- 
With Regards,
Amit Kapila.