On 2020/05/01 10:07, Masahiro Ikeda wrote:
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.
Thanks for updating the patch! It looks good to me.
I applied cosmetic changes to the patch (attached). Barring any objection,
I will push this patch (also back-patch to v10 where wait-event for timeline
file was added).
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
diff --git a/src/backend/access/transam/timeline.c
b/src/backend/access/transam/timeline.c
index de57d699af..aeaaf44a4a 100644
--- a/src/backend/access/transam/timeline.c
+++ b/src/backend/access/transam/timeline.c
@@ -78,7 +78,6 @@ readTimeLineHistory(TimeLineID targetTLI)
List *result;
char path[MAXPGPATH];
char histfname[MAXFNAMELEN];
- char fline[MAXPGPATH];
FILE *fd;
TimeLineHistoryEntry *entry;
TimeLineID lasttli = 0;
@@ -123,15 +122,27 @@ readTimeLineHistory(TimeLineID targetTLI)
* Parse the file...
*/
prevend = InvalidXLogRecPtr;
- while (fgets(fline, sizeof(fline), fd) != NULL)
+ for (;;)
{
- /* skip leading whitespace and check for # comment */
+ char fline[MAXPGPATH];
+ char *res;
char *ptr;
TimeLineID tli;
uint32 switchpoint_hi;
uint32 switchpoint_lo;
int nfields;
+ pgstat_report_wait_start(WAIT_EVENT_TIMELINE_HISTORY_READ);
+ res = fgets(fline, sizeof(fline), fd);
+ pgstat_report_wait_end();
+ if (ferror(fd))
+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("could not read file \"%s\":
%m", path)));
+ if (res == NULL)
+ break;
+
+ /* skip leading whitespace and check for # comment */
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)
