Re: Optionally automatically disable logical replication subscriptions on error

From: Peter Smith <smithpb2250(at)gmail(dot)com>
To: "osumi(dot)takamichi(at)fujitsu(dot)com" <osumi(dot)takamichi(at)fujitsu(dot)com>
Cc: Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>, "tanghy(dot)fnst(at)fujitsu(dot)com" <tanghy(dot)fnst(at)fujitsu(dot)com>, "wangw(dot)fnst(at)fujitsu(dot)com" <wangw(dot)fnst(at)fujitsu(dot)com>, vignesh C <vignesh21(at)gmail(dot)com>, Greg Nancarrow <gregn4422(at)gmail(dot)com>, Mark Dilger <mark(dot)dilger(at)enterprisedb(dot)com>, "Smith, Peter" <peters(at)fast(dot)au(dot)fujitsu(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>
Subject: Re: Optionally automatically disable logical replication subscriptions on error
Date: 2022-03-02 00:34:17
Message-ID: CAHut+PucrizJpqhSyD7dKj1yRkNMskqmiekD_RRqYpdDdusMRQ@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Please see below my review comments for v24.

======

1. src/backend/replication/logical/worker.c - start_table_sync

+ /* Report the worker failed during table synchronization */
+ pgstat_report_subscription_error(MySubscription->oid, !am_tablesync_worker());

(This review comment is just FYI in case you did not do this deliberately)

FYI, you didn't really need to call am_tablesync_worker() here because
it is already asserted for the sync phase that it MUST be a tablesync
worker.

OTOH, IMO it documents the purpose of the parm so if it was deliberate
then that is OK too.

~~~

2. src/backend/replication/logical/worker.c - start_table_sync

+ PG_CATCH();
+ {
+ /*
+ * Abort the current transaction so that we send the stats message in
+ * an idle state.
+ */
+ AbortOutOfAnyTransaction();
+
+ /* Report the worker failed during table synchronization */
+ pgstat_report_subscription_error(MySubscription->oid, !am_tablesync_worker());
+

[Maybe you will say that this review comment is unrelated to
disable_on_err, but since this is a totally new/refactored function
then I think maybe there is no problem to make this change at the same
time. Anyway there is no function change, it is just rearranging some
comments.]

I felt the separation of those 2 statements and comments makes that
code less clean than it could/should be. IMO they should be grouped
together.

SUGGESTED
/*
* Report the worker failed during table synchronization. Abort the
* current transaction so that the stats message is sent in an idle
* state.
*/
AbortOutOfAnyTransaction();
pgstat_report_subscription_error(MySubscription->oid, !am_tablesync_worker());

~~~

3. src/backend/replication/logical/worker.c - start_apply

+ /*
+ * Abort the current transaction so that we send the stats message in
+ * an idle state.
+ */
+ AbortOutOfAnyTransaction();
+
+ /* Report the worker failed during the application of the change */
+ pgstat_report_subscription_error(MySubscription->oid, !am_tablesync_worker());

Same comment as #2 above, but this code fragment is in start_apply function.

~~~

4. src/test/subscription/t/029_disable_on_error.pl - comment

+# Drop the unique index on the sub and re-enabled the subscription.
+# Then, confirm that we have finished the apply.

SUGGESTED (tweak the comment wording)
# Drop the unique index on the sub and re-enable the subscription.
# Then, confirm that the previously failing insert was applied OK.

------
Kind Regards,
Peter Smith.
Fujitsu Australia

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Noboru Saito 2022-03-02 00:57:21 Re: Separate the result of \watch for each query execution (psql)
Previous Message Andreas Karlsson 2022-03-02 00:08:59 Re: speed up a logical replica setup