MIME-Version: 1.0
References: <CAJpy0uBSbBWAWzb2j3OSSGDwhUBZNDAuMWH9B=A9xtVXdOLnCw@mail.gmail.com>
 <Zdcqb3Ok+B/yOKrl@ip-10-97-1-34.eu-west-3.compute.internal>
 <CAA4eK1KcQSk7wzC7Zfrth9OhrjW2HvxL4tKgU42qqH7p6jn+FA@mail.gmail.com>
 <CAA4eK1Lzt6nT6Vrjq+4FEEVE1toK=9sN6vJrGWCziwW-7k8Xpg@mail.gmail.com>
 <OS0PR01MB5716E563BFFD591D1F414F4294562@OS0PR01MB5716.jpnprd01.prod.outlook.com>
 <OS0PR01MB5716383F68248912EF8D7CE594552@OS0PR01MB5716.jpnprd01.prod.outlook.com>
 <OS0PR01MB57161DE4BDB3194C496546A694552@OS0PR01MB5716.jpnprd01.prod.outlook.com>
 <OS0PR01MB5716E2CCA142FA07A294D98C94552@OS0PR01MB5716.jpnprd01.prod.outlook.com>
 <CAJpy0uAW3GDeCUuOUqdYQymX3TLyjEKQYLUwG9tUgjYJtZYq2w@mail.gmail.com>
 <OS3PR01MB5718FDA975F9694D8BA7B9A794552@OS3PR01MB5718.jpnprd01.prod.outlook.com>
 <Zdh+Z9isbb7FxSl4@ip-10-97-1-34.eu-west-3.compute.internal>
 <CAA4eK1LnLvM2+-2YfQBk6UHYzdEdNtTuhEunxOO3_n-TqDR0ew@mail.gmail.com>
 <OS0PR01MB571654A0288C53A676051AE794592@OS0PR01MB5716.jpnprd01.prod.outlook.com>
 <CAHut+PsGTQb0ijYOfd=x=qX5Aubj=F-pqnfjQ47Jt3MeyBipqA@mail.gmail.com>
 <OS0PR01MB57160590C2CBED48976A113B94582@OS0PR01MB5716.jpnprd01.prod.outlook.com>
 <CAHut+Pvqr5wTK3wpcRzSXxiAmBoRa249w0NMjzUOkJg3TTiLCQ@mail.gmail.com>
In-Reply-To: <CAHut+Pvqr5wTK3wpcRzSXxiAmBoRa249w0NMjzUOkJg3TTiLCQ@mail.gmail.com>
From: Amit Kapila <amit.kapila16@gmail.com>
Date: Thu, 29 Feb 2024 15:23:30 +0530
Message-ID: <CAA4eK1JaHpcS9ZpAGYHSeDQAtLbas55HLqcfnn7n3uWwH-Ko8A@mail.gmail.com>
Subject: Re: Synchronizing slots from primary to standby
To: Peter Smith <smithpb2250@gmail.com>
Cc: "Zhijie Hou (Fujitsu)" <houzj.fnst@fujitsu.com>, shveta malik <shveta.malik@gmail.com>, 
	Masahiko Sawada <sawada.mshk@gmail.com>, Ajin Cherian <itsajin@gmail.com>, 
	Dilip Kumar <dilipbalaut@gmail.com>, Nisha Moond <nisha.moond412@gmail.com>, 
	"Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>, Bertrand Drouvot <bertranddrouvot.pg@gmail.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>, 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/CAA4eK1JaHpcS9ZpAGYHSeDQAtLbas55HLqcfnn7n3uWwH-Ko8A%40mail.gmail.com>
Precedence: bulk

On Thu, Feb 29, 2024 at 8:29=E2=80=AFAM Peter Smith <smithpb2250@gmail.com>=
 wrote:
>
> On Wed, Feb 28, 2024 at 1:23=E2=80=AFPM Zhijie Hou (Fujitsu)
> <houzj.fnst@fujitsu.com> wrote:
> >
> > On Tuesday, February 27, 2024 3:18 PM Peter Smith <smithpb2250@gmail.co=
m> wrote:
> ...
> > > 20.
> > > +#
> > > +# | ----> standby1 (primary_slot_name =3D sb1_slot) # | ----> standb=
y2
> > > +(primary_slot_name =3D sb2_slot) # primary ----- | # | ----> subscri=
ber1
> > > +(failover =3D true) # | ----> subscriber2 (failover =3D false)
> > >
> > > In the diagram, the "--->" means a mixture of physical standbys and l=
ogical
> > > pub/sub replication. Maybe it can be a bit clearer?
> > >
> > > SUGGESTION
> > >
> > > # primary (publisher)
> > > #
> > > #     (physical standbys)
> > > #     | ----> standby1 (primary_slot_name =3D sb1_slot)
> > > #     | ----> standby2 (primary_slot_name =3D sb2_slot)
> > > #
> > > #     (logical replication)
> > > #     | ----> subscriber1 (failover =3D true, slot_name =3D lsub1_slo=
t)
> > > #     | ----> subscriber2 (failover =3D false, slot_name =3D lsub2_sl=
ot)
> > >
> >
> > I think one can distinguish it based on the 'standby' and 'subscriber' =
as well, because
> > 'standby' normally refer to physical standby while the other refer to l=
ogical.
> >

I think Peter's suggestion will make the setup clear.

>
> Ok, but shouldn't it at least include info about the logical slot
> names associated with the subscribers (slot_name =3D lsub1_slot,
> slot_name =3D lsub2_slot) like suggested above?
>
> =3D=3D=3D=3D=3D=3D
>
> Here are some more review comments for v100-0001
>
> =3D=3D=3D=3D=3D=3D
> doc/src/sgml/config.sgml
>
> 1.
> +       <para>
> +        Lists the streaming replication standby server slot names that l=
ogical
> +        WAL sender processes will wait for. Logical WAL sender processes=
 will
> +        send decoded changes to plugins only after the specified replica=
tion
> +        slots confirm receiving WAL. This guarantees that logical replic=
ation
> +        slots with failover enabled do not consume changes until those c=
hanges
> +        are received and flushed to corresponding physical standbys. If =
a
> +        logical replication connection is meant to switch to a physical =
standby
> +        after the standby is promoted, the physical replication slot for=
 the
> +        standby should be listed here. Note that logical replication wil=
l not
> +        proceed if the slots specified in the standby_slot_names do
> not exist or
> +        are invalidated.
> +       </para>
>
> Is that note ("Note that logical replication will not proceed if the
> slots specified in the standby_slot_names do not exist or are
> invalidated") meant only for subscriptions marked for 'failover' or
> any subscription? Maybe wording can be modified to help clarify it?
>

I think it is implicit that here we are talking about failover slots.
I think clarifying again the same could be repetitive considering the
previous sentence: "This guarantees that logical replication slots
with failover enabled do not consume .." have mentioned it.

> =3D=3D=3D=3D=3D=3D
> src/backend/replication/slot.c
>
> 2.
> +/*
> + * A helper function to validate slots specified in GUC standby_slot_nam=
es.
> + */
> +static bool
> +validate_standby_slots(char **newval)
> +{
> + char    *rawname;
> + List    *elemlist;
> + bool ok;
> +
> + /* Need a modifiable copy of string */
> + rawname =3D pstrdup(*newval);
> +
> + /* Verify syntax and parse string into a list of identifiers */
> + ok =3D SplitIdentifierString(rawname, ',', &elemlist);
> +
> + if (!ok)
> + {
> + GUC_check_errdetail("List syntax is invalid.");
> + }
> +
> + /*
> + * If the replication slots' data have been initialized, verify if the
> + * specified slots exist and are logical slots.
> + */
> + else if (ReplicationSlotCtl)
> + {
> + foreach_ptr(char, name, elemlist)
> + {
> + ReplicationSlot *slot;
> +
> + slot =3D SearchNamedReplicationSlot(name, true);
> +
> + if (!slot)
> + {
> + GUC_check_errdetail("replication slot \"%s\" does not exist",
> + name);
> + ok =3D false;
> + break;
> + }
> +
> + if (!SlotIsPhysical(slot))
> + {
> + GUC_check_errdetail("\"%s\" is not a physical replication slot",
> + name);
> + ok =3D false;
> + break;
> + }
> + }
> + }
> +
> + pfree(rawname);
> + list_free(elemlist);
> + return ok;
> +}
>
> 2a.
> I didn't mention this previously because I thought this function was
> not going to change anymore, but since Bertrand suggested some changes
> [1], I will say IMO the { } are fine here for the single statement,
> but I think it will be better to rearrange this code to be like below.
> Having a 2nd NOP 'else' gives a much better place where you can put
> your ReplicationSlotCtl comment.
>
> if (!ok)
> {
>   GUC_check_errdetail("List syntax is invalid.");
> }
> else if (!ReplicationSlotCtl)
> {
>   <Insert big comment here about why it is OK to skip when
> ReplicationSlotCtl is NULL>
> }
> else
> {
>   foreach_ptr ...
> }
>

+1. This will make the code and reasoning to skip clear.

Few additional comments on the latest patch:
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D
1.
 static XLogRecPtr
 WalSndWaitForWal(XLogRecPtr loc)
{
...
+ if (!XLogRecPtrIsInvalid(RecentFlushPtr) && loc <=3D RecentFlushPtr &&
+ (!replication_active || StandbyConfirmedFlush(loc, WARNING)))
+ {
+ /*
+ * Fast path to avoid acquiring the spinlock in case we already know
+ * we have enough WAL available and all the standby servers have
+ * confirmed receipt of WAL up to RecentFlushPtr. This is particularly
+ * interesting if we're far behind.
+ */
  return RecentFlushPtr;
+ }
...
...
+ * Wait for WALs to be flushed to disk only if the postmaster has not
+ * asked us to stop.
+ */
+ if (loc > RecentFlushPtr && !got_STOPPING)
+ wait_event =3D WAIT_EVENT_WAL_SENDER_WAIT_FOR_WAL;
+
+ /*
+ * Check if the standby slots have caught up to the flushed position.
+ * It is good to wait up to RecentFlushPtr and then let it send the
+ * changes to logical subscribers one by one which are already covered
+ * in RecentFlushPtr without needing to wait on every change for
+ * standby confirmation. Note that after receiving the shutdown signal,
+ * an ERROR is reported if any slots are dropped, invalidated, or
+ * inactive. This measure is taken to prevent the walsender from
+ * waiting indefinitely.
+ */
+ else if (replication_active &&
+ !StandbyConfirmedFlush(RecentFlushPtr, WARNING))
+ {
+ wait_event =3D WAIT_EVENT_WAIT_FOR_STANDBY_CONFIRMATION;
+ wait_for_standby =3D true;
+ }
+ else
+ {
+ /* Already caught up and doesn't need to wait for standby_slots. */
  break;
+ }
...
}

Can we try to move these checks into a separate function that returns
a boolean and has an out parameter as wait_event?

2. How about naming StandbyConfirmedFlush() as StandbySlotsAreCaughtup?

--=20
With Regards,
Amit Kapila.