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
