Re: Fix slot synchronization with two_phase decoding enabled

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 note that the PG17 also suffers from data loss after promotion
due to a bug in fast-forward mode as described in email [1]. So, we
should try to get that fixed as well.

Thanks for your feedback.

[1] - https://www.postgresql.org/message-id/OS0PR01MB57163087F86621D44D9A72BF94BB2%40OS0PR01MB5716.jpnprd01.prod.outlook.com

--
With Regards,
Amit Kapila.