On Mon, Mar 4, 2024 at 6:57 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> On Sun, Mar 3, 2024 at 2:56 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Sun, Mar 3, 2024 at 5:17 AM Peter Smith <smithpb2250@gmail.com> wrote:
> > >
> ...
> > > 9. NeedToWaitForWal
> > >
> > > + /*
> > > + * Check if the standby slots have caught up to the flushed position. It
> > > + * is good to wait up to flushed position and then let it send the changes
> > > + * to logical subscribers one by one which are already covered in flushed
> > > + * position without needing to wait on every change for standby
> > > + * confirmation. Note that after receiving the shutdown signal, an ERROR
> > > + * is reported if any slots are dropped, invalidated, or inactive. This
> > > + * measure is taken to prevent the walsender from waiting indefinitely.
> > > + */
> > > + if (NeedToWaitForStandby(target_lsn, flushed_lsn, wait_event))
> > > + return true;
> > >
> > > I felt it was confusing things for this function to also call to the
> > > other one -- it seems an overlapping/muddling of the purpose of these.
> > > I think it will be easier to understand if the calling code just calls
> > > to one or both of these functions as required.
> > >
> >
> > I felt otherwise because the caller has to call these functions at
> > more than one place which makes the caller's code difficult to follow.
> > It is better to encapsulate the computation of wait_event.
> >
>
> You may have misinterpreted my review comment because I didn't say
> anything about changing the encapsulation of the computation of the
> wait_event.
>
No, I have understood it in the same way as you have outlined in this
email and liked the way the current patch has it.
--
With Regards,
Amit Kapila.