Re: Why are wait events not reported even though it reads/writes atimeline history file?

On 2020-05-01 00:25, Fujii Masao wrote:
> 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.

Sorry for that.

I thought to rename it, but I changed to use feof()
for clarify the difference from ferror().


> The code should not be inject into the variable declaration block.

Thanks for the comment.
I moved the code block after 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)));

Thanks, I agree your comment.
I added the above code to the v3 patch I attached.


Regards,
-- 
Masahiro Ikeda
NTT DATA CORPORATION