MIME-Version: 1.0
References: 
 <OS0PR01MB5716BB506AF934376FF3A8BB947BA@OS0PR01MB5716.jpnprd01.prod.outlook.com>
 <CAFiTN-uGkrCDi9kqXJqLB+3ATn_ZULmRER73VhkLSDSAnO8SNg@mail.gmail.com>
 <OSCPR01MB149660EB8A874AC3AE777C658F57BA@OSCPR01MB14966.jpnprd01.prod.outlook.com>
 <CAFiTN-uiVOGwPNmDBPmLnC-86hX9-pKEYH2D_OKUmJd5RAUv=g@mail.gmail.com>
 <CAPpHfdsbjXd_iXQwOTS3iioyEmuNz3HdQG8E8tHB0LodN=RXLQ@mail.gmail.com>
 <OSCPR01MB14966C00587EEA20AA28718F1F57AA@OSCPR01MB14966.jpnprd01.prod.outlook.com>
In-Reply-To: 
 <OSCPR01MB14966C00587EEA20AA28718F1F57AA@OSCPR01MB14966.jpnprd01.prod.outlook.com>
From: Alexander Korotkov <aekorotkov@gmail.com>
Date: Thu, 26 Jun 2025 17:48:45 +0300
Message-ID: 
 <CAPpHfdvW1Cbo65UfWcO2uUCXnWosavL9f_UbNRAecTQH4o_RDA@mail.gmail.com>
Subject: Re: Slot's restart_lsn may point to removed WAL segment after hard
 restart unexpectedly
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>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CAPpHfdvW1Cbo65UfWcO2uUCXnWosavL9f_UbNRAecTQH4o_RDA%40mail.gmail.com>
Precedence: bulk

Dear Kuroda-san,

On Thu, Jun 26, 2025 at 6:46=E2=80=AFAM 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_resta=
rt_lsn.
> +                * We need this to know if we need to recompute the requi=
red LSN.
> +                */
> +               if (s->last_saved_restart_lsn !=3D s->data.restart_lsn)
> +                       last_saved_restart_lsn_updated =3D true;
>
> I feel no need to set to true if last_saved_restart_lsn_updated is alread=
y 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