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>
 <CAA4eK1JaHpcS9ZpAGYHSeDQAtLbas55HLqcfnn7n3uWwH-Ko8A@mail.gmail.com>
 <CAA4eK1KC9tb0AUKKnkqyuXXc-B89KQDk5NEWxhm15-dPG2pt3g@mail.gmail.com>
 <OS0PR01MB5716E281A55E5053DF4976D1945E2@OS0PR01MB5716.jpnprd01.prod.outlook.com>
 <OS0PR01MB571669D7F110110F7D87C48D945E2@OS0PR01MB5716.jpnprd01.prod.outlook.com>
In-Reply-To: <OS0PR01MB571669D7F110110F7D87C48D945E2@OS0PR01MB5716.jpnprd01.prod.outlook.com>
From: Masahiko Sawada <sawada.mshk@gmail.com>
Date: Fri, 1 Mar 2024 15:10:57 +0900
Message-ID: <CAD21AoBhty79zHgXYMNHH1KqO2VtmjRi22QPmYP2yaHC9WFVdw@mail.gmail.com>
Subject: Re: Synchronizing slots from primary to standby
To: "Zhijie Hou (Fujitsu)" <houzj.fnst@fujitsu.com>
Cc: Amit Kapila <amit.kapila16@gmail.com>, shveta malik <shveta.malik@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>, Peter Smith <smithpb2250@gmail.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/CAD21AoBhty79zHgXYMNHH1KqO2VtmjRi22QPmYP2yaHC9WFVdw%40mail.gmail.com>
Precedence: bulk

On Fri, Mar 1, 2024 at 12:42=E2=80=AFPM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:
>
> On Friday, March 1, 2024 10:17 AM Zhijie Hou (Fujitsu) <houzj.fnst@fujits=
u.com> wrote:
> >
> >
> > Attach the V102 patch set which addressed Amit and Shveta's comments.
> > Thanks Shveta for helping addressing the comments off-list.
>
> The cfbot reported a compile warning, here is the new version patch which=
 fixed it,
> Also removed some outdate comments in this version.
>

Thank you for updating the patch!

I've reviewed the v102-0001 patch. Here are some comments:

---
I got a compiler warning:

walsender.c:1829:6: warning: variable 'wait_event' is used
uninitialized whenever '&&' condition is false
[-Wsometimes-uninitialized]
        if (!XLogRecPtrIsInvalid(RecentFlushPtr) &&
            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
walsender.c:1871:7: note: uninitialized use occurs here
                if (wait_event =3D=3D WAIT_EVENT_WAL_SENDER_WAIT_FOR_WAL)
                    ^~~~~~~~~~
walsender.c:1829:6: note: remove the '&&' if its condition is always true
        if (!XLogRecPtrIsInvalid(RecentFlushPtr) &&
            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
walsender.c:1818:20: note: initialize the variable 'wait_event' to
silence this warning
        uint32          wait_event;
                                  ^
                                   =3D 0
1 warning generated.

---
+void
+assign_standby_slot_names(const char *newval, void *extra)
+{
+        List      *standby_slots;
+        MemoryContext oldcxt;
+        char      *standby_slot_names_cpy =3D extra;
+

Given that the newval and extra have the same data (standby_slot_names
value), why do we not use newval instead? I think that if we use
newval, we don't need to guc_strdup() in check_standby_slot_names(),
we might need to do list_copy_deep() instead, though. It's not clear
to me as there is no comment.

---
+
+        standby_slot_oldest_flush_lsn =3D min_restart_lsn;
+

IIUC we expect that standby_slot_oldest_flush_lsn never moves
backward. If so, I think it's better to have an assertion here.

---
Resetting standby_slot_names doesn't work:

=3D# alter system set standby_slot_names to '';
ERROR:  invalid value for parameter "standby_slot_names": """"
DETAIL:  replication slot "" does not exist

---
+        /*
+         * Switch to the memory context under which GUC variables are allo=
cated
+         * (GUCMemoryContext).
+         */
+        oldcxt =3D
MemoryContextSwitchTo(GetMemoryChunkContext(standby_slot_names_cpy));
+        standby_slot_names_list =3D list_copy(standby_slots);
+        MemoryContextSwitchTo(oldcxt);

Why do we not explicitly switch to GUCMemoryContext?

---
+        if (!standby_slot_names_list)
+                return true;
+

Probably "standby_slot_names_list =3D=3D NIL" is more consistent with
other places. The same can be applied in WaitForStandbyConfirmation().

---
+        /*
+         * Return true if all the standbys have already caught up to
the passed in
+         * WAL localtion.
+         */
+

s/localtion/location/

---
I was a bit surprised by the fact that standby_slot_names value is
handled in a different way than a similar parameter
synchronous_standby_names. For example, the following command doesn't
work unless there is a replication slot 'slot1, slot2':

=3D# alter system set standby_slot_names to 'slot1, slot2';
ERROR:  invalid value for parameter "standby_slot_names": ""slot1, slot2""
DETAIL:  replication slot "slot1, slot2" does not exist

Whereas "alter system set synchronous_standby_names to stb1, stb2;"
can correctly separate the string into 'stb1' and 'stb2'.

Probably it would be okay since this behavior of standby_slot_names is
the same as other GUC parameters that accept a comma-separated string.
But I was confused a bit the first time I used it.

---
+        /*
+         * "*" is not accepted as in that case primary will not be able to=
 know
+         * for which all standbys to wait for. Even if we have physical sl=
ots
+         * info, there is no way to confirm whether there is any standby
+         * configured for the known physical slots.
+         */
+        if (strcmp(*newval, "*") =3D=3D 0)
+        {
+                GUC_check_errdetail("\"*\" is not accepted for
standby_slot_names");
+                return false;
+        }

Why only '*' is checked aside from validate_standby_slots()? I think
that the doc doesn't mention anything about '*' and '*' cannot be used
as a replication slot name. So even if we don't have this check, it
might be no problem.

Regards,

--=20
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com