public inbox for [email protected]
help / color / mirror / Atom feedFrom: Masahiko Sawada <[email protected]>
To: Zhijie Hou (Fujitsu) <[email protected]>
Cc: Amit Kapila <[email protected]>
Cc: shveta malik <[email protected]>
Cc: Ajin Cherian <[email protected]>
Cc: Dilip Kumar <[email protected]>
Cc: Nisha Moond <[email protected]>
Cc: Hayato Kuroda (Fujitsu) <[email protected]>
Cc: Peter Smith <[email protected]>
Cc: Bertrand Drouvot <[email protected]>
Cc: Bharath Rupireddy <[email protected]>
Cc: Peter Eisentraut <[email protected]>
Cc: Bruce Momjian <[email protected]>
Cc: Ashutosh Sharma <[email protected]>
Cc: Andres Freund <[email protected]>
Cc: pgsql-hackers <[email protected]>
Cc: Alvaro Herrera <[email protected]>
Subject: Re: Synchronizing slots from primary to standby
Date: Fri, 1 Mar 2024 15:10:57 +0900
Message-ID: <CAD21AoBhty79zHgXYMNHH1KqO2VtmjRi22QPmYP2yaHC9WFVdw@mail.gmail.com> (raw)
In-Reply-To: <OS0PR01MB571669D7F110110F7D87C48D945E2@OS0PR01MB5716.jpnprd01.prod.outlook.com>
References: <CAJpy0uBSbBWAWzb2j3OSSGDwhUBZNDAuMWH9B=A9xtVXdOLnCw@mail.gmail.com>
<Zdcqb3Ok+B/[email protected]>
<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>
<[email protected]>
<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>
On Fri, Mar 1, 2024 at 12:42 PM Zhijie Hou (Fujitsu)
<[email protected]> wrote:
>
> On Friday, March 1, 2024 10:17 AM Zhijie Hou (Fujitsu) <[email protected]> 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 == 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;
^
= 0
1 warning generated.
---
+void
+assign_standby_slot_names(const char *newval, void *extra)
+{
+ List *standby_slots;
+ MemoryContext oldcxt;
+ char *standby_slot_names_cpy = 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 = 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:
=# 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 allocated
+ * (GUCMemoryContext).
+ */
+ oldcxt =
MemoryContextSwitchTo(GetMemoryChunkContext(standby_slot_names_cpy));
+ standby_slot_names_list = 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 == 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':
=# 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 slots
+ * info, there is no way to confirm whether there is any standby
+ * configured for the known physical slots.
+ */
+ if (strcmp(*newval, "*") == 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,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
view thread (68+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: Synchronizing slots from primary to standby
In-Reply-To: <CAD21AoBhty79zHgXYMNHH1KqO2VtmjRi22QPmYP2yaHC9WFVdw@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox