On Mon, Mar 4, 2024 at 4:52 PM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:
>
> 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!
>
>
> 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?
>
I think we can use but it seems code is easier to follow this way but
this is just a matter of personal choice.
> 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?
>
Yes, I think so. Adding checks based on wait_events sounds a bit awkward.
--
With Regards,
Amit Kapila.