Re: About to add WAL write/fsync statistics to pg_stat_wal view - Mailing list pgsql-hackers

On 2021/03/15 10:39, Masahiro Ikeda wrote:
> Thanks, I understood get_sync_bit() checks the sync flags and
> the write unit of generated wal data and replicated wal data is different.
> (It's interesting optimization whether to use kernel cache or not.)
> 
> OK. Although I agree to separate the stats for the walrecever,
> I want to hear opinions from other people too. I didn't change the patch.
> 
> Please feel to your comments.

What about applying the patch for common WAL write function like
XLogWriteFile(), separately from the patch for walreceiver's stats?
Seems the former reaches the consensus, so we can commit it firstly.
Also even only the former change is useful because which allows
walreceiver to report WALWrite wait event.

> OK. I agree.
> 
> I wonder to change the error check ways depending on who calls this function?
> Now, only the walreceiver checks (1)errno==0 and doesn't check (2)errno==ENITR.
> Other processes are the opposite.
> 
> IIUC, it's appropriate that every process checks (1)(2).
> Please let me know my understanding is wrong.

I'm thinking the same. Regarding (2), commit 79ce29c734 introduced
that code. According to the following commit log, it seems harmless
to retry on EINTR even walreceiver.

     Also retry on EINTR. All signals used in the backend are flagged SA_RESTART
     nowadays, so it shouldn't happen, but better to be defensive.

>> BTW, currently XLogWrite() increments IO timing even when pg_pwrite()
>> reports an error. But this is useless. Probably IO timing should be
>> incremented after the return code of pg_pwrite() is checked, instead?
> 
> Yes, I agree. I fixed it.
> (v18-0003-Makes-the-wal-receiver-report-WAL-statistics.patch)

Thanks for the patch!

              nleft = nbytes;
              do
              {
-                errno = 0;
+                written = XLogWriteFile(openLogFile, from, nleft, (off_t) startoffset,
+                                        ThisTimeLineID, openLogSegNo, wal_segment_size);

Can we merge this do-while loop in XLogWrite() into the loop
in XLogWriteFile()?

If we do that, ISTM that the following codes are not necessary in XLogWrite().

                  nleft -= written;
                  from += written;

+ * 'segsize' is a segment size of WAL segment file.

Since segsize is always wal_segment_size, segsize argument seems
not necessary in XLogWriteFile().

+XLogWriteFile(int fd, const void *buf, size_t nbyte, off_t offset,
+              TimeLineID timelineid, XLogSegNo segno, int segsize)

Why did you use "const void *" instead of "char *" for *buf?

Regarding 0005 patch, I will review it later.

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION