From: Fujii Masao <masao.fujii@oss.nttdata.com>
To: Masahiko Sawada <masahiko.sawada@2ndquadrant.com>
To: 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: Thu, 29 Oct 2020 00:16:22 +0900
Message-ID: <9493cda6-ac77-6aff-28ea-4ff7774ba78c@oss.nttdata.com> (raw)
In-Reply-To: <CA+fd4k4jOQV51-LJ=B3b7R2mZ+W3AhFkM3XaqyYcqAx_nUk6aw@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>
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.
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.
+ 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?
+ 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.
+extern const char *get_procsignal_reason_desc(ProcSignalReason reason);
Is this garbage?
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.
+ Controls whether a log message is produced when the startup process
+ is waiting longer than longer than <varname>deadlock_timeout</varname>.
Typo: "longer than longer than" should be "longer than".
Probably the section about hot standby in high-availability.sgml would need
to be updated.
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: <9493cda6-ac77-6aff-28ea-4ff7774ba78c@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