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