Re: Optionally automatically disable logical replication subscriptions on error
От | Peter Smith |
---|---|
Тема | Re: Optionally automatically disable logical replication subscriptions on error |
Дата | |
Msg-id | CAHut+PucrizJpqhSyD7dKj1yRkNMskqmiekD_RRqYpdDdusMRQ@mail.gmail.com обсуждение исходный текст |
Ответ на | RE: Optionally automatically disable logical replication subscriptions on error ("osumi.takamichi@fujitsu.com" <osumi.takamichi@fujitsu.com>) |
Ответы |
RE: Optionally automatically disable logical replication subscriptions on error
("osumi.takamichi@fujitsu.com" <osumi.takamichi@fujitsu.com>)
Re: Optionally automatically disable logical replication subscriptions on error (Masahiko Sawada <sawada.mshk@gmail.com>) |
Список | 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
В списке pgsql-hackers по дате отправления:
Следующее
От: Noboru SaitoДата:
Сообщение: Re: Separate the result of \watch for each query execution (psql)