From: | Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> |
---|---|
To: | Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> |
Cc: | Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com>, Petr Jelinek <petr(dot)jelinek(at)2ndquadrant(dot)com>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: [HACKERS] Subscription code improvements |
Date: | 2018-03-08 00:42:13 |
Message-ID: | CAD21AoBX9LPFWRXR3fJxq=kJd_gW+Tgn0WS3qHXqHQ54exTyqw@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Wed, Mar 7, 2018 at 11:13 PM, Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)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
Attachment | Content-Type | Size |
---|---|---|
0001-Improve-messaging-during-logical-replication-worker-_v3.patch | application/octet-stream | 1.7 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Tomas Vondra | 2018-03-08 00:42:20 | Re: [HACKERS] Lazy hash table for XidInMVCCSnapshot (helps Zipfian a bit) |
Previous Message | Tsunakawa, Takayuki | 2018-03-08 00:28:04 | RE: Protect syscache from bloating with negative cache entries |