Re: Fix slot synchronization with two_phase decoding enabled

On Tue, Apr 22, 2025 at 3:00 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Mon, Apr 21, 2025 at 8:44 AM Zhijie Hou (Fujitsu)
> <houzj.fnst@fujitsu.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.fnst@fujitsu.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".

Regards,

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