Re: Enhancing Memory Context Statistics Reporting

Hi,

On 2025-04-07 21:57:57 +0530, Rahila Syed wrote:
> > > diff --git a/src/backend/postmaster/auxprocess.c
> > b/src/backend/postmaster/auxprocess.c
> > > index 4f6795f7265..d3b4df27935 100644
> > > --- a/src/backend/postmaster/auxprocess.c
> > > +++ b/src/backend/postmaster/auxprocess.c
> > > @@ -84,6 +84,13 @@ AuxiliaryProcessMainCommon(void)
> > > /* register a before-shutdown callback for LWLock cleanup */
> > > before_shmem_exit(ShutdownAuxiliaryProcess, 0);
> > >
> > > + /*
> > > + * The before shmem exit callback frees the DSA memory occupied by
> > the
> > > + * latest memory context statistics that could be published by
> > this aux
> > > + * proc if requested.
> > > + */
> > > + before_shmem_exit(AtProcExit_memstats_dsa_free, 0);
> > > +
> > > SetProcessingMode(NormalProcessing);
> > > }
> >
> > How about putting it into BaseInit()? Or maybe just register it when its
> > first used?
> >
> >
> Problem with registering it when dsa is first used is that dsa is used in an
> interrupt handler. The handler could be called from the
> PG_ENSURE_ERROR_CLEANUP block. This block operates under the assumption that
> the before_shmem_exit callback registered at the beginning, will be the last
> one in the registered callback list at the end of the block. However, this
> won't be the case if a callback is registered from an interrupt handler
> called in the PG_ENSURE_ERROR_CLEANUP block.

Ugh, I really dislike PG_ENSURE_ERROR_CLEANUP().

That's not an argument against moving it to BaseInit() though, as that's
called before procsignal is even initialized and before signals are unmasked.

> I don't really understand why DSA_DEFAULT_INIT_SEGMENT_SIZE is
>
> something that makes sense to use here?
> >
> >
> To determine the memory limit per backend in multiples of
> DSA_DEFAULT_INIT_SEGMENT_SIZE.
> Currently it is set to 1 * DSA_DEFAULT_INIT_SEGMENT_SIZE.
> Since a call to dsa_create would create a DSA segment of this size, I
> thought it makes sense
> to define a limit related to the segment size.

I strongly disagree. The limit should be in an understandable unit, not on
another subystems's defaults that might change at some point.

> > + /* If the dsm mapping could not be found, attach to the area */
> > > + if (dsm_seg != NULL)
> > > + return;
> >
> > I don't understand what we do here with the dsm? Why do we not need
> > cleanup
> > if we are already attached to the dsm segment?
> >
>
> I am not expecting to hit this case, since we are always detaching from the
> dsa.

Pretty sure it's reachable, consider a failure of dsa_allocate(). That'll
throw an error, while attached to the segment.

> This could be an assert but since it is a cleanup code, I thought returning
> would be a harmless step.

The problem is that the code seems wrong - if we are already attached we'll
leak the memory!

As I also mentioned, I don't understand why we're constantly
attaching/detaching from the dsa/dsm either. It just seems to make things more
complicated an dmore expensive.

Greetings,

Andres Freund