Re: Fix slot synchronization with two_phase decoding enabled

On Fri, May 2, 2025 at 3:05 PM shveta malik <shveta.malik@gmail.com> wrote:
>
> On Fri, May 2, 2025 at 12:57 PM Nisha Moond <nisha.moond412@gmail.com> wrote:
> >
> >
> > Please find the v11 patch addressing the above points and all other
> > comments. I have also optimized the test by reducing the number of
> > subscriptions and slots.
> >
>
> Thanks for the patch. Few comments:
>
> 1)
>
> pg_upgrade/t/003_logical_slots.pl:
>
> - "SELECT slot_name, two_phase, failover FROM pg_replication_slots");
> -is($result, qq(regress_sub|t|t), 'check the slot exists on new cluster');
> + "SELECT slot_name, two_phase FROM pg_replication_slots");
> +is($result, qq(regress_sub|t), 'check the slot exists on new cluster');
>
> With this change, the failover property test during upgrade is
> removed. Shall we add it again either using ALTER SUB to enable
> failover and two_phase together or a new subscription with failover
> alone?
>

If ALTER SUB is used to set failover for the existing subscription,
then pg_upgrade will fail while trying to create a slot with both
failover and two_phase on the upgraded node $newpub.
So, I've implemented the other suggestion by adding a separate
pub-subs to verify the failover property.

> 2)
> +          The default is false. This option cannot be set together with
> +          <literal>failover</literal> when creating a logical replication slot.
> +          However, once the slot is created with <literal>two_phase =
> true</literal>,
> +          <literal>failover</literal> can be set to true after the slot has
> +          consumed all transactions up to the point where two-phase decoding
> +          was enabled.
>
>
> Suggestion:  all transactions --> all the changes.
>

Done.
~~~

Please find the v12 patch with above suggested changes.

--
Thanks,
Nisha