Re: Fix slot synchronization with two_phase decoding enabled

On Thu, Apr 24, 2025 at 2:24 AM Nisha Moond <nisha(dot)moond412(at)gmail(dot)com> wrote:
>
> On Thu, Apr 24, 2025 at 12:28 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >
> > On Wed, Apr 23, 2025 at 11:04 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> > >
> > > On Tue, Apr 22, 2025 at 3:00 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > > >
> > > > On Mon, Apr 21, 2025 at 8:44 AM Zhijie Hou (Fujitsu)
> > > > <houzj(dot)fnst(at)fujitsu(dot)com> wrote:
> > > > >
> > > > > On Sat, Apr 19, 2025 at 2:19 AM Masahiko Sawada wrote:
> > > > > >
> > > > > > On Tue, Apr 8, 2025 at 10:14 PM Zhijie Hou (Fujitsu)
> > > > > > <houzj(dot)fnst(at)fujitsu(dot)com> wrote:
> > > > > > >
> > > > > > >
> > > > > > > ----------
> > > > > > > Approach 2
> > > > > > > ----------
> > > > > > >
> > > > > > > Instead of disallowing the use of two-phase and failover together, a more
> > > > > > > flexible strategy could be only restrict failover for slots with two-phase
> > > > > > > enabled when there's a possibility of existing prepared transactions before
> > > > > > the
> > > > > > > two_phase_at that are not yet replicated. During slot creation with
> > > > > > two-phase
> > > > > > > and failover, we could check for any decoded prepared transactions when
> > > > > > > determining the decoding start point (DecodingContextFindStartpoint). For
> > > > > > > subsequent attempts to alter failover to true, we ensure that two_phase_at is
> > > > > > > less than restart_lsn, indicating that all prepared transactions have been
> > > > > > > committed and replicated, thus the bug would not happen.
> > > > > > >
> > > > > > > pros:
> > > > > > >
> > > > > > > This method minimizes restrictions for users. Especially during slot creation
> > > > > > > with (two_phase=on, failover=on), as it’s uncommon for transactions to
> > > > > > prepare
> > > > > > > during consistent snapshot creation, the restriction becomes almost
> > > > > > > unnoticeable.
> > > > > >
> > > > > > I think this approach can work for the transactions that are prepared
> > > > > > while the slot is created. But if I understand the problem correctly,
> > > > > > while the initial table sync is performing, the slot's two_phase is
> > > > > > still false, so we need to deal with the transactions that are
> > > > > > prepared during the initial table sync too. What do you think?
> > > > > >
> > > > >
> > > > > Yes, I agree that we need to restrict this case too. Given that we haven't
> > > > > started decoding when setting two_phase=true during CreateDecodingContext()
> > > > > after tablesync, we could check prepared transactions afterwards during
> > > > > decoding. This could involve reporting an ERROR when skipping a prepared
> > > > > transaction during decoding if its prepare LSN is less than two_phase_at.
> > > > >
> > > >
> > > > It will make it difficult for users to detect it as this happens at a
> > > > later point of time.
> > > >
> > > > > Alternatively, a simpler method would be to prevent this situation entirely
> > > > > during the CREATE SUBSCRIPTION command. For example, we could restrict slots
> > > > > created with failover set to true and twophase is later modified to true after
> > > > > tablesync. Although the simpler check is more user-visible, it may offer less
> > > > > flexibility.
> > > > >
> > > >
> > > > I agree with your point, but OTOH, I am also afraid of adding too many
> > > > smart checks in the back-branch. If we follow what you say here, then
> > > > users have the following ways in PG17 to enable both failover and
> > > > two_phase. (a) During Create Subscription, users can set both
> > > > 'failover' and 'two_phase', if 'copy_data' is false, or (b), if
> > > > 'copy_data' is true, during Create Subscription, then users can enable
> > > > 'two_phase' and wait for it to be enabled. Then use Alter Subscription
> > > > to set 'failover'.
> > >
> > > Yet another idea would be to disallow enabling both two_phase and
> > > failover at CREATE SUBSCRIPTION regardless of copy_data value and to
> > > add check when enabling failover for the two_phase-enabled-slots. For
> > > example, users who want to enable both need to do two steps:
> > >
> > > 1. CREATE SUBSCRIPTION with two_phase = true and failover = false.
> > > 2. ALTER SUBSCRIPTION with failover = true.
> > >
> > > At ALTER SUBSCRIPTION with failover = true, the subscriber checks if
> > > the two_phase states is ready (and possibly check if the slot's
> > > two_phase has been enabled too), otherwise raises an ERROR. Then, when
> > > the publisher enables the failover for the two_phase-enabled-slot up
> > > on walrcv_alter_slot() request, it checks the slot's restart_lsn has
> > > passed slot's two_phase_at, otherwise raise an error with the message
> > > like "the slot need to consume change upto %X/%X to enable failover".
> > >
> >
> > This should further simplify the checks with an additional restriction
> > during the CREATE SUBSCRIPTION time. I am in favor of it because I
> > want the code in this area to be as much same in HEAD and backbranches
> > as possible.
> >
>
> Please find the updated patch for Approach 3, which implements the
> idea suggested by Swada-san above.

Thank you for the patch! I think we need to update the documentation
as well. I'll also review the patch shortly so could you please
prepare the documentation changes?

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com