On Tuesday, December 28, 2021 11:53 AM Wang, Wei/王 威 <wangw.fnst@fujitsu.com> wrote:
> On Thursday, December 16, 2021 8:51 PM osumi.takamichi@fujitsu.com
> <osumi.takamichi@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