On Wed, Sep 27, 2023 at 3:13 PM Drouvot, Bertrand <bertranddrouvot...@gmail.com> wrote: > > On 9/19/23 6:50 AM, shveta malik wrote: > > > > 1) patch001: wait for physical-standby confirmation logic is now > > integrated with WalSndWaitForWal(). Now walsender waits for physical > > standby's confirmation to take changes upto RecentFlushPtr in > > WalSndWaitForWal(). This allows walsender to send the changes to > > logical subscribers one by one which are already covered in > > RecentFlushPtr without needing to wait on every commit for physical > > standby confirmation. > > + /* XXX: Is waiting for 1 second before retrying enough or more or > less? */ > + (void) WaitLatch(MyLatch, > + WL_LATCH_SET | WL_TIMEOUT | > WL_EXIT_ON_PM_DEATH, > + 1000L, > + > WAIT_EVENT_WAL_SENDER_WAIT_FOR_STANDBY_CONFIRMATION); > > I think it would be better to let the physical walsender(s) wake up those > logical > walsender(s) (instead of waiting for 1 sec or such). Maybe we could introduce > a new CV that would > broadcast in PhysicalConfirmReceivedLocation() when restart_lsn is changed, > what do you think? >
Yes, I also think there should be some way for physical walsender to wake up logical walsenders instead of just waiting. By the way, do you think we need a GUC like standby_slot_names (please see discussion [1])? > Still regarding preventing the logical replication to go ahead of > physical replication standbys specified in standby_slot_names: we currently > don't impose this > limitation to pg_logical_slot_get_changes and friends (that don't start a > dedicated walsender). > > Shouldn't we also prevent them to go ahead of physical replication standbys > specified in standby_slot_names? > Yes, I also think similar handling is required in pg_logical_slot_get_changes_guts(). We do call GetFlushRecPtr(), so the handling similar to what the patch is trying to do in WalSndWaitForWal() can be done. [1] - https://www.postgresql.org/message-id/CAJpy0uA%2Bt3XP2M0qtEmrOG1gSwHghjHPno5AtwTXM-94-%2Bc6JQ%40mail.gmail.com -- With Regards, Amit Kapila.
