Horton

Re: Show WAL write and fsync stats in pg_stat_io

Nazir Bilal Yavuz <byavuz81@gmail.com>

View thread
From: Nazir Bilal Yavuz <byavuz81@gmail.com>
To: Michael Paquier <michael@paquier.xyz>
Cc: Melanie Plageman <melanieplageman@gmail.com>, Andres Freund <andres@anarazel.de>, pgsql-hackers <pgsql-hackers@postgresql.org>, "bharath.rupireddyforpostgres@gmail.com" <bharath.rupireddyforpostgres@gmail.com>, Thomas Munro <thomas.munro@gmail.com>, Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Date: 2025-01-16T08:40:51Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Fix copy-paste error related to the autovacuum launcher in pgstat_io.c

  2. Move SQL tests of pg_stat_io for WAL data to recovery test 029_stats_restart

  3. Add data for WAL in pg_stat_io and backend statistics

  4. Improve comment on top of pgstat_count_io_op_time()

  5. Refactor pgstat_prepare_io_time() with an input argument instead of a GUC

Attachments

Hi,

On Fri, 19 Apr 2024 at 11:01, Nazir Bilal Yavuz <byavuz81@gmail.com> wrote:
>
> If I am not missing any new changes, the only problem is reading
> variable bytes now. We have discussed a couple of solutions:

With the recent commit [1], pg_stat_io tracks IOs as bytes instead of
blocks. This solves the variable IO size problem.

I encountered another problem while rebasing the patch. The problem is
basically we do not expect any pending stats while restoring the stats
at the initdb. However, WAL IOs (WAL read and WAL init IOs for now)
may happen before restoring the stats, so we end up having pending
stats before restoring them and that causes initdb to fail.

I wrote this problem to another thread [2] but this thread is a better
place to discuss it, so rewriting the problem:

This is where we restore stats and do not expect any pending stats at
the Assert:

'''
pgstat_restore_stats() ->
pgstat_read_statsfile() ->
pgstat_reset_after_failure() ->
pgstat_drop_all_entries() ->
pgstat_drop_entry_internal() ->
We have an assertion there which checks if there is a pending stat entry:

    /* should already have released local reference */
    if (pgStatEntryRefHash)
        Assert(!pgstat_entry_ref_hash_lookup(pgStatEntryRefHash, shent->key));
'''

This is where the WAL read happens before restoring the stats:

'''
BootstrapModeMain() ->
InitPostgres() ->
StartupXLOG() ->
ReadCheckpointRecord() ->
InitWalRecovery() ->
... ->
XLogReadAhead() ->
XLogDecodeNextRecord() ->
ReadPageInternal() ->
state->routine.page_read = XLogPageRead() then WAL read happens
'''

So, this assert fails because we have pending stats for the
PGSTAT_KIND_BACKEND. It is only PGSTAT_KIND_BACKEND because all
fixed-numbered stats (which include PGSTAT_KIND_IO) are reset there:
'pgstat_reset_after_failure() -> kind_info->reset_all_cb()' at the
pgstat_reset_after_failure(). It seems that we do not care about stats
that happen before restoring the stats part as we reset all
fixed-numbered stats there, so not counting these WAL IOs at the
initdb may be a one solution.

A simple reproducer patch is attached, it includes two
pgstat_count_io_op() calls. I did not include the rest of the patchset
as I thought it may increase the complexity. To reproduce, just run
initdb on assert enabled build after applying the patch. Then you
should see:

creating configuration files ... ok
running bootstrap script ... TRAP: failed
Assert("!pgstat_entry_ref_hash_lookup(pgStatEntryRefHash,
shent->key)"), File:
"../../postgres/src/backend/utils/activity/pgstat_shmem.c", Line: 859,
PID: 51001
.../install/bin/postgres(ExceptionalCondition+0xab) [0x55da0959feea]

I would be happy to hear your thoughts.

[1] f92c854cf
[2] postgr.es/m/CAN55FZ1uOq%3DFVJObp0bdj-Z8q1ZRNmA-RymPqbMD%2Bp4QaHXP3A%40mail.gmail.com

-- 
Regards,
Nazir Bilal Yavuz
Microsoft