On Tuesday, December 28, 2021 11:53 AM Wang, Wei/王 威 <wangw.f...@fujitsu.com> wrote: > On Thursday, December 16, 2021 8:51 PM osumi.takami...@fujitsu.com > <osumi.takami...@fujitsu.com> wrote: > > Attached the updated patch v14. > > A comment to the timing of printing a log: Thank you for your review !
> After the log[1] was printed, I altered subscription's option
> (DISABLE_ON_ERROR) from true to false before invoking
> DisableSubscriptionOnError to disable subscription. Subscription was not
> disabled.
> [1] "LOG: logical replication subscription "sub1" will be disabled due to an
> error"
>
> I found this log is printed in function WorkerErrorRecovery:
> + ereport(LOG,
> + errmsg("logical replication subscription \"%s\" will
> be disabled due to an error",
> + MySubscription->name));
> This log is printed here, but in DisableSubscriptionOnError, there is a check
> to
> confirm subscription's disableonerr field. If disableonerr is found changed
> from
> true to false in DisableSubscriptionOnError, subscription will not be
> disabled.
>
> In this case, "disable subscription" is printed, but subscription will not be
> disabled actually.
> I think it is a little confused to user, so what about moving this message
> after
> the check which is mentioned above in DisableSubscriptionOnError?
Makes sense. I moved the log print after
the check of the necessity to disable the subscription.
Also, I've scrutinized and refined the new TAP test as well for refactoring.
As a result, I fixed wait_for_subscriptions()
so that some extra codes that can be simplified,
such as escaped variable and one part of WHERE clause, are removed.
Other change I did is to replace two calls of wait_for_subscriptions()
with polling_query_until() for the subscriber, in order to
make the tests better and more suitable for the test purposes.
Again, for this refinement, I've conducted a tight loop test
to check no timing issue and found no problem.
Best Regards,
Takamichi Osumi
v15-0001-Optionally-disable-subscriptions-on-error.patch
Description: v15-0001-Optionally-disable-subscriptions-on-error.patch
