From: Fujii Masao <masao.fujii@oss.nttdata.com>
To: Masahiko Sawada <masahiko.sawada@2ndquadrant.com>
Cc: Drouvot, Bertrand <bdrouvot@amazon.com>
Cc: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Cc: Alvaro Herrera <alvherre@alvh.no-ip.org>
Cc: PostgreSQL-development <pgsql-hackers@postgresql.org>
Subject: Re: Add Information during standby recovery conflicts
Date: Fri, 30 Oct 2020 12:25:10 +0900
Message-ID: <c141f3fc-2cff-a8f5-08b5-0aafa173c949@oss.nttdata.com> (raw)
In-Reply-To: <CA+fd4k6xVsPLvV2YO9PKceCBqdxvxQJL7sPS1nmVA-iAk2n58A@mail.gmail.com>
References: <CA+fd4k4DTjgNXrgbQ7=2hVidG+inzqJUU2BziV=sL7yR0BmC=A@mail.gmail.com>
<20201015.121338.832998229451118046.horikyota.ntt@gmail.com>
<CA+fd4k53s4-P1PuebNgGysO8uHmiitKWxKXcFYgLU07iUx0AsQ@mail.gmail.com>
<20201015.145210.622238854316424234.horikyota.ntt@gmail.com>
<CA+fd4k5JjcphpPw8oorRGAC5v4z-Bn-6dJ9=vRtX3O7gp+5YYg@mail.gmail.com>
<c51826fa-d9d4-4f99-d837-1872ce2d402a@amazon.com>
<CA+fd4k4jOQV51-LJ=B3b7R2mZ+W3AhFkM3XaqyYcqAx_nUk6aw@mail.gmail.com>
<9493cda6-ac77-6aff-28ea-4ff7774ba78c@oss.nttdata.com>
<CA+fd4k6xVsPLvV2YO9PKceCBqdxvxQJL7sPS1nmVA-iAk2n58A@mail.gmail.com>
On 2020/10/30 10:29, Masahiko Sawada wrote:
> ,
>
> On Thu, 29 Oct 2020 at 00:16, Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>>
>>
>>
>> On 2020/10/27 9:41, Masahiko Sawada wrote:
>>> On Tue, 20 Oct 2020 at 22:02, Drouvot, Bertrand <bdrouvot@amazon.com> wrote:
>>>>
>>>> Hi,
>>>>
>>>> On 10/15/20 9:15 AM, Masahiko Sawada wrote:
>>>>> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
>>>>>
>>>>>
>>>>>
>>>>> On Thu, 15 Oct 2020 at 14:52, Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote:
>>>>>> At Thu, 15 Oct 2020 14:28:57 +0900, Masahiko Sawada <masahiko.sawada@2ndquadrant.com> wrote in
>>>>>>>> ereport(..(errmsg("%s", _("hogehoge")))) results in
>>>>>>>> fprintf((translated("%s")), translate("hogehoge")).
>>>>>>>>
>>>>>>>> So your change (errmsg("%s", gettext_noop("hogehoge")) results in
>>>>>>>>
>>>>>>>> fprintf((translated("%s")), DONT_translate("hogehoge")).
>>>>>>>>
>>>>>>>> which leads to a translation problem.
>>>>>>>>
>>>>>>>> (errmsg(gettext_noop("hogehoge"))
>>>>>>> This seems equivalent to (errmsg("hogehoge")), right?
>>>>>> Yes and no. However eventually the two works the same way,
>>>>>> "(errmsg(gettext_noop("hogehoge"))" is a shorthand of
>>>>>>
>>>>>> 1: char *msg = gettext_noop("hogehoge");
>>>>>> ...
>>>>>> 2: .. (errmsg(msg));
>>>>>>
>>>>>> That is, the line 1 only registers a message id "hogehoge" and doesn't
>>>>>> translate. The line 2 tries to translate the content of msg and it
>>>>>> finds the translation for the message id "hogehoge".
>>>>> Understood.
>>>>>
>>>>>>> I think I could understand translation stuff. Given we only report the
>>>>>>> const string returned from get_recovery_conflict_desc() without
>>>>>>> placeholders, the patch needs to use errmsg_internal() instead while
>>>>>>> not changing _() part. (errmsg(get_recovery_conflict_desc())) is not
>>>>>>> good (warned by -Wformat-security).
>>>>>> Ah, right. we get a complain if no value parameters added. We can
>>>>>> silence it by adding a dummy parameter to errmsg, but I'm not sure
>>>>>> which is preferable.
>>>>> Okay, I'm going to use errmsg_internal() for now until a better idea comes.
>>>>>
>>>>> I've attached the updated patch that fixed the translation part.
>>>>
>>>> Thanks for reviewing and helping on this patch!
>>>>
>>>> The patch tester bot is currently failing due to:
>>>>
>>>> "proc.c:1290:5: error: ‘standbyWaitStart’ may be used uninitialized in
>>>> this function [-Werror=maybe-uninitialized]"
>>>>
>>>> I've attached a new version with the minor change to fix it.
>>>>
>>>
>>> Thank you for updating the patch!
>>>
>>> I've looked at the patch and revised a bit the formatting etc.
>>>
>>> After some thoughts, I think it might be better to report the waiting
>>> time as well. it would help users and there is no particular reason
>>> for logging the report only once. It also helps make the patch clean
>>> by reducing the variables such as recovery_conflict_logged. I’ve
>>> implemented it in the v8 patch.
>>
>> I read v8 patch. Here are review comments.
>
> Thank you for your review.
>
>>
>> When recovery conflict with buffer pin happens, log message is output
>> every deadlock_timeout. Is this intentional behavior? If yes, IMO that's
>> not good because lots of messages can be output.
>
> Agreed.
>
> if we were to log the recovery conflict only once in bufferpin
> conflict case, we would log it multiple times only in lock conflict
> case. So I guess it's better to do that in all conflict cases.
Yes, I agree that this behavior basically should be consistent between all cases.
>
>>
>> + if (log_recovery_conflict_waits)
>> + waitStart = GetCurrentTimestamp();
>>
>> What happens if log_recovery_conflict_waits is off at the beginning and
>> then it's changed during waiting for the conflict? In this case, waitStart is
>> zero, but log_recovery_conflict_waits is on. This may cause some problems?
>
> Hmm, I didn't see a path that happens to reload the config file during
> waiting for buffer cleanup lock. Even if the startup process receives
> SIGHUP during that, it calls HandleStartupProcInterrupts() at the next
> convenient time. It could be the beginning of main apply loop or
> inside WaitForWALToBecomeAvailable() and so on but I didn’t see it in
> the wait loop for taking a buffer cleanup.
Yes, you're right. I seem to have read the code wrongly.
> However, I think it’s
> better to use (waitStart > 0) for safety when checking if we log the
> recovery conflict instead of log_recovery_conflict_waits.
>
>>
>> + if (report_waiting)
>> + ts = GetCurrentTimestamp();
>>
>> GetCurrentTimestamp() doesn't need to be called every cycle
>> in the loop after "logged" is true and "new_status" is not NULL.
>
> Agreed
>
>>
>> +extern const char *get_procsignal_reason_desc(ProcSignalReason reason);
>>
>> Is this garbage?
>
> Yes.
>
>>
>> When log_lock_waits is enabled, both "still waiting for ..." and "acquired ..."
>> messages are output. Like this, when log_recovery_conflict_waits is enabled,
>> not only "still waiting ..." but also "resolved ..." message should be output?
>> The latter message might not need to be output if the conflict is canceled
>> due to max_standby_xxx_delay parameter. The latter message would be
>> useful to know how long the recovery was waiting for the conflict. Thought?
>> It's ok to implement this as a separate patch later, though.
>
> There was a discussion that the latter message without waiting time is
> not necessarily needed because the canceled process will log
> "canceling statement due to conflict with XXX" as you mentioned. I
> agreed with that. But I agree that the latter message with waiting
> time would help users, for instance when the startup process is
> waiting for multiple processes and it takes a time to cancel all of
> them.
I agree that it's useful to output the wait time.
But as I told in previous email, it's ok to focus on the current patch
for now and then implement this as a separate patch later.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: masao.fujii@oss.nttdata.com, masahiko.sawada@2ndquadrant.com, bdrouvot@amazon.com, horikyota.ntt@gmail.com, alvherre@alvh.no-ip.org
Subject: Re: Add Information during standby recovery conflicts
In-Reply-To: <c141f3fc-2cff-a8f5-08b5-0aafa173c949@oss.nttdata.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox