Re: Inconsistency in reporting checkpointer stats

Thanks for the review.

> In pgstat_checkpointer.c, it looks like you missed adding
> CHECKPOINTER_COMP(slru_written) in pgstat_checkpointer_snapshot_cb().

Fixed it.

> + <row>
> + <entry role="catalog_table_entry"><para role="column_definition">
> + <structfield>slru_written</structfield> <type>bigint</type>
> + </para>
> + <para>
> + Number of SLRU buffers written during checkpoints and restartpoints
> + </para></entry>
> + </row>
>
> This entry should be moved to the pg_stat_checkpointer documentation.

Fixed it.

> + CheckpointStats.ckpt_slru_written,
> + (double) CheckpointStats.ckpt_slru_written * 100 / NBuffers,
>
> I don't think NBuffers represents the maximum number of SLRU buffers.
> We might need to calculate this based on specific GUC settings,
> like transaction_buffers.

Great observation. Since the SLRU buffers written during a checkpoint
can include transaction_buffers, commit_timestamp_buffers,
subtransaction_buffers, multixact_member_buffers,
multixact_offset_buffers, and serializable_buffers, the total count of
SLRU buffers should be the sum of all these types. We might need to
introduce a global variable, such as total_slru_count, in the
globals.c file to hold this sum. The num_slots variable in the
SlruSharedData structure needs to be accessed from all types of SLRU
and stored in total_slru_count. This can then be used during logging
to calculate the percentage of SLRU buffers written. However, I’m
unsure if this effort is justified. While it makes sense for normal
buffers to display the percentage, the additional code required might
not provide significant value to users. Therefore, I have removed this
in the attached v6 patch. If it is really required, I am happy to make
the above changes and share the updated patch.

Best Regards,
Nitin Jadhav
Azure Database for PostgreSQL
Microsoft

On Wed, Sep 18, 2024 at 6:57 PM Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)com> wrote:
>
>
>
> On 2024/07/18 16:08, Nitin Jadhav wrote:
> > I apologize for not being active on this thread. However, I have now
> > returned to the thread and confirmed that the inconsistency is still
> > present in the latest code. I believe it’s crucial to address this
> > issue, and I am currently submitting the v5 version of the patch. The
> > v4 version had addressed the feedback from Bharath, Kyotaro, Andres,
> > and Robert. The current version has been rebased to incorporate
> > Vignesh’s suggestions. In response to Michael’s comments, I’ve moved
> > the new ‘slru_written’ column from the ‘pg_stat_bgwriter’ view to the
> > ‘pg_stat_checkpointer’ in the attached patch.
> >
> > To summarize our discussions, we’ve reached a consensus to correct the
> > mismatch between the information on buffers written as displayed in
> > the ‘pg_stat_checkpointer’ view and the checkpointer log message.
> > We’ve also agreed to separate the SLRU buffers data from the buffers
> > written and present the SLRU buffers data in a distinct field.
> >
> > I have created the new commitfest entry here
> > https://commitfest.postgresql.org/49/5130/.
> > Kindly share if any comments.
>
> Thanks for updating the patch!
>
> In pgstat_checkpointer.c, it looks like you missed adding
> CHECKPOINTER_COMP(slru_written) in pgstat_checkpointer_snapshot_cb().
>
> + <row>
> + <entry role="catalog_table_entry"><para role="column_definition">
> + <structfield>slru_written</structfield> <type>bigint</type>
> + </para>
> + <para>
> + Number of SLRU buffers written during checkpoints and restartpoints
> + </para></entry>
> + </row>
>
> This entry should be moved to the pg_stat_checkpointer documentation.
>
> + CheckpointStats.ckpt_slru_written,
> + (double) CheckpointStats.ckpt_slru_written * 100 / NBuffers,
>
> I don't think NBuffers represents the maximum number of SLRU buffers.
> We might need to calculate this based on specific GUC settings,
> like transaction_buffers.
>
> Regards,
>
> --
> Fujii Masao
> Advanced Computing Technology Center
> Research and Development Headquarters
> NTT DATA CORPORATION
>