Date: Wed, 27 Nov 2024 16:00:14 +0000
From: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
To: Michael Paquier <michael@paquier.xyz>
Cc: Nazir Bilal Yavuz <byavuz81@gmail.com>,
	Alvaro Herrera <alvherre@alvh.no-ip.org>,
	Kyotaro Horiguchi <horikyota.ntt@gmail.com>,
	pgsql-hackers@lists.postgresql.org
Subject: Re: per backend I/O statistics
Message-ID: <Z0dCDjItim7tp/jB@ip-10-97-1-34.eu-west-3.compute.internal>
References: <Zz3wIuQUJysX5O3d@ip-10-97-1-34.eu-west-3.compute.internal>
 <Zz5gMM9w59guO9S_@paquier.xyz>
 <Zz9sno+JJbWqdXhQ@ip-10-97-1-34.eu-west-3.compute.internal>
 <Zz_gHQ2g8MUe8oJ1@paquier.xyz>
 <Z0A3pmORCbhRe0Ug@ip-10-97-1-34.eu-west-3.compute.internal>
 <Z0PNpAjhcS7S-1LI@paquier.xyz>
 <Z0QjeIkwC0HNI16K@ip-10-97-1-34.eu-west-3.compute.internal>
 <Z0Qk3nZcaVpbtCKU@paquier.xyz>
 <Z0ScL5W6EeWXShb3@ip-10-97-1-34.eu-west-3.compute.internal>
 <Z0a9QuH8HRLfT5kY@paquier.xyz>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Z0a9QuH8HRLfT5kY@paquier.xyz>
Archived-At: <https://www.postgresql.org/message-id/Z0dCDjItim7tp/jB%40ip-10-97-1-34.eu-west-3.compute.internal>
Precedence: bulk

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