Feedback-ID: id4a34324:Fastmail
Date: Thu, 16 Jan 2025 11:28:43 -0500
From: Andres Freund <andres@anarazel.de>
To: Michael Paquier <michael@paquier.xyz>
Cc: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>, 
	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: <66efowskppsns35v5u2m7k4sdnl7yoz5bo64tdjwq7r5lhplrz@y7dme5xwh2r5>
References: <Z2EZegYbH22LQHhN@paquier.xyz>
 <Z2EyxyEJPMMldj1t@ip-10-97-1-34.eu-west-3.compute.internal>
 <Z2FAyFSg-lRGFZio@paquier.xyz>
 <Z2FF6ScWef9yIrJL@ip-10-97-1-34.eu-west-3.compute.internal>
 <Z2JWUaY8AHBs99yp@paquier.xyz>
 <Z2KDy56Ls4nBl1Y8@ip-10-97-1-34.eu-west-3.compute.internal>
 <Z2OfYh3QJ7M5D0i-@paquier.xyz>
 <CAN55FZ0ZQTf-D+5Rup4H6zp+n3E2EuonYuzTiPkp=1GhesZU_A@mail.gmail.com>
 <Z4dyHHxGfGK7bGjd@ip-10-97-1-34.eu-west-3.compute.internal>
 <Z4d_eggsxtBEdJAG@paquier.xyz>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <Z4d_eggsxtBEdJAG@paquier.xyz>
Archived-At: <https://www.postgresql.org/message-id/66efowskppsns35v5u2m7k4sdnl7yoz5bo64tdjwq7r5lhplrz%40y7dme5xwh2r5>
Precedence: bulk

Hi,

On 2025-01-15 18:27:22 +0900, Michael Paquier wrote:
> On Wed, Jan 15, 2025 at 08:30:20AM +0000, Bertrand Drouvot wrote:
> > On Wed, Jan 15, 2025 at 11:03:54AM +0300, Nazir Bilal Yavuz wrote:
> >> With this commit it may not be possible to count IOs in the critical
> >> sections. I think the problem happens only if the local
> >> PgStat_BackendPending entry is being created for the first time for
> >> this backend in the critical section.
> >
> > Yeah, I encountered the exact same thing and mentioned it in [1] (see R1.).
> >
> > In [1]狢 did propose to use a new PendingBackendWalStats variable to "bypass" the
> > pgstat_prep_backend_pending() usage.
> >
> > Michael mentioned in [2] that is not really consistent with the rest (what I
> > agree with) and that "we should rethink a bit the way pending entries are
> > retrieved". I did not think about it yet but that might be the way to
> > go, thoughts?
> >
> > [1]: https://www.postgresql.org/message-id/Z3zqc4o09dM/Ezyz%40ip-10-97-1-34.eu-west-3.compute.internal
> > [2]: https://www.postgresql.org/message-id/Z4dRlNuhSQ3hPPv2%40paquier.xyz
>
> My problem is that this is not only related to backend stats, but to
> all variable-numbered stats kinds that require this behavior.

The issue here imo is that recently IO stats were turned into a variable
numbered stat, where it previously wasn't.  It seems like a bad idea that now
we regularly do a somewhat sizable memory allocation for backend stats. For
table stats allocating the pending memory is somewhat a necessity - there
could be a lot of different tables after all. And later transactions won't
necessarily access the same set of tables, so we can't just keep the memory
for pending around - but that's not true for IO.


> One other case where I've seen this as being an issue is injection point
> stats, for example, while enabling these stats for all callbacks and some of
> them are run in critical sections.

That seems somewhat fundamental - you could have arbitrary numbers of
injection points and thus need arbitrary amounts of memory. You can't just do
that in a critical section. And it's not just about a potential memory
allocation for pending stats, the DSM allocation can't be done in a critical
section either.


> A generic solution to the problem would be to allow
> pgStatPendingContext to have MemoryContextAllowInCriticalSection() set
> temporarily for the allocation of the pending data.
> Perhaps not for all stats kinds, just for these where we're OK with
> the potential memory footprint so we could have a flag in
> PgStat_KindInfo.

That seems like an *incredibly* bad idea. That restriction exists for a
reason.


> Andres, do you have any thoughts about that?  Not sure what would be
> your view on this matter.  MemoryContextAllowInCriticalSection() for
> stats kinds that allow the case would be OK for me I guess, we should
> not be talking about a lot of memory.

My view is that for IO stats no memory allocation should be required - that
used to be the case and should be the case again. And for something like
injection points (not really convinced that it's a good thing to have stats
for, but ...), allocation the shared memory stats object outside of the
critical section and not having any pending stats seems to be the right
approach.

Greetings,

Andres Freund