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.


Masahiko Sawada
NTT Open Source Software Center

Attachment: 0001-Improve-messaging-during-logical-replication-worker-_v3.patch
Description: Binary data

Reply via email to