Removing the pgstat_flush_io() call from the walwriter

Hi,

On 2024-12-18 15:14:07 +0000, Bertrand Drouvot wrote:

While working on [1], it has been noticed that pgstat_flush_io() is called for
the walwriter. Indeed, it's coming from the pgstat_report_wal() call in
WalWriterMain(). That can not report any I/O stats activity (as the
walwriter is not part of the I/O stats tracking, see pgstat_tracks_io_bktype()).

The behavior is there since 28e626bde00 and I did not find any explicit reason
to do so provided in the linked thread [2].

Calling pgstat_flush_io() from there looks unnecessary, so $SUBJECT, until the
walwriter is part of the I/O stats tracking system.

I don't really see the point of this change? What do we gain by moving stuff
around like you did?

Greetings,

Andres Freund

Hi,

On Wed, Dec 18, 2024 at 10:35:45AM -0500, Andres Freund wrote:

Hi,

On 2024-12-18 15:14:07 +0000, Bertrand Drouvot wrote:

While working on [1], it has been noticed that pgstat_flush_io() is called for
the walwriter. Indeed, it's coming from the pgstat_report_wal() call in
WalWriterMain(). That can not report any I/O stats activity (as the
walwriter is not part of the I/O stats tracking, see pgstat_tracks_io_bktype()).

The behavior is there since 28e626bde00 and I did not find any explicit reason
to do so provided in the linked thread [2].

Calling pgstat_flush_io() from there looks unnecessary, so $SUBJECT, until the
walwriter is part of the I/O stats tracking system.

I don't really see the point of this change? What do we gain by moving stuff
around like you did?

Thanks for looking at it!

The purpose is to remove the unnecessary pgstat_flush_io() call from
WalWriterMain(). In passing the patch also removes pgstat_report_wal() because
it just ends up calling pgstat_flush_wal().

Is your remark related to the way it has been done or do you think it's just
fine to keep the useless pgstat_flush_io() call? (I agree that the gain is not
that much as pgstat_io_flush_cb() probably just returns false here when checking
for "have_iostats").

Regards,

--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

Hi,

On 2024-12-18 15:53:39 +0000, Bertrand Drouvot wrote:

On Wed, Dec 18, 2024 at 10:35:45AM -0500, Andres Freund wrote:

Hi,

On 2024-12-18 15:14:07 +0000, Bertrand Drouvot wrote:

While working on [1], it has been noticed that pgstat_flush_io() is called for
the walwriter. Indeed, it's coming from the pgstat_report_wal() call in
WalWriterMain(). That can not report any I/O stats activity (as the
walwriter is not part of the I/O stats tracking, see pgstat_tracks_io_bktype()).

The behavior is there since 28e626bde00 and I did not find any explicit reason
to do so provided in the linked thread [2].

Calling pgstat_flush_io() from there looks unnecessary, so $SUBJECT, until the
walwriter is part of the I/O stats tracking system.

I don't really see the point of this change? What do we gain by moving stuff
around like you did?

Thanks for looking at it!

The purpose is to remove the unnecessary pgstat_flush_io() call from
WalWriterMain(). In passing the patch also removes pgstat_report_wal() because
it just ends up calling pgstat_flush_wal().

Is your remark related to the way it has been done or do you think it's just
fine to keep the useless pgstat_flush_io() call? (I agree that the gain is not
that much as pgstat_io_flush_cb() probably just returns false here when checking
for "have_iostats").

Yea, i think it's fine to just call it unnecessarily. Particularly because we
do want to actually report IO stats for walwriter eventually.

Greetings,

Andres Freund

Hi,

On Wed, Dec 18, 2024 at 11:55:11AM -0500, Andres Freund wrote:

Yea, i think it's fine to just call it unnecessarily. Particularly because we
do want to actually report IO stats for walwriter eventually.

Yeah, better to spend time and energy on making it "necessary" instead: I'll
try to have a look at adding IO stats for walwriter.

Regards,

--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

Hi,

On Thu, 19 Dec 2024 at 08:50, Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:

Hi,

On Wed, Dec 18, 2024 at 11:55:11AM -0500, Andres Freund wrote:

Yea, i think it's fine to just call it unnecessarily. Particularly because we
do want to actually report IO stats for walwriter eventually.

Yeah, better to spend time and energy on making it "necessary" instead: I'll
try to have a look at adding IO stats for walwriter.

I have been working on showing all WAL stats in the pg_stat_io [1]https://commitfest.postgresql.org/51/4950/
view but currently it is blocked because the size of the WAL read IO
may vary and it is not possible to show this on the pg_stat_io view.
Proposed a fix [2]https://commitfest.postgresql.org/51/5256/ for that (by counting IOs as bytes instead of
blocks in the pg_stat_io) which you already reviewed :).

[1]: https://commitfest.postgresql.org/51/4950/
[2]: https://commitfest.postgresql.org/51/5256/

--
Regards,
Nazir Bilal Yavuz
Microsoft

Hi,

On Thu, Dec 19, 2024 at 05:50:43PM +0300, Nazir Bilal Yavuz wrote:

Hi,

On Thu, 19 Dec 2024 at 08:50, Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:

Hi,

On Wed, Dec 18, 2024 at 11:55:11AM -0500, Andres Freund wrote:

Yea, i think it's fine to just call it unnecessarily. Particularly because we
do want to actually report IO stats for walwriter eventually.

Yeah, better to spend time and energy on making it "necessary" instead: I'll
try to have a look at adding IO stats for walwriter.

I have been working on showing all WAL stats in the pg_stat_io [1]
view

Oh nice, I did not realize that you were working on it, thanks for the
notification ;-) We'll have a look at it!

Regards,

--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com