Re: Add tracking of backend memory allocated to pg_stat_activity - Mailing list pgsql-hackers

Hi Melanie,
Thank you for looking at this and for the feedback. Responses inline
below.

On Mon, 2022-11-07 at 16:17 -0500, Melanie Plageman wrote:
> On Fri, Nov 04, 2022 at 08:56:13AM -0400, Reid Thompson wrote:
> > From a8de5d29c0c6f10962181926a49ad4fec1e52bd1 Mon Sep 17 00:00:00
> > 2001
> > From: Reid Thompson <jreidthompson@nc.rr.com>
> > Date: Thu, 11 Aug 2022 12:01:25 -0400
> > Subject: [PATCH] Add tracking of backend memory allocated to pg_stat_activity
> > +
>
> It doesn't seem like you need the backend_ prefix in the view since
> that is implied by it being in pg_stat_activity.

I will remove the prefix.

> For the wording on the description, I find "memory allocated to this
> backend" a bit confusing. Perhaps you could reword it to make clear
> you mean that the number represents the balance of allocations by this
> backend. Something like:
>
>         Memory currently allocated to this backend in bytes. This is the
>         balance of bytes allocated and freed by this backend.
> I would also link to the system administration function
> pg_size_pretty() so users know how to easily convert the value.

Thanks, I'll make these changes

> If you end up removing shared memory as Andres suggests in [1], I
> would link pg_shmem_allocations view here and point out that shared memory
> allocations can be viewed there instead (and why).
>
> You could instead add dynamic shared memory allocation to the
> pg_shmem_allocations view as suggested as follow-on work by the
> commit which introduced it, ed10f32e3.
>
> > +/* --------
> > + * pgstat_report_backend_allocated_bytes_increase() -
> > + *
> > + * Called to report increase in memory allocated for this backend
> > + * --------
> > + */
>
> It seems like you could combine the
> pgstat_report_backend_allocated_bytes_decrease/increase() by either
> using a signed integer to represent the allocation/deallocation or passing in
> a "direction" that is just a positive or negative multiplier enum.
>
> Especially if you don't use the write barriers, I think you could
> simplify the logic in the two functions.
>
> If you do combine them, you might shorten the name to
> pgstat_report_backend_allocation() or pgstat_report_allocation().

Agreed. This seems a cleaner, simpler way to go.  I'll add it to the
TODO list.

> > +       /*
> > +        * Do not allow backend_allocated_bytes to go below zero.
> > ereport if we
> > +        * would have. There's no need for a lock around the read
> > here as it's
> > +        * being referenced from the same backend which means that
> > there shouldn't
> > +        * be concurrent writes. We want to generate an ereport in
> > these cases.
> > +        */
> > +       if (deallocation > beentry->backend_allocated_bytes)
> > +       {
> > +               ereport(LOG, errmsg("decrease reduces reported
> > backend memory allocated below zero; setting reported to 0"));
> > +
>
> I also think it would be nice to include the deallocation amount and
> backend_allocated_bytes amount in the log message.
> It also might be nice to start the message with something more clear
> than "decrease".
> For example, I would find this clear as a user:
>
>         Backend [backend_type or pid] deallocated [deallocation number] bytes,
>         [backend_allocated_bytes - deallocation number] more than this backend
>         has reported allocating.

Sounds good, I'll implement these changes

> > diff --git a/src/backend/utils/adt/pgstatfuncs.c
> > b/src/backend/utils/adt/pgstatfuncs.c
> > index 96bffc0f2a..b6d135ad2f 100644
> > --- a/src/backend/utils/adt/pgstatfuncs.c
> > +++ b/src/backend/utils/adt/pgstatfuncs.c
> > @@ -553,7 +553,7 @@ pg_stat_get_progress_info(PG_FUNCTION_ARGS)
> >  Datum
> >  pg_stat_get_activity(PG_FUNCTION_ARGS)
> >  {
> > -#define PG_STAT_GET_ACTIVITY_COLS      30
> > +#define PG_STAT_GET_ACTIVITY_COLS      31
> >         int                     num_backends =
> >  
> > +               values[30] = UInt64GetDatum(beentry->backend_allocated_bytes);
>
> Though not the fault of this patch, it is becoming very difficult to
> keep the columns straight in pg_stat_get_activity(). Perhaps you
> could add a separate commit to add an enum for the columns so the function
> is easier to understand.
>
> > diff --git a/src/include/utils/backend_status.h
> > b/src/include/utils/backend_status.h
> > index b582b46e9f..75d87e8308 100644
> > --- a/src/include/utils/backend_status.h
> > +++ b/src/include/utils/backend_status.h
> > @@ -169,6 +169,9 @@ typedef struct PgBackendStatus
> >  
> >         /* query identifier, optionally computed using
> > post_parse_analyze_hook */
> >         uint64          st_query_id;
> > +
> > +       /* Current memory allocated to this backend */
> > +       uint64          backend_allocated_bytes;
> >  } PgBackendStatus;
>
> I don't think you need the backend_ prefix here since it is in
> PgBackendStatus.

Agreed again, I'll remove the prefix.

> > @@ -313,7 +316,9 @@ extern const char
> > *pgstat_get_backend_current_activity(int pid, bool checkUser);
> >  extern const char *pgstat_get_crashed_backend_activity(int pid,
> > char *buffer,
> >                                                                    
> >                                         int buflen);
> >  extern uint64 pgstat_get_my_query_id(void);
> > -
> > +extern void pgstat_report_backend_allocated_bytes_increase(uint64
> > allocation);
> > +extern void pgstat_report_backend_allocated_bytes_decrease(uint64
> > deallocation);
> > +extern uint64 pgstat_get_all_backend_memory_allocated(void);
> >  
> >  /* ----------
> >   * Support functions for the SQL-callable functions to
> > diff --git a/src/test/regress/expected/rules.out
> > b/src/test/regress/expected/rules.out
> > index 624d0e5aae..ba9f494806 100644
> > --- a/src/test/regress/expected/rules.out
> > +++ b/src/test/regress/expected/rules.out
> > @@ -1753,10 +1753,11 @@ pg_stat_activity| SELECT s.datid,
> >      s.state,
> >      s.backend_xid,
> >      s.backend_xmin,
> > +    s.backend_allocated_bytes,
> >      s.query_id,
> >      s.query,
> >      s.backend_type
>
> Seems like it would be possible to add a functional test to
> stats.sql.

I will look at adding this.


> - Melanie
>
> [1]
> https://www.postgresql.org/message-id/20221105024146.xxlbtsxh2niyz2fu%40awork3.anarazel.de

Thanks again,
Reid

--
Reid Thompson
Senior Software Engineer
Crunchy Data, Inc.

reid.thompson@crunchydata.com
www.crunchydata.com