Re: Fix slot synchronization with two_phase decoding enabled

On Wed, Apr 30, 2025 at 5:45 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Tue, Apr 29, 2025 at 5:00 AM Nisha Moond <nisha.moond412@gmail.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