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

On 2020/05/01 10:07, Masahiro Ikeda wrote:
> 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.

Thanks for updating the patch! It looks good to me.

I applied cosmetic changes to the patch (attached). Barring any objection,
I will push this patch (also back-patch to v10 where wait-event for timeline
file was added).

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION