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