Subject: Re: Why are wait events not reported even though it reads/writes a
 timeline history file?
To: Masahiro Ikeda <ikedamsh@oss.nttdata.com>,
 Michael Paquier <michael@paquier.xyz>
Cc: pgsql-hackers@postgresql.org
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>
From: Fujii Masao <masao.fujii@oss.nttdata.com>
Message-ID: <688e355d-6a70-b127-87ed-0b1fa8403dda@oss.nttdata.com>
Date: Fri, 1 May 2020 00:25:46 +0900
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:68.0)
 Gecko/20100101 Thunderbird/68.7.0
MIME-Version: 1.0
In-Reply-To: <24cc681e7048f984e490f5d01f7ead19@oss.nttdata.com>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Precedence: bulk


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