From: Masahiro Ikeda <ikedamsh@oss.nttdata.com>
To: Fujii Masao <masao.fujii@oss.nttdata.com>
Cc: 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, 01 May 2020 10:07:21 +0900
Message-ID: <eb6997cd4131d8beab9f39dc79abb015@oss.nttdata.com> (raw)
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>
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
Attachments:
[text/x-diff] v3-0001-Fix-to-report-wait-events-about-timeline-history-.patch (2.0K, ../eb6997cd4131d8beab9f39dc79abb015@oss.nttdata.com/2-v3-0001-Fix-to-report-wait-events-about-timeline-history-.patch)
download | inline diff:
From 976a796c1ff7ed191d9f3df6c1111a333e700e3a Mon Sep 17 00:00:00 2001
From: Masahiro Ikeda <ikedamsh@oss.nttdata.com>
Date: Fri, 1 May 2020 09:38:51 +0900
Subject: [PATCH v3] Fix to report wait events about timeline history file.
Even though a timeline history file is read or written,
some wait events are not reported. This patch fixes those issues.
---
src/backend/access/transam/timeline.c | 15 ++++++++++++++-
1 file changed, 14 insertions(+), 1 deletion(-)
diff --git a/src/backend/access/transam/timeline.c b/src/backend/access/transam/timeline.cindex de57d699af..4dfe9fddd1 100644--- a/src/backend/access/transam/timeline.c+++ b/src/backend/access/transam/timeline.c@@ -123,7 +123,7 @@ readTimeLineHistory(TimeLineID targetTLI)
* Parse the file...
*/
prevend = InvalidXLogRecPtr;
- while (fgets(fline, sizeof(fline), fd) != NULL)+ for (;;)
{
/* skip leading whitespace and check for # comment */
char *ptr;
@@ -132,6 +132,17 @@ readTimeLineHistory(TimeLineID targetTLI)
uint32 switchpoint_lo;
int nfields;
+ pgstat_report_wait_start(WAIT_EVENT_TIMELINE_HISTORY_READ);+ fgets(fline, sizeof(fline), fd);+ pgstat_report_wait_end();++ if (feof(fd))+ break;+ else if (ferror(fd))+ ereport(ERROR,+ (errcode_for_file_access(),+ errmsg("could not read file \"%s\": %m", path)));+
for (ptr = fline; *ptr; ptr++)
{
if (!isspace((unsigned char) *ptr))
@@ -393,6 +404,7 @@ writeTimeLineHistory(TimeLineID newTLI, TimeLineID parentTLI,
nbytes = strlen(buffer);
errno = 0;
+ pgstat_report_wait_start(WAIT_EVENT_TIMELINE_HISTORY_WRITE);
if ((int) write(fd, buffer, nbytes) != nbytes)
{
int save_errno = errno;
@@ -408,6 +420,7 @@ writeTimeLineHistory(TimeLineID newTLI, TimeLineID parentTLI,
(errcode_for_file_access(),
errmsg("could not write to file \"%s\": %m", tmppath)));
}
+ pgstat_report_wait_end();
pgstat_report_wait_start(WAIT_EVENT_TIMELINE_HISTORY_SYNC);
if (pg_fsync(fd) != 0)
--
2.25.1
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: ikedamsh@oss.nttdata.com, masao.fujii@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: <eb6997cd4131d8beab9f39dc79abb015@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