Re: Inconsistency in reporting checkpointer stats

> IMO, there's no need for 2 separate patches for these changes.

I will make it a single patch while sharing the next patch.

> + (errmsg("restartpoint complete: wrote %d buffers (%.1f%%), "
> + "wrote %d slru buffers (%.1f%%); %d WAL
> file(s) added, "
> + "%d removed, %d recycled; write=%ld.%03d s, "
> + "sync=%ld.%03d s, total=%ld.%03d s; sync files=%d, "
> + "longest=%ld.%03d s, average=%ld.%03d s;
> distance=%d kB, "
> + "estimate=%d kB; lsn=%X/%X, redo lsn=%X/%X",
> Hm, restartpoint /checkpoint complete message is already too long to
> read and adding slru buffers to it make it further longer. Note that
> we don't need to add every checkpoint stat to the log message but to
> pg_stat_bgwriter. Isn't it enough to show SLRU buffers information in
> pg_stat_bgwriter alone?

I understand that the log message is too long already but I feel it is
ok since it logs only one time per checkpoint and as discussed
upthread, SLRU information is potentially useful.

> Can't one look at pg_stat_slru's blks_written
> (pgstat_count_slru_page_written()) to really track the SLRUs written?
> Or is it that one may want to track SLRUs during a checkpoint
> separately? Is there a real use-case/customer reported issue driving
> this change?
>
> After looking at pg_stat_slru.blks_written, I think the best way is to
> just leave things as-is and let CheckpointStats count slru buffers too
> unless there's really a reported issue that says
> pg_stat_slru.blks_written doesn't serve the purpose.

The v1 patch corresponds to what you are suggesting. But the question
is not about tracking slru buffers, it is about separating this
information from main buffers count during checkpoint. I think there
is enough discussion done upthread to exclude slru buffers from
CheckpointStats.ckpt_bufs_written. Please share if you still strongly
feel that a separate counter is not required.

Thanks & Regards,
Nitin Jadhav

On Fri, Jan 27, 2023 at 10:45 AM Bharath Rupireddy
<bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
>
> On Wed, Dec 21, 2022 at 5:15 PM Nitin Jadhav
> <nitinjadhavpostgres(at)gmail(dot)com> wrote:
> >
> > I have modified the code accordingly and attached the new version of
> > patches. patch 0001 fixes the inconsistency in checkpointer stats and
> > patch 0002 separates main buffer and SLRU buffer count from checkpoint
> > complete log message.
>
> IMO, there's no need for 2 separate patches for these changes.
>
> + (errmsg("restartpoint complete: wrote %d buffers (%.1f%%), "
> + "wrote %d slru buffers (%.1f%%); %d WAL
> file(s) added, "
> + "%d removed, %d recycled; write=%ld.%03d s, "
> + "sync=%ld.%03d s, total=%ld.%03d s; sync files=%d, "
> + "longest=%ld.%03d s, average=%ld.%03d s;
> distance=%d kB, "
> + "estimate=%d kB; lsn=%X/%X, redo lsn=%X/%X",
> Hm, restartpoint /checkpoint complete message is already too long to
> read and adding slru buffers to it make it further longer. Note that
> we don't need to add every checkpoint stat to the log message but to
> pg_stat_bgwriter. Isn't it enough to show SLRU buffers information in
> pg_stat_bgwriter alone?
>
> Can't one look at pg_stat_slru's blks_written
> (pgstat_count_slru_page_written()) to really track the SLRUs written?
> Or is it that one may want to track SLRUs during a checkpoint
> separately? Is there a real use-case/customer reported issue driving
> this change?
>
> After looking at pg_stat_slru.blks_written, I think the best way is to
> just leave things as-is and let CheckpointStats count slru buffers too
> unless there's really a reported issue that says
> pg_stat_slru.blks_written doesn't serve the purpose.
>
> --
> Bharath Rupireddy
> PostgreSQL Contributors Team
> RDS Open Source Databases
> Amazon Web Services: https://aws.amazon.com