Re: per backend WAL statistics

Hi,

On 2025-01-16 17:11:09 +0000, Bertrand Drouvot wrote:
> On Thu, Jan 16, 2025 at 11:38:47AM -0500, Andres Freund wrote:
> > Hi,
> > 
> > On 2025-01-16 15:59:31 +0000, Bertrand Drouvot wrote:
> > > On Wed, Jan 15, 2025 at 03:11:32PM +0900, Michael Paquier wrote:
> > > > On Fri, Jan 10, 2025 at 09:40:38AM +0000, Bertrand Drouvot wrote:
> > > > + * WAL pending statistics are incremented inside a critical section
> > > > + * (see XLogWrite()), so we can't use pgstat_prep_pending_entry() and we rely on
> > > > + * PendingBackendWalStats instead.
> > > > + */
> > > > +extern PGDLLIMPORT PgStat_PendingWalStats PendingBackendWalStats;
> > > > 
> > > > Hmm.  This makes me wonder if we should rethink a bit the way pending
> > > > entries are retrieved and if we should do it beforehand for the WAL
> > > > paths to avoid allocations in some critical sections.  Isn't that also
> > > > because we avoid calling pgstat_prep_backend_pending() for the I/O
> > > > case as only backends are supported now, discarding cases like the
> > > > checkpointer where I/O could happen in a critical path?  As a whole,
> > > > the approach taken by the patch is not really consistent with the
> > > > rest.
> > 
> > > I agree that's better to have a generic solution and to be consistent with
> > > the other variable-numbered stats. 
> > > 
> > > The attached is implementing in 0001 the proposition done in [1], i.e:
> > > 
> > > 1. It adds a new allow_critical_section to PgStat_KindInfo for pgstats kinds
> > > 2. It ensures to set temporarly allowInCritSection to true when needed
> > >
> > > Note that for safety reason 0001 does set allowInCritSection back to false
> > > unconditionally (means not checking again for allow_critical_section).
> > 
> > This is a preposterously bad idea.  The restriction to not allocate memory in
> > critical sections exists for a reason,
> 
> Thanks for sharing your thoughts on it. In [1], you said:
> 
> "
> 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
> "
> 
> So, do you think that the initial proposal that has been made here (See R1. in
> [2]) i.e make use of a new PendingBackendWalStats variable:

Well, I think this first needs be fixed for for the IO stats change made in

commit 9aea73fc61d
Author: Michael Paquier <michael@paquier.xyz>
Date:   2024-12-19 13:19:22 +0900
 
    Add backend-level statistics to pgstats


Once we have a pattern to model after, we can apply the same scheme here.


> "
> 0003 does not rely on pgstat_prep_backend_pending() for its pending statistics
> but on a new PendingBackendWalStats variable. The reason is that the pending wal
> statistics are incremented in a critical section (see XLogWrite(), and so
> a call to pgstat_prep_pending_entry() could trigger a failed assertion:
> MemoryContextAllocZero()->"CritSectionCount == 0 || (context)->allowInCritSection"
> "
> 
> and implemented up to v4 is a viable approach?

Yes-ish.  I think it would be better to make it slightly more general than
that, handling this for all types of backend stats, not just for WAL.

Greetings,

Andres Freund