Re: Fix slot synchronization with two_phase decoding enabled

On Wed, May 21, 2025 at 4:02 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Fri, May 9, 2025 at 5:09 AM Nisha Moond <nisha.moond412@gmail.com> wrote:
> >
> > On Thu, May 8, 2025 at 11:35 AM shveta malik <shveta.malik@gmail.com> wrote:
> > >
> > > On Wed, May 7, 2025 at 4:36 PM Nisha Moond <nisha.moond412@gmail.com> wrote:
> > > >
> > > >
> > > > Attached is the v13 patch with the above comments addressed.
> > > >
> > > > --
> > >
> > > Thanks for the patch.
> > >
> > > I think we can have the restriction mentioned under the 'two_phase'
> > > section as well along with the 'failover' section in the CREATE
> > > SUBSCRIPTION doc, similar to what we have in CREATE_REPLICATION_SLOT.
> > >
> > > Apart from this, I have no more comments.
> > >
> >
> > Thanks for the review.
> > Please find the v14 patch, updated with the above suggestion and
> > rephrased changes in alter_subscription.sgml with the missing links.
>
> Thank you for updating the patch. I'm reviewing the v14 patch and will
> share some comments tomorrow.

Here are review comments for v14 patch:

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?

---
+# 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.

---
@@ -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.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com