Re: Removing the pgstat_flush_io() call from the walwriter

От:

Andres Freund <andres@anarazel.de>

Дата:

18 декабря 2024 г. в 19:55:11

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

Re: Removing the pgstat_flush_io() call from the walwriter

От:

Nazir Bilal Yavuz <byavuz81@gmail.com>

Дата:

19 декабря 2024 г. в 17:50:43

Hi,

On Thu, 19 Dec 2024 at 08:50, Bertrand Drouvot
 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 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] 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

Re: Removing the pgstat_flush_io() call from the walwriter

От:

Andres Freund <andres@anarazel.de>

Дата:

18 декабря 2024 г. в 18:35:45

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

Re: Removing the pgstat_flush_io() call from the walwriter

От:

Bertrand Drouvot <bertranddrouvot.pg@gmail.com>

Дата:

18 декабря 2024 г. в 18:53:39

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

Re: Removing the pgstat_flush_io() call from the walwriter

От:

Bertrand Drouvot <bertranddrouvot.pg@gmail.com>

Дата:

19 декабря 2024 г. в 08:49:51

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

Re: Removing the pgstat_flush_io() call from the walwriter

От:

Bertrand Drouvot <bertranddrouvot.pg@gmail.com>

Дата:

19 декабря 2024 г. в 18:07:17

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
>  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