Re: Optionally automatically disable logical replication subscriptions on error

On Fri, Nov 26, 2021 at 8:06 PM osumi(dot)takamichi(at)fujitsu(dot)com
<osumi(dot)takamichi(at)fujitsu(dot)com> wrote:
>
> On Monday, November 22, 2021 3:53 PM vignesh C <vignesh21(at)gmail(dot)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