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