MIME-Version: 1.0
Content-Type: multipart/mixed;
 boundary="=_2d21b17bc047f832055378e377553f2b"
Date: Fri, 01 May 2020 10:07:21 +0900
From: Masahiro Ikeda <ikedamsh@oss.nttdata.com>
To: Fujii Masao <masao.fujii@oss.nttdata.com>
Cc: Michael Paquier <michael@paquier.xyz>, pgsql-hackers@postgresql.org
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>
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>
 <688e355d-6a70-b127-87ed-0b1fa8403dda@oss.nttdata.com>
Message-ID: <eb6997cd4131d8beab9f39dc79abb015@oss.nttdata.com>
User-Agent: Roundcube Webmail/1.1.9
Precedence: bulk

--=_2d21b17bc047f832055378e377553f2b
Content-Transfer-Encoding: 8bit
Content-Type: text/plain; charset=UTF-8;
 format=flowed

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
--=_2d21b17bc047f832055378e377553f2b
Content-Transfer-Encoding: base64
Content-Type: text/x-diff;
 name=v3-0001-Fix-to-report-wait-events-about-timeline-history-.patch
Content-Disposition: attachment;
 filename=v3-0001-Fix-to-report-wait-events-about-timeline-history-.patch;
 size=2004

RnJvbSA5NzZhNzk2YzFmZjdlZDE5MWQ5ZjNkZjZjMTExMWEzMzNlNzAwZTNhIE1vbiBTZXAgMTcg
MDA6MDA6MDAgMjAwMQpGcm9tOiBNYXNhaGlybyBJa2VkYSA8aWtlZGFtc2hAb3NzLm50dGRhdGEu
Y29tPgpEYXRlOiBGcmksIDEgTWF5IDIwMjAgMDk6Mzg6NTEgKzA5MDAKU3ViamVjdDogW1BBVENI
IHYzXSBGaXggdG8gcmVwb3J0IHdhaXQgZXZlbnRzIGFib3V0IHRpbWVsaW5lIGhpc3RvcnkgZmls
ZS4KCkV2ZW4gdGhvdWdoIGEgdGltZWxpbmUgaGlzdG9yeSBmaWxlIGlzIHJlYWQgb3Igd3JpdHRl
biwKc29tZSB3YWl0IGV2ZW50cyBhcmUgbm90IHJlcG9ydGVkLiBUaGlzIHBhdGNoIGZpeGVzIHRo
b3NlIGlzc3Vlcy4KLS0tCiBzcmMvYmFja2VuZC9hY2Nlc3MvdHJhbnNhbS90aW1lbGluZS5jIHwg
MTUgKysrKysrKysrKysrKystCiAxIGZpbGUgY2hhbmdlZCwgMTQgaW5zZXJ0aW9ucygrKSwgMSBk
ZWxldGlvbigtKQoKZGlmZiAtLWdpdCBhL3NyYy9iYWNrZW5kL2FjY2Vzcy90cmFuc2FtL3RpbWVs
aW5lLmMgYi9zcmMvYmFja2VuZC9hY2Nlc3MvdHJhbnNhbS90aW1lbGluZS5jCmluZGV4IGRlNTdk
Njk5YWYuLjRkZmU5ZmRkZDEgMTAwNjQ0Ci0tLSBhL3NyYy9iYWNrZW5kL2FjY2Vzcy90cmFuc2Ft
L3RpbWVsaW5lLmMKKysrIGIvc3JjL2JhY2tlbmQvYWNjZXNzL3RyYW5zYW0vdGltZWxpbmUuYwpA
QCAtMTIzLDcgKzEyMyw3IEBAIHJlYWRUaW1lTGluZUhpc3RvcnkoVGltZUxpbmVJRCB0YXJnZXRU
TEkpCiAJICogUGFyc2UgdGhlIGZpbGUuLi4KIAkgKi8KIAlwcmV2ZW5kID0gSW52YWxpZFhMb2dS
ZWNQdHI7Ci0Jd2hpbGUgKGZnZXRzKGZsaW5lLCBzaXplb2YoZmxpbmUpLCBmZCkgIT0gTlVMTCkK
Kwlmb3IgKDs7KQogCXsKIAkJLyogc2tpcCBsZWFkaW5nIHdoaXRlc3BhY2UgYW5kIGNoZWNrIGZv
ciAjIGNvbW1lbnQgKi8KIAkJY2hhcgkgICAqcHRyOwpAQCAtMTMyLDYgKzEzMiwxNyBAQCByZWFk
VGltZUxpbmVIaXN0b3J5KFRpbWVMaW5lSUQgdGFyZ2V0VExJKQogCQl1aW50MzIJCXN3aXRjaHBv
aW50X2xvOwogCQlpbnQJCQluZmllbGRzOwogCisJCXBnc3RhdF9yZXBvcnRfd2FpdF9zdGFydChX
QUlUX0VWRU5UX1RJTUVMSU5FX0hJU1RPUllfUkVBRCk7CisJCWZnZXRzKGZsaW5lLCBzaXplb2Yo
ZmxpbmUpLCBmZCk7CisJCXBnc3RhdF9yZXBvcnRfd2FpdF9lbmQoKTsKKworCQlpZiAoZmVvZihm
ZCkpCisJCQlicmVhazsKKwkJZWxzZSBpZiAoZmVycm9yKGZkKSkKKwkJCWVyZXBvcnQoRVJST1Is
CisJCQkJKGVycmNvZGVfZm9yX2ZpbGVfYWNjZXNzKCksCisJCQkJZXJybXNnKCJjb3VsZCBub3Qg
cmVhZCBmaWxlIFwiJXNcIjogJW0iLCBwYXRoKSkpOworCiAJCWZvciAocHRyID0gZmxpbmU7ICpw
dHI7IHB0cisrKQogCQl7CiAJCQlpZiAoIWlzc3BhY2UoKHVuc2lnbmVkIGNoYXIpICpwdHIpKQpA
QCAtMzkzLDYgKzQwNCw3IEBAIHdyaXRlVGltZUxpbmVIaXN0b3J5KFRpbWVMaW5lSUQgbmV3VExJ
LCBUaW1lTGluZUlEIHBhcmVudFRMSSwKIAogCW5ieXRlcyA9IHN0cmxlbihidWZmZXIpOwogCWVy
cm5vID0gMDsKKwlwZ3N0YXRfcmVwb3J0X3dhaXRfc3RhcnQoV0FJVF9FVkVOVF9USU1FTElORV9I
SVNUT1JZX1dSSVRFKTsKIAlpZiAoKGludCkgd3JpdGUoZmQsIGJ1ZmZlciwgbmJ5dGVzKSAhPSBu
Ynl0ZXMpCiAJewogCQlpbnQJCQlzYXZlX2Vycm5vID0gZXJybm87CkBAIC00MDgsNiArNDIwLDcg
QEAgd3JpdGVUaW1lTGluZUhpc3RvcnkoVGltZUxpbmVJRCBuZXdUTEksIFRpbWVMaW5lSUQgcGFy
ZW50VExJLAogCQkJCShlcnJjb2RlX2Zvcl9maWxlX2FjY2VzcygpLAogCQkJCSBlcnJtc2coImNv
dWxkIG5vdCB3cml0ZSB0byBmaWxlIFwiJXNcIjogJW0iLCB0bXBwYXRoKSkpOwogCX0KKwlwZ3N0
YXRfcmVwb3J0X3dhaXRfZW5kKCk7CiAKIAlwZ3N0YXRfcmVwb3J0X3dhaXRfc3RhcnQoV0FJVF9F
VkVOVF9USU1FTElORV9ISVNUT1JZX1NZTkMpOwogCWlmIChwZ19mc3luYyhmZCkgIT0gMCkKLS0g
CjIuMjUuMQoK
--=_2d21b17bc047f832055378e377553f2b--