From: Fujii Masao <masao.fujii@oss.nttdata.com>
To: Masahiro Ikeda <ikedamsh@oss.nttdata.com>
To: Michael Paquier <michael@paquier.xyz>
Cc: pgsql-hackers@postgresql.org
Subject: Re: Why are wait events not reported even though it reads/writes a timeline history file?
Date: Fri, 1 May 2020 00:25:46 +0900
Message-ID: <688e355d-6a70-b127-87ed-0b1fa8403dda@oss.nttdata.com> (raw)
In-Reply-To: <24cc681e7048f984e490f5d01f7ead19@oss.nttdata.com>
References: <d11b0c910b63684424e06772eb844ab5@oss.nttdata.com>
<c2bed73d-2044-8ab1-075a-75ce0316a814@oss.nttdata.com>
<3480dda4f5a043d8819de9e8653dc331@oss.nttdata.com>
<b208e403-e654-b609-3802-7998b21d016e@oss.nttdata.com>
<20200428060958.GG279958@paquier.xyz>
<24cc681e7048f984e490f5d01f7ead19@oss.nttdata.com>
On 2020/04/28 17:42, Masahiro Ikeda wrote:
> On 2020-04-28 15:09, Michael Paquier wrote:
>> On Tue, Apr 28, 2020 at 02:49:00PM +0900, Fujii Masao wrote:
>>> Isn't it safer to report the wait event during fgets() rather than putting
>>> those calls around the whole loop, like other code does? For example,
>>> writeTimeLineHistory() reports the wait event during read() rather than
>>> whole loop.
>>
>> Yeah, I/O wait events should be taken only during the duration of the
>> system calls. Particularly here, you may finish with an elog() that
>> causes the wait event to be set longer than it should, leading to a
>> rather incorrect state if a snapshot of pg_stat_activity is taken.
>> --
>
> Thanks for your comments.
>
> I fixed it to report the wait event during fgets() only.
> Please review the v2 patch I attached.
Thanks for updating the patch! Here are the review comments from me.
+ char *result;
+ pgstat_report_wait_start(WAIT_EVENT_TIMELINE_HISTORY_READ);
+ result = fgets(fline, sizeof(fline), fd);
+ pgstat_report_wait_end();
+ if (result == NULL)
+ break;
+
/* skip leading whitespace and check for # comment */
char *ptr;
Since the variable name "result" has been already used in this function,
it should be renamed.
The code should not be inject into the variable declaration block.
When reading this patch, I found that IO-error in fgets() has not
been checked there. Though this is not the issue that you reported,
but it seems better to fix it together. So what about adding
the following code?
if (ferror(fd))
ereport(ERROR,
(errcode_for_file_access(),
errmsg("could not read file \"%s\": %m", path)));
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, ikedamsh@oss.nttdata.com, michael@paquier.xyz
Subject: Re: Why are wait events not reported even though it reads/writes a timeline history file?
In-Reply-To: <688e355d-6a70-b127-87ed-0b1fa8403dda@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