RE: Optionally automatically disable logical replication subscriptions on error

On Wednesday, December 1, 2021 3:02 PM vignesh C <vignesh21@gmail.com> wrote:
> On Tue, Nov 30, 2021 at 5:34 PM osumi.takamichi@fujitsu.com
> <osumi.takamichi@fujitsu.com> wrote:
> >
> > On Tuesday, November 30, 2021 1:10 PM Greg Nancarrow
> <gregn4422@gmail.com> wrote:
> > > On Sat, Nov 27, 2021 at 1:36 AM osumi.takamichi@fujitsu.com
> > > <osumi.takamichi@fujitsu.com> wrote:
> > > >
> > > > This v7 uses v26 of skip xid patch [1]
> > > This patch no longer applies on the latest source.
> > > Also, the patch is missing an update to doc/src/sgml/catalogs.sgml,
> > > for the new "subdisableonerr" column of pg_subscription.
> > Thanks for your review !
> >
> > Fixed the documentation accordingly. Further, this comment invoked
> > some more refactoring of codes since I wrote some internal codes
> > related to 'disable_on_error' in an inconsistent order.
> > I fixed this by keeping patch's codes
> > after that of 'two_phase' subscription option as much as possible.
> >
> > I also conducted both pgindent and pgperltidy.
> >
> > Now, I'll share the v8 that uses PG
> > whose commit id is after 8d74fc9 (pg_stat_subscription_workers).
> 
> Thanks for the updated patch, few small comments:
I appreciate your check.

> 1) This should be changed:
> +       <structfield>subdisableonerr</structfield> <type>bool</type>
> +      </para>
> +      <para>
> +       If true, the subscription will be disabled when subscription
> +       worker detects an error
> +      </para></entry>
> +     </row>
> 
> to:
> +       <structfield>subdisableonerr</structfield> <type>bool</type>
> +      </para>
> +      <para>
> +       If true, the subscription will be disabled when subscription
> +       worker detects non-transient errors
> +      </para></entry>
> +     </row>
Fixed. Actually, there's no clear definition what "non-transient" means
in the documentation. So, I added some words to your suggestion,
which would give clearer understanding to users.

> 2) "Disable On Err" can be changed to "Disable On Error"
> +                                                         ",
> subtwophasestate AS \"%s\"\n"
> +                                                         ",
> subdisableonerr AS \"%s\"\n",
> +
> gettext_noop("Two phase commit"),
> +
> gettext_noop("Disable On Err"));
Fixed.

> 3) Can add a line in the commit message saying "Bump catalog version."
> as the patch involves changing the catalog.
Hmm, let me postpone this fix till the final version.
The catalog version gets easily updated by other patch commits
and including it in the middle of development can become
cause of conflicts of my patch when applied to the PG,
which is possible to make other reviewers stop reviewing.

> 4) This prototype is not required, since the function is called after the function
> definition:
>  static inline void set_apply_error_context_xact(TransactionId xid,
> TimestampTz ts);  static inline void reset_apply_error_context_info(void);
> +static bool IsTransientError(ErrorData *edata);
Fixed.

> 5) we could use the new style here:
> +       ereport(LOG,
> +                       (errmsg("logical replication subscription
> \"%s\" will be disabled due to error: %s",
> +                                       MySubscription->name,
> + edata->message)));
> 
> change it to:
> +       ereport(LOG,
> +                       errmsg("logical replication subscription
> \"%s\" will be disabled due to error: %s",
> +                                       MySubscription->name,
> + edata->message));
> 
> Similarly it can be changed in the other ereports added.
Removed the unnecessary parentheses.

Best Regards,
    Takamichi Osumi