RE: Optionally automatically disable logical replication subscriptions on error

On Monday, December 13, 2021 6:57 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
> On Mon, Dec 6, 2021 at 4:22 PM osumi(dot)takamichi(at)fujitsu(dot)com
> <osumi(dot)takamichi(at)fujitsu(dot)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