Re: per backend I/O statistics

Hi,

On Wed, Nov 27, 2024 at 03:33:38PM +0900, Michael Paquier wrote:
> On Mon, Nov 25, 2024 at 03:47:59PM +0000, Bertrand Drouvot wrote:
> > It also takes care of most of the comments that you have made in [1], meaning
> > that it:
> >
> > - removes the backend type from PgStat_Backend and look for the backend type
> > at "display" time.
> > - creates PgStat_BackendPendingIO and PgStat_PendingIO now refers to it (I
> > used PgStat_BackendPendingIO and not PgStat_BackendPending because this is what
> > it is after all).
> > - adds the missing comment related to the PID in the doc.
> > - merges 0004 with 0001 (so that pg_stat_get_backend_io() is now part of 0001).
> > - creates its own pgstat_backend.c file.
>
> I have begun studying the patch, and I have one question.
>
> +void
> +pgstat_create_backend_stat(ProcNumber procnum)
> [...]
> + /* Create the per-backend statistics entry */
> + if (pgstat_tracks_per_backend_bktype(MyBackendType))
> + pgstat_create_backend_stat(MyProcNumber);
>
> Perhaps that's a very stupid question, but I was looking at this part
> of the patch, and wondered why we don't use init_backend_cb? I
> vaguely recalled that MyProcNumber and MyBackendType would be set
> before we go through the pgstat initialization step. MyDatabaseId is
> filled after the pgstat initialization, but we don't care about this
> information for such backend stats.

Using init_backend_cb (and moving the pgstat_tracks_per_backend_bktype() check
in it) is currently producing a failed assertion:

TRAP: failed Assert("pgstat_is_initialized && !pgstat_is_shutdown"), File: "pgstat.c", Line: 1567, PID: 2080000
postgres: postgres postgres [local] initializing(ExceptionalCondition+0xbb)[0x5fd0c69afaed]
postgres: postgres postgres [local] initializing(pgstat_assert_is_up+0x3f)[0x5fd0c67d1b77]
postgres: postgres postgres [local] initializing(pgstat_get_entry_ref+0x95)[0x5fd0c67db068]
postgres: postgres postgres [local] initializing(pgstat_prep_pending_entry+0xaa)[0x5fd0c67d1181]
postgres: postgres postgres [local] initializing(pgstat_create_backend_stat+0x96)[0x5fd0c67d3986]

But even if we, say move the "pgstat_is_initialized = true" before the init_backend_cb()
call in pgstat_initialize() (not saying that makes sense, just trying out), then
we are back to the restore/reset issue but this time during initdb:

TRAP: failed Assert("!pgstat_entry_ref_hash_lookup(pgStatEntryRefHash, shent->key)"), File: "pgstat_shmem.c", Line: 852, PID: 2109086
/home/postgres/postgresql/pg_installed/pg18/bin/postgres(ExceptionalCondition+0xbb)[0x621723499c5a]
/home/postgres/postgresql/pg_installed/pg18/bin/postgres(+0x70dd2b)[0x6217232c5d2b]
/home/postgres/postgresql/pg_installed/pg18/bin/postgres(pgstat_drop_all_entries+0x61)[0x6217232c60b5]
/home/postgres/postgresql/pg_installed/pg18/bin/postgres(+0x705356)[0x6217232bd356]
/home/postgres/postgresql/pg_installed/pg18/bin/postgres(+0x70462a)[0x6217232bc62a]
/home/postgres/postgresql/pg_installed/pg18/bin/postgres(pgstat_restore_stats+0x1c)[0x6217232b9f01]
/home/postgres/postgresql/pg_installed/pg18/bin/postgres(StartupXLOG+0x693)[0x621722da9697]
/home/postgres/postgresql/pg_installed/pg18/bin/postgres(InitPostgres+0x1d8)[0x6217234b40df]
/home/postgres/postgresql/pg_installed/pg18/bin/postgres(BootstrapModeMain+0x532)[0x621722dd79bf]

where "PID: 2109086" is a B_STANDALONE_BACKEND.

This is due to the fact that, during the initdb bootstrap, init_backend_cb() is
called before StartupXLOG().

One option could be to move B_STANDALONE_BACKEND in the "false" section of
pgstat_tracks_per_backend_bktype() and ensure that we set pgstat_is_initialized
to true before init_backend_cb() is called (but I don't think that makes that much
sense).

I'd vote to just keep the pgstat_create_backend_stat() call in pgstat_beinit().

That said, we probably should document that init_backend_cb() should not call
pgstat_get_entry_ref(), but that's probably worth another thread.

Regards,

--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com