Re: Fix slot synchronization with two_phase decoding enabled

to
On Fri, May 23, 2025 at 10:06 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)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.
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.

> ---
> +# Advance the slot's restart_lsn to allow enabling the failover option
> +# on a two_phase-enabled subscription using ALTER SUBSCRIPTION.
> +$publisher->safe_psql(
> + 'postgres', qq(
> + BEGIN;
> + SELECT txid_current();
> + SELECT pg_log_standby_snapshot();
> + COMMIT;
> + BEGIN;
> + SELECT txid_current();
> + SELECT pg_log_standby_snapshot();
> + COMMIT;
> +));
> +
> +# Alter subscription to enable failover
> +$subscriber1->psql(
> + 'postgres',
> + "ALTER SUBSCRIPTION regress_mysub2 DISABLE;
> + ALTER SUBSCRIPTION regress_mysub2 SET (failover = true);");
>
> I think we need to wait for the subscription to consume these changes
> before disabling the subscription. If the publisher doesn't consume
> these WAL records for some reason, the subsequent "ALTER SUBSCRIPTION
> ... SET (failover = true)" will fail.
>

Done.

> ---
> @@ -25,6 +25,8 @@ $publisher->init(
> $publisher->append_conf('postgresql.conf', 'autovacuum = off');
> $publisher->start;
>
> +$publisher->safe_psql('postgres', "CREATE TABLE tab_full (a int)");
> +
> $publisher->safe_psql('postgres',
> "CREATE PUBLICATION regress_mypub FOR ALL TABLES;");
>
> @@ -42,6 +44,8 @@ my $slot_creation_time_on_primary = $publisher->safe_psql(
> SELECT current_timestamp;
> ]);
>
> +$subscriber1->safe_psql('postgres', "CREATE TABLE tab_full (a int)");
> +
> # Create a subscription that enables failover.
> $subscriber1->safe_psql('postgres',
> "CREATE SUBSCRIPTION regress_mysub1 CONNECTION
> '$publisher_connstr' PUBLICATION regress_mypub WITH (slot_name =
> lsub1_slot, copy_data = false, failover = true, enabled = false);"
> @@ -98,6 +102,114 @@ my ($result, $stdout, $stderr) =
> $subscriber1->psql('postgres',
> ok( $stderr =~ /ERROR: cannot set failover for enabled subscription/,
> "altering failover is not allowed for enabled subscription");
>
> I think it's better to create the tables before the new test starts
> with a comment explaining why we need to create the table here.
>

Done.
~~~

Please find the updated patch v15, addressing above comments.

--
Thanks,
Nisha