RE: Optionally automatically disable logical replication subscriptions on error

On Monday, November 29, 2021 2:38 PM vignesh C <vignesh21(at)gmail(dot)com>
> Thanks for the updated patch, Few comments:
Thank you for your review !

> 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); }
Fixed.

> 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)
> +{
Fixed.

> 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)
Fixed.

> 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);
Fixed.

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

The new patch v8 is shared in [1].

[1] - https://www.postgresql.org/message-id/TYCPR01MB83735AA021E0F614A3AB3221ED679%40TYCPR01MB8373.jpnprd01.prod.outlook.com

Best Regards,
Takamichi Osumi