Re: Show WAL write and fsync stats in pg_stat_io

Hi,

On Fri, 19 Apr 2024 at 11:01, Nazir Bilal Yavuz <byavuz81(at)gmail(dot)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