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
From 976a796c1ff7ed191d9f3df6c1111a333e700e3a Mon Sep 17 00:00:00 2001
From: Masahiro Ikeda <ikeda...@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.c
index 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
