Re: Optionally automatically disable logical replication subscriptions on error - Mailing list pgsql-hackers

On Fri, Nov 26, 2021 at 8:06 PM osumi.takamichi@fujitsu.com
<osumi.takamichi@fujitsu.com> wrote:
>
> On Monday, November 22, 2021 3:53 PM vignesh C <vignesh21@gmail.com> wrote:
> > Few comments:
> Thank you so much for your review !
>
> > 1) Changes to handle pg_dump are missing. It should be done in
> > dumpSubscription and getSubscriptions
> Fixed.
>
> > 2) "And" is missing
> > --- a/doc/src/sgml/ref/alter_subscription.sgml
> > +++ b/doc/src/sgml/ref/alter_subscription.sgml
> > @@ -201,8 +201,8 @@ ALTER SUBSCRIPTION <replaceable
> > class="parameter">name</replaceable> RENAME TO <
> >        information.  The parameters that can be altered
> >        are <literal>slot_name</literal>,
> >        <literal>synchronous_commit</literal>,
> > -      <literal>binary</literal>, and
> > -      <literal>streaming</literal>.
> > +      <literal>binary</literal>,<literal>streaming</literal>
> > +      <literal>disable_on_error</literal>.
> > Should be:
> > -      <literal>binary</literal>, and
> > -      <literal>streaming</literal>.
> > +      <literal>binary</literal>,<literal>streaming</literal>, and
> > +      <literal>disable_on_error</literal>.
> Fixed.
>
> > 3) Should we change this :
> > +          Specifies whether the subscription should be automatically
> > disabled
> > +          if replicating data from the publisher triggers non-transient errors
> > +          such as referential integrity or permissions errors. The default is
> > +          <literal>false</literal>.
> > to:
> > +          Specifies whether the subscription should be automatically
> > disabled
> > +          while replicating data from the publisher triggers
> > non-transient errors
> > +          such as referential integrity, permissions errors, etc. The
> > default is
> > +          <literal>false</literal>.
> I preferred the previous description. The option
> "disable_on_error" works with even one error.
> If we use "while", the nuance would be like
> we keep disabling a subscription more than once.
> This situation happens only when user makes
> the subscription enable without resolving the non-transient error,
> which sounds a bit unnatural. So, I wanna keep the previous description.
> If you are not satisfied with this, kindly let me know.
>
> This v7 uses v26 of skip xid patch [1]

Thanks for the updated patch, Few comments:
1) Since this function is used only from 027_disable_on_error and not
used by others, this can be moved to 027_disable_on_error:
+sub wait_for_subscriptions
+{
+       my ($self, $dbname, @subscriptions) = @_;
+
+       # Unique-ify the subscriptions passed by the caller
+       my %unique = map { $_ => 1 } @subscriptions;
+       my @unique = sort keys %unique;
+       my $unique_count = scalar(@unique);
+
+       # Construct a SQL list from the unique subscription names
+       my @escaped = map { s/'/''/g; s/\\/\\\\/g; $_ } @unique;
+       my $sublist = join(', ', map { "'$_'" } @escaped);
+
+       my $polling_sql = qq(
+               SELECT COUNT(1) = $unique_count FROM
+                       (SELECT s.oid
+                               FROM pg_catalog.pg_subscription s
+                               LEFT JOIN pg_catalog.pg_subscription_rel sr
+                               ON sr.srsubid = s.oid
+                               WHERE (sr IS NULL OR sr.srsubstate IN
('s', 'r'))
+                                 AND s.subname IN ($sublist)
+                                 AND s.subenabled IS TRUE
+                        UNION
+                        SELECT s.oid
+                               FROM pg_catalog.pg_subscription s
+                               WHERE s.subname IN ($sublist)
+                                 AND s.subenabled IS FALSE
+                       ) AS synced_or_disabled
+               );
+       return $self->poll_query_until($dbname, $polling_sql);
+}

2) The empty line after comment is not required, it can be removed
+# Create non-unique data in both schemas on the publisher.
+#
+for $schema (@schemas)
+{

3) Similarly it can be changed across the file
+# Wait for the initial subscription synchronizations to finish or fail.
+#
+$node_subscriber->wait_for_subscriptions('postgres', @schemas)
+       or die "Timed out while waiting for subscriber to synchronize data";

+# Enter unique data for both schemas on the publisher.  This should succeed on
+# the publisher node, and not cause any additional problems on the subscriber
+# side either, though disabled subscription "s1" should not replicate anything.
+#
+for $schema (@schemas)

4) Since subid is used only at one place, no need of subid variable,
you could replace subid with subform->oid in LockSharedObject
+       Datum           values[Natts_pg_subscription];
+       HeapTuple       tup;
+       Oid                     subid;
+       Form_pg_subscription subform;

+       subid = subform->oid;
+       LockSharedObject(SubscriptionRelationId, subid, 0, AccessExclusiveLock);

5) "permissions errors" should be "permission errors"
+          Specifies whether the subscription should be automatically disabled
+          if replicating data from the publisher triggers non-transient errors
+          such as referential integrity or permissions errors. The default is
+          <literal>false</literal>.
+         </para>

Regards,
Vignesh