Horton

Re: Slot's restart_lsn may point to removed WAL segment after hard restart unexpectedly

Alexander Korotkov <aekorotkov@gmail.com>

View thread
From: Alexander Korotkov <aekorotkov@gmail.com>
To: "Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>
Cc: Dilip Kumar <dilipbalaut@gmail.com>, "Zhijie Hou (Fujitsu)" <houzj.fnst@fujitsu.com>, Amit Kapila <amit.kapila16@gmail.com>, Vitaly Davydov <v.davydov@postgrespro.ru>, Tom Lane <tgl@sss.pgh.pa.us>, Alexander Lakhin <exclusion@gmail.com>, pgsql-hackers <pgsql-hackers@postgresql.org>, Masahiko Sawada <sawada.mshk@gmail.com>, "tomas@vondra.me" <tomas@vondra.me>, vignesh C <vignesh21@gmail.com>
Date: 2025-06-26T14:48:45Z
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. Update comment for ReplicationSlot.last_saved_restart_lsn

  2. Fix CheckPointReplicationSlots() with max_replication_slots == 0

  3. Remove excess assert from InvalidatePossiblyObsoleteSlot()

  4. Improve runtime and output of tests for replication slots checkpointing.

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

  6. Fix an assert in CheckPointReplicationSlots().

Dear Kuroda-san,

On Thu, Jun 26, 2025 at 6:46 AM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> Dear Alexander,
> >
> > Good idea.  But I think we should associate the "updated" flag
> > directly to the fact that one slot (no matter logical or physical)
> > changed its last_saved_restart_lsn.  See the attached patch.  I'm
> > going to push it if no objections.
>
> +               /*
> +                * Track if we're going to update slot's last_saved_restart_lsn.
> +                * We need this to know if we need to recompute the required LSN.
> +                */
> +               if (s->last_saved_restart_lsn != s->data.restart_lsn)
> +                       last_saved_restart_lsn_updated = true;
>
> I feel no need to set to true if last_saved_restart_lsn_updated is already true.
> Other than that it's OK for me.

Thank you for your feedback.

Regarding last_saved_restart_lsn_updated, I think the opposite.  I
think we should check if last_saved_restart_lsn_updated is set already
only if it could promise us some economy of resources.  In our case
the main check only compares two fields of slot.  And that fields are
to be accessed anyway.  So, we are not going to save any RAM accesses.
Therefore, checking for last_saved_restart_lsn_updated seems like
unnecessary code complication (and I don't see we're doing that in
other places).  So, I'm going to push this patch "as is".



------
Regards,
Alexander Korotkov
Supabase