RE: Optionally automatically disable logical replication subscriptions on error

On Monday, December 13, 2021 6:57 PM vignesh C <vignesh21@gmail.com> wrote:
> On Mon, Dec 6, 2021 at 4:22 PM osumi.takamichi@fujitsu.com
> <osumi.takamichi@fujitsu.com> wrote:
> >
> > I've attached the new version v12.
I appreciate your review.


> Thanks for the updated patch, few comments:
> 1) This is not required as it is not used in the caller.
> +++ b/src/backend/replication/logical/launcher.c
> @@ -132,6 +132,7 @@ get_subscription_list(void)
>                 sub->dbid = subform->subdbid;
>                 sub->owner = subform->subowner;
>                 sub->enabled = subform->subenabled;
> +               sub->disableonerr = subform->subdisableonerr;
>                 sub->name = pstrdup(NameStr(subform->subname));
>                 /* We don't fill fields we are not interested in. */
Okay.
The comment of the get_subscription_list() mentions that
we collect and fill only fields related to worker start/stop.
Then, I didn't need it. Fixed.


> 2) Should this be changed:
> +       <structfield>subdisableonerr</structfield> <type>bool</type>
> +      </para>
> +      <para>
> +       If true, the subscription will be disabled when subscription
> +       worker detects any errors
> +      </para></entry>
> +     </row>
> To:
> +       <structfield>subdisableonerr</structfield> <type>bool</type>
> +      </para>
> +      <para>
> +       If true, the subscription will be disabled when subscription's
> +       worker detects any errors
> +      </para></entry>
> +     </row>
I felt either is fine. So fixed.


> 3) The last line can be slightly adjusted to keep within 80 chars:
> +          Specifies whether the subscription should be automatically disabled
> +          if any errors are detected by subscription workers during data
> +          replication from the publisher. The default is
> <literal>false</literal>.
Fixed.

> 4) Similarly this too can be handled:
> --- a/src/backend/catalog/system_views.sql
> +++ b/src/backend/catalog/system_views.sql
> @@ -1259,7 +1259,7 @@ REVOKE ALL ON pg_replication_origin_status FROM
> public;
>  -- All columns of pg_subscription except subconninfo are publicly readable.
>  REVOKE ALL ON pg_subscription FROM public;  GRANT SELECT (oid,
> subdbid, subname, subowner, subenabled, subbinary,
> -              substream, subtwophasestate, subslotname,
> subsynccommit, subpublications)
> +              substream, subtwophasestate, subdisableonerr,
> subslotname, subsynccommit, subpublications)
>      ON pg_subscription TO public;
I split the line into two to make each line less than 80 chars.

> 5) Since disabling subscription code is common in and else, can we move it
> below:
> +                       if (MySubscription->disableonerr)
> +                       {
> +                               WorkerErrorRecovery();
> +                               error_recovery_done = true;
> +                       }
> +                       else
> +                       {
> +                               /*
> +                                * Some work in error recovery work is
> done. Switch to the old
> +                                * memory context and rethrow.
> +                                */
> +                               MemoryContextSwitchTo(ecxt);
> +                               PG_RE_THROW();
> +                       }
> +               }
> +               else
> +               {
> +                       /*
> +                        * Don't miss any error, even when it's not
> reported to stats
> +                        * collector.
> +                        */
> +                       if (MySubscription->disableonerr)
> +                       {
> +                               WorkerErrorRecovery();
> +                               error_recovery_done = true;
> +                       }
> +                       else
> +                               /* Simply rethrow because of no recovery
> work */
> +                               PG_RE_THROW();
> +               }
I moved the common code below those condition branches.


> 6) Can we move LockSharedObject below the if condition.
> +       subform = (Form_pg_subscription) GETSTRUCT(tup);
> +       LockSharedObject(SubscriptionRelationId, subform->oid, 0,
> AccessExclusiveLock);
> +
> +       /*
> +        * We would not be here unless this subscription's
> disableonerr field was
> +        * true when our worker began applying changes, but check whether
> that
> +        * field has changed in the interim.
> +        */
> +       if (!subform->subdisableonerr)
> +       {
> +               /*
> +                * Disabling the subscription has been done already. No need
> of
> +                * additional work.
> +                */
> +               heap_freetuple(tup);
> +               table_close(rel, RowExclusiveLock);
> +               CommitTransactionCommand();
> +               return;
> +       }
> +
Fixed.

Besides all of those changes, I've removed the obsolete
comment of DisableSubscriptionOnError in v12.

Best Regards,
    Takamichi Osumi