Thread: Re: 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] 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
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