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

[Masahiro Ikeda]

On 2020-04-27 12:25, Fujii Masao wrote:

On 2020/04/24 11:29, Masahiro Ikeda wrote:

Hi,

There are two unexpected codes for me about wait events for timeline history file.

Please let me know your thoughts whether if we need to change.


1. readTimeLineHistory() function in timeline.c

The readTimeLineHistory() reads a timeline history file,
but it doesn't report “WAIT_EVENT_TIMELINE_HISTORY_READ".

Yeah, this sounds strange.

In my understanding, sscanf() is blocking read.
So, it's important to report a wait event.

Shouldn't the wait event be reported during fgets() rather than sscanf()?

2. writeTimeLineHistory() function in timeline.c

The writeTimeLineHistory() function may write a timeline history file twice,

but it reports “WAIT_EVENT_TIMELINE_HISTORY_WRITE" only once.

It makes sense to report a wait event twice, because both of them use write().

Yes.

I attached a patch to mention the code line number.


I checked the commit log which "WAIT_EVENT_TIMELINE_HISTORY_READ" and

"WAIT_EVENT_TIMELINE_HISTORY_WRITE" are committed and the discussion about it.

But I can't find the reason.

Please give me your comments.
If we need to change, I can make a patch to fix them.

Thanks! I agree to fix those issues.

Thanks for the comments. I attach a patch to fix those issues.
Please review it.

By the way, which is correct "timeline's history file" or "timeline history file"? The timeline.c has both. In my understanding, the latter is correct. If so, I will modify together.

Maybe both are correct?? I have no strong opinion about this.

OK, I didn't fix it at this time.

Regards,
--
Masahiro Ikeda
From 9e5165673714b929a76bf39792f04ea5e348f0a3 Mon Sep 17 00:00:00 2001
From: Masahiro Ikeda <ikeda...@nttdata.co.jp>
Date: Tue, 28 Apr 2020 01:39:25 +0000
Subject: [PATCH] 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 | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/src/backend/access/transam/timeline.c b/src/backend/access/transam/timeline.c
index de57d69..7a652a8 100644
--- a/src/backend/access/transam/timeline.c
+++ b/src/backend/access/transam/timeline.c
@@ -123,6 +123,7 @@ readTimeLineHistory(TimeLineID targetTLI)
 	 * Parse the file...
 	 */
 	prevend = InvalidXLogRecPtr;
+	pgstat_report_wait_start(WAIT_EVENT_TIMELINE_HISTORY_READ);
 	while (fgets(fline, sizeof(fline), fd) != NULL)
 	{
 		/* skip leading whitespace and check for # comment */
@@ -172,6 +173,7 @@ readTimeLineHistory(TimeLineID targetTLI)
 
 		/* we ignore the remainder of each line */
 	}
+	pgstat_report_wait_end();
 
 	FreeFile(fd);
 
@@ -393,6 +395,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 +411,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.9.5