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

On Monday, November 29, 2021 2:38 PM vignesh C <vignesh21@gmail.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