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
