On Tuesday, March 5, 2024 2:35 PM shveta malik <shveta.ma...@gmail.com> wrote: > > On Tue, Mar 5, 2024 at 9:15 AM Amit Kapila <amit.kapil...@gmail.com> wrote: > > > > On Tue, Mar 5, 2024 at 6:10 AM Peter Smith <smithpb2...@gmail.com> > wrote: > > > > > > ====== > > > src/backend/replication/walsender.c > > > > > > 5. NeedToWaitForWal > > > > > > + /* > > > + * Check if the standby slots have caught up to the flushed > > > + position. It > > > + * is good to wait up to the flushed position and then let the > > > + WalSender > > > + * send the changes to logical subscribers one by one which are > > > + already > > > + * covered by the flushed position without needing to wait on every > > > + change > > > + * for standby confirmation. > > > + */ > > > + if (NeedToWaitForStandbys(flushed_lsn, wait_event)) return true; > > > + > > > + *wait_event = 0; > > > + return false; > > > +} > > > + > > > > > > 5a. > > > The comment (or part of it?) seems misplaced because it is talking > > > WalSender sending changes, but that is not happening in this function. > > > > > > > I don't think so. This is invoked only by walsender and a static > > function. I don't see any other better place to mention this. > > > > > Also, isn't what this is saying already described by the other > > > comment in the caller? e.g.: > > > > > > > Oh no, here we are explaining the wait order. > > I think there is a scope of improvement here. The comment inside > NeedToWaitForWal() which states that we need to wait here for standbys on > flush-position(and not on each change) should be outside of this function. It > is > too embedded. And the comment which states the order of wait (first flush and > then standbys confirmation) should be outside the for-loop in > WalSndWaitForWal(), but yes we do need both the comments. Attached a > patch (.txt) for comments improvement, please merge if appropriate.
Thanks, I have slightly modified the top-up patch and merged it. Attach the V106 patch which addressed above and Peter's comments[1]. [1] https://www.postgresql.org/message-id/CAHut%2BPsATK8z1TEcfFE8zWoS1hagqsvaWYCgom_zYtScfwO7uQ%40mail.gmail.com Best Regards, Hou zj
v106-0001-Allow-logical-walsenders-to-wait-for-the-physic.patch
Description: v106-0001-Allow-logical-walsenders-to-wait-for-the-physic.patch
v106-0002-Document-the-steps-to-check-if-the-standby-is-r.patch
Description: v106-0002-Document-the-steps-to-check-if-the-standby-is-r.patch
