On Wed, Sep 27, 2023 at 3:13 PM Drouvot, Bertrand
<bertranddrouvot.pg@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.