Re: Fix slot synchronization with two_phase decoding enabled

On Wed, Apr 30, 2025 at 5:45 AM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>
> On Tue, Apr 29, 2025 at 5:00 AM Nisha Moond <nisha(dot)moond412(at)gmail(dot)com> wrote:
>
> Thank you for updating the patch! Here are some comments on v10.
>

Thanks for reviewing the patch!

> ---
> +
> +# Also wait for two-phase to be enabled
> +$subscriber1->poll_query_until(
> + 'postgres', qq[
> + SELECT count(1) = 0 FROM pg_subscription WHERE subname =
> 'regress_mysub2' and subtwophasestate NOT IN ('e');]
> +) or die "Timed out while waiting for subscriber to enable twophase";
> +
> +# Try to enable the failover for the subscription, should give error
> +($result, $stdout, $stderr) = $subscriber1->psql(
> + 'postgres',
> + "ALTER SUBSCRIPTION regress_mysub2 DISABLE;
> + ALTER SUBSCRIPTION regress_mysub2 SET (failover = true);");
> +ok( $stderr =~
> + qr/ERROR: could not alter replication slot "lsub2_slot": ERROR:
> cannot enable failover for a two-phase enabled replication slot/,
> + "failover can't be enabled if restart_lsn < two_phase_at on a
> two_phase subscription."
> +);
>
> I think it's possible between two tests that the walsender consumes
> some changes (e.g. generated by autovacuums) then the second check
> fails (i.e., ALTER SUBSCRIPTION SET (failover = ture) succeeds).
>

Yes, this is a possibility. To account for this negative test case
where restart_lsn < two_phase_at, I updated the test to attempt
altering a two_phase-enabled slot without any associated subscription.

> ---
> +# Test that SQL API disallows slot creation with both two_phase and
> failover enabled
> +($result, $stdout, $stderr) = $publisher->psql('postgres',
> + "SELECT pg_create_logical_replication_slot('regress_mysub3',
> 'pgoutput', false, true, true);"
> +);
> +ok( $stderr =~
> + /ERROR: cannot enable both "failover" and "two_phase" options
> during replication slot creation/,
> + "cannot create slot with both two_phase and failover enabled");
>
> Isn't this test already covered by test_decoding/sql/slot.sql?
>

Yes, it is covered in slot.sql, hence removed from here.

> I've attached a patch that contains comment changes I mentioned above.
> Please incorporate it if you agree with them.
>

I have incorporated the suggested changes and updated a couple more
places accordingly.
~~~

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,
Nisha