Date: Tue, 19 Nov 2024 16:28:55 +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: <Zzy8x7fGkWEKB7SF@ip-10-97-1-34.eu-west-3.compute.internal>
References: <ZyibjiLgoLx+kS4n@ip-10-97-1-34.eu-west-3.compute.internal>
 <ZypXy3fH/EyALaTR@ip-10-97-1-34.eu-west-3.compute.internal>
 <Zyqsm6ilLLADAGw2@paquier.xyz>
 <Zyt0Rh6Uwvf+mlA1@ip-10-97-1-34.eu-west-3.compute.internal>
 <ZywO84-2K8fxEIx8@paquier.xyz>
 <ZyzrrFNvXC2YvcNE@ip-10-97-1-34.eu-west-3.compute.internal>
 <Zy4bmvgHqGjcK1pI@ip-10-97-1-34.eu-west-3.compute.internal>
 <ZzWZV9LdyZ9aFSWs@paquier.xyz>
 <ZzX7Y5i56UrOtlRC@ip-10-97-1-34.eu-west-3.compute.internal>
 <ZzvuScIJW9kwnuyE@paquier.xyz>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZzvuScIJW9kwnuyE@paquier.xyz>
Archived-At: <https://www.postgresql.org/message-id/Zzy8x7fGkWEKB7SF%40ip-10-97-1-34.eu-west-3.compute.internal>
Precedence: bulk

Hi,

On Tue, Nov 19, 2024 at 10:47:53AM +0900, Michael Paquier wrote:
> On Thu, Nov 14, 2024 at 01:30:11PM +0000, Bertrand Drouvot wrote:
> > - change the arguments to false in the pgstat_drop_entry_internal() call in 
> > pgstat_drop_all_entries()
> > - start the engine
> > - kill -9 postgres
> > - restart the engine
> > 
> > You'll see the assert failing due to the startup process. I don't think it is
> > doing something wrong though, just populating its backend stats. Do you have
> > another view on it?
> 
> It feels sad that we have to plug in the internals at this level for
> this particular case.  Perhaps there is something to do with more
> callbacks.  Or perhaps there is just no point in tracking the stats of
> auxiliary processes because we already have this data in the existing
> pg_stat_io already?

We can not discard the "per backend" stats collection for auxiliary processes
because those are the source for pg_stat_io too (once the 2 flush callbacks are
merged).

I have another idea: after a bit more of investigation it turns out that
only the startup process is generating the failed assertion.

So, for the startup process only, what about?

- don't call pgstat_create_backend_stat() in pgstat_beinit()...
- but call it in StartupXLOG() instead (after the stats are discarded or restored).

That way we could get rid of the changes linked to the assertion and still handle
the startup process particular case. Thoughts?

> > How would that work for someone doing "select * from pg_stat_get_backend_io(<pid>)"?
> > 
> > Do you mean copy/paste part of the code from pg_stat_get_activity() into
> > pg_stat_get_backend_io() to get the backend type? That sounds like an idea,
> > I'll have a look at it.
> 
> I was under the impression that we should keep PgStat_Backend for the
> reset_timestamp part, but drop BackendType as it can be guessed from
> pg_stat_activity itself.

Removing BackendType sounds doable, I'll look at it.

> >> Structurally, it may be cleaner to keep all the callbacks and the
> >> backend-I/O specific logic into a separate file, perhaps
> >> pgstat_io_backend.c or pgstat_backend_io?
> > 
> > Yeah, I was wondering the same. What about pgstat_backend.c (that would contain
> > only one "section" dedicated to IO currently)?
> 
> pgstat_backend.c looks good to me.  Could there be other stats than
> just IO, actually?  Perhaps not, just asking..

Yeah that's the reason why I suggested "pgstat_backend.c". I would not be
surprised if we add more "per backend" stats (that are not I/O related) in the
future as the main machinery would be there.

Regards,

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