On Wed, Mar 7, 2018 at 11:13 PM, Alvaro Herrera <alvhe...@alvh.no-ip.org> wrote: > 0001: > > there are a bunch of other messages of the same ilk in the file. I > don't like how the current messages are worded; maybe Peter or Petr had > some reason why they're like that, but I would have split out the reason > for not starting or stopping into errdetail. Something like > > errmsg("logical replication apply worker for subscription \"%s\" will not > start", ...) > errdetail("Subscription has been disabled during startup.") > > But I think we should change all these messages in unison rather than > only one of them. > > Now, your patch changes "will not start" to "will stop". But is that > correct? ISTM that this happens when a worker is starting and > determines that it is not needed. So "will not start" is more correct. > "Will stop" would be correct if the worker had been running for some > time and suddenly decided to terminate, but that doesn't seem to be the > case, unless I misread the code. > > Your patch also changes "disabled during startup" to just "disabled". > Is that a useful change? ISTM that if the subscription had been > disabled prior to startup, then the worker would not have started at > all, so we would not be seeing this message in the first place. Again, > maybe I am misreading the code? Please explain.
I think you're not misreading the code. I agree with you. "will not start" and "disabled during startup" would be more appropriate here. But Petr might have other opinion on this. Also I noticed I overlooked one change of v1 patch proposed by Petr. Attached a new patch incorporated your review comment and the hunk I overlooked. Regards, -- Masahiko Sawada NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center
Description: Binary data