On Sun, May 25, 2025 at 11:34 PM Nisha Moond <nisha.moond412@gmail.com> wrote:
>
> to
> On Fri, May 23, 2025 at 10:06 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> >
> > Here are review comments for v14 patch:
> >
>
> Thank you for the review.
>
> > I think we need to include a basic test case where we simply create a
> > subscription with two_phase=true and then enable the failover via
> > ALTER SUBSCRIPTION after making sure that slot's restart_lsn passed
> > two_phase_at. Probably if we use this case in 003_logical_slots.pl, we
> > can avoid creating regress_sub2?
> >
>
> A test has been added in 040_standby_failover_slots_sync.pl to enable
> failover via ALTER SUBSCRIPTION.
Yes but the slot is originally created via SQL API, which seems
uncommon usage in practice. I thought it would be good to have the
basic steps in the tests to enable both fields.
> Regarding regress_sub2 in 003_logical_slots.pl :
> If we use ALTER SUBSCRIPTION to set failover=true on regress_sub, it
> leads to pg_upgrade failure, as it attempts to create a slot on the
> new_node(upgraded version) with both two_phase and failover enabled,
> which is an unsupported combination.
I think that the pg_upgrade test should cover the case where it
restores logical slots with both fields enabled in the new cluster.
When I actually tried this case, I realized that pg_upgrade doesn't
handle this case; it tried to create the logical slot via SQL API but
it failed as we don't allow it to create a logical slot with enabling
both two_phase and failover. How can we handle this case?
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
