On Sun, May 25, 2025 at 11:34 PM Nisha Moond <nisha.moond...@gmail.com> wrote: > > to > On Fri, May 23, 2025 at 10:06 PM Masahiko Sawada <sawada.m...@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
