Re: Optionally automatically disable logical replication subscriptions on error

On Tue, Nov 30, 2021 at 5:34 PM osumi(dot)takamichi(at)fujitsu(dot)com
<osumi(dot)takamichi(at)fujitsu(dot)com> wrote:
>
> On Tuesday, November 30, 2021 1:10 PM Greg Nancarrow <gregn4422(at)gmail(dot)com> wrote:
> > On Sat, Nov 27, 2021 at 1:36 AM osumi(dot)takamichi(at)fujitsu(dot)com
> > <osumi(dot)takamichi(at)fujitsu(dot)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:
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>

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"));

3) Can add a line in the commit message saying "Bump catalog version."
as the patch involves changing the catalog.

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

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.

Regards,
Vignesh