On Monday, March 4, 2024 7:22 PM Bertrand Drouvot <bertranddrouvot...@gmail.com> 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
