public inbox for [email protected]  

help / color / mirror / Atom feed

From: Zhijie Hou (Fujitsu) <[email protected]>
To: Bertrand Drouvot <[email protected]>
Cc: Amit Kapila <[email protected]>
Cc: Peter Smith <[email protected]>
Cc: shveta malik <[email protected]>
Cc: Masahiko Sawada <[email protected]>
Cc: Ajin Cherian <[email protected]>
Cc: Dilip Kumar <[email protected]>
Cc: Nisha Moond <[email protected]>
Cc: Hayato Kuroda (Fujitsu) <[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: Mon, 4 Mar 2024 13:26:13 +0000
Message-ID: <OS0PR01MB5716AB56177CDE559109238B94232@OS0PR01MB5716.jpnprd01.prod.outlook.com> (raw)
In-Reply-To: <[email protected]>
References: <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>
	<CAHut+PtDo7LRmAQ4CdR7jSscZBvU7O4rsBSGYobSEPuj-93bXw@mail.gmail.com>
	<OS0PR01MB5716B48653125E199A0901D3945D2@OS0PR01MB5716.jpnprd01.prod.outlook.com>
	<CAA4eK1KpFuUPkaFDMXoUOujGVHj4yGWxQ_Xx3DSUU09ieR12Fg@mail.gmail.com>
	<OS0PR01MB5716CD1AB0090155B2ED5C3F945C2@OS0PR01MB5716.jpnprd01.prod.outlook.com>
	<[email protected]>

On Monday, March 4, 2024 7:22 PM Bertrand Drouvot <[email protected]> wrote:
> 
> Hi,
> 
> On Sun, Mar 03, 2024 at 07:56:32AM +0000, Zhijie Hou (Fujitsu) wrote:
> > Here is the V104 patch which addressed above and Peter's comments.
> 
> Thanks!
> 
> A few more random comments:

Thanks for the comments!

> 
> 1 ===
> 
> +        The function may be blocked if the specified slot is a failover
> + enabled
> 
> s/blocked/waiting/ ?

Changed.

> 
> 2 ===
> 
> +                * specified slot when waiting for them to catch up. See
> +                * StandbySlotsHaveCaughtup for details.
> 
> s/StandbySlotsHaveCaughtup/StandbySlotsHaveCaughtup()/ ?

Changed.

> 
> 3 ===
> 
> +       /* Now verify if the specified slots really exist and have
> + correct type */
> 
> remove "really"?

Changed.

> 
> 4 ===
> 
> +       /*
> +        * Don't need to wait for the standbys to catch up if there is no value in
> +        * standby_slot_names.
> +        */
> +       if (standby_slot_names_list == NIL)
> +               return true;
> +
> +       /*
> +        * Don't need to wait for the standbys to catch up if we are on a
> standby
> +        * server, since we do not support syncing slots to cascading standbys.
> +        */
> +       if (RecoveryInProgress())
> +               return true;
> +
> +       /*
> +        * Don't need to wait for the standbys to catch up if they are already
> +        * beyond the specified WAL location.
> +        */
> +       if (!XLogRecPtrIsInvalid(standby_slot_oldest_flush_lsn) &&
> +               standby_slot_oldest_flush_lsn >= wait_for_lsn)
> +               return true;
> 
> What about using OR conditions instead?
> 
> 5 ===
> 
> +static bool
> +NeedToWaitForStandby(XLogRecPtr target_lsn, XLogRecPtr flushed_lsn,
> +                                        uint32 *wait_event)
> 
> Not a big deal but does it need to return a bool? (I mean it all depends of the
> *wait_event value). Is it for better code readability in the caller?
> 
> 6 ===
> 
> +static bool
> +NeedToWaitForWal(XLogRecPtr target_lsn, XLogRecPtr flushed_lsn,
> +                                uint32 *wait_event)
> 
> Same questions as for NeedToWaitForStandby().

I also feel the current style looks a bit cleaner, so didn’t change these.

Best Regards,
Hou zj

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: <OS0PR01MB5716AB56177CDE559109238B94232@OS0PR01MB5716.jpnprd01.prod.outlook.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