MIME-Version: 1.0
References: <CAA4eK1+AFY4E0f9h0-JF-navrf-vRMTMrN6H7PDKiR=C6p84Nw@mail.gmail.com>
 <CAJpy0uCcFKiofcp5V_hRvHrY_qVNX+eKvB0CPDNxSk2FbyMJCA@mail.gmail.com>
 <OS0PR01MB5716E731AC1B97A96B014B929460A@OS0PR01MB5716.jpnprd01.prod.outlook.com>
 <CAJpy0uDhR=m2Pr76aJZnvtKZpCEiLbMbjbPReCv5fS0MA3ch2w@mail.gmail.com>
 <CAJpy0uDWgSBUsTav1Eeo7E7i38GjXr_o0WdWamm3jGdctcoC4A@mail.gmail.com>
 <ZZa4pLFCe2mAks1m@ip-10-97-1-34.eu-west-3.compute.internal>
 <CAJpy0uBGSjwhZTBo6MxT6TkwO2AX5fa1kw7UC3RQLiapUJReuw@mail.gmail.com>
 <CAFiTN-vTaWUh0Eq8UpETMk=ZGFEMkD-1gXtosODZeCiCfi92kQ@mail.gmail.com>
 <CAA4eK1KLR2BP1GULkA9FMj3aKFbOcQLvu6b5fAht+YjCVRkc1w@mail.gmail.com>
 <CAFiTN-skn=ffyC8PMzg7gfpyNLfx7v9roae-+W8HYbpzoO0mkg@mail.gmail.com>
 <OS0PR01MB57169DD55EC8D9D1EDB7A0C2946A2@OS0PR01MB5716.jpnprd01.prod.outlook.com>
 <CAA4eK1L74EUU7+Dr11joY+Xact_MsjZu9G12sX+vRUWJsptP8Q@mail.gmail.com>
In-Reply-To: <CAA4eK1L74EUU7+Dr11joY+Xact_MsjZu9G12sX+vRUWJsptP8Q@mail.gmail.com>
From: Amit Kapila <amit.kapila16@gmail.com>
Date: Thu, 11 Jan 2024 16:22:56 +0530
Message-ID: <CAA4eK1+ORhnb2zUUpO_FcgWUUbwvQX5+=tdCehx7x2nB2x8VMg@mail.gmail.com>
Subject: Re: Synchronizing slots from primary to standby
To: "Zhijie Hou (Fujitsu)" <houzj.fnst@fujitsu.com>, Masahiko Sawada <sawada.mshk@gmail.com>, 
	Dilip Kumar <dilipbalaut@gmail.com>, Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Cc: shveta malik <shveta.malik@gmail.com>, Peter Smith <smithpb2250@gmail.com>, 
	Nisha Moond <nisha.moond412@gmail.com>, 
	"Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>, 
	Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>, 
	Peter Eisentraut <peter.eisentraut@enterprisedb.com>, Bruce Momjian <bruce@momjian.us>, 
	Ashutosh Sharma <ashu.coek88@gmail.com>, Andres Freund <andres@anarazel.de>, 
	pgsql-hackers <pgsql-hackers@postgresql.org>, Ajin Cherian <itsajin@gmail.com>, 
	Alvaro Herrera <alvherre@alvh.no-ip.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: <https://www.postgresql.org/message-id/CAA4eK1%2BORhnb2zUUpO_FcgWUUbwvQX5%2B%3DtdCehx7x2nB2x8VMg%40mail.gmail.com>
Precedence: bulk

On Tue, Jan 9, 2024 at 6:39=E2=80=AFPM Amit Kapila <amit.kapila16@gmail.com=
> wrote:
>
> +static bool
> +synchronize_one_slot(WalReceiverConn *wrconn, RemoteSlot *remote_slot)
> {
> ...
> + /* Slot ready for sync, so sync it. */
> + else
> + {
> + /*
> + * Sanity check: With hot_standby_feedback enabled and
> + * invalidations handled appropriately as above, this should never
> + * happen.
> + */
> + if (remote_slot->restart_lsn < slot->data.restart_lsn)
> + elog(ERROR,
> + "cannot synchronize local slot \"%s\" LSN(%X/%X)"
> + " to remote slot's LSN(%X/%X) as synchronization"
> + " would move it backwards", remote_slot->name,
> + LSN_FORMAT_ARGS(slot->data.restart_lsn),
> + LSN_FORMAT_ARGS(remote_slot->restart_lsn));
> ...
> }
>
> I was thinking about the above code in the patch and as far as I can
> think this can only occur if the same name slot is re-created with
> prior restart_lsn after the existing slot is dropped. Normally, the
> newly created slot (with the same name) will have higher restart_lsn
> but one can mimic it by copying some older slot by using
> pg_copy_logical_replication_slot().
>
> I don't think as mentioned in comments even if hot_standby_feedback is
> temporarily set to off, the above shouldn't happen. It can only lead
> to invalidated slots on standby.
>
> To close the above race, I could think of the following ways:
> 1. Drop and re-create the slot.
> 2. Emit LOG/WARNING in this case and once remote_slot's LSN moves
> ahead of local_slot's LSN then we can update it; but as mentioned in
> your previous comment, we need to update all other fields as well. If
> we follow this then we probably need to have a check for catalog_xmin
> as well.
>

The second point as mentioned is slightly misleading, so let me try to
rephrase it once again: Emit LOG/WARNING in this case and once
remote_slot's LSN moves ahead of local_slot's LSN then we can update
it; additionally, we need to update all other fields like two_phase as
well. If we follow this then we probably need to have a check for
catalog_xmin as well along remote_slot's restart_lsn.

> Now, related to this the other case which needs some handling is what
> if the remote_slot's restart_lsn is greater than local_slot's
> restart_lsn but it is a re-created slot with the same name. In that
> case, I think the other properties like 'two_phase', 'plugin' could be
> different. So, is simply copying those sufficient or do we need to do
> something else as well?
>

Bertrand, Dilip, Sawada-San, and others, please share your opinion on
this problem as I think it is important to handle this race condition.

--=20
With Regards,
Amit Kapila.