Re: Synchronizing slots from primary to standby

On Mon, Mar 4, 2024 at 2:38 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Mon, Mar 4, 2024 at 6:57 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
> >
> > On Sun, Mar 3, 2024 at 2:56 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > >
> > > On Sun, Mar 3, 2024 at 5:17 AM Peter Smith <smithpb2250(at)gmail(dot)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.
>

OK, if the code will remain as-is wouldn't it be better to anyway
change the function name to indicate what it really does?

e.g. NeedToWaitForWal --> NeedToWaitForWalFlushOrStandbys

----------
Kind Regards,
Peter Smith.
Fujitsu Australia