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

On Wed, 2022-11-09 at 09:23 -0500, Reid Thompson wrote:
> 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:
> >
> > 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.

done

>
> > 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

done

> > > +/* --------
> > > + * 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.

done

>
> > > +       /*
> > > +        * 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

done

> > > 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.

done

> > >  /* ----------
> > >   * 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.

done

patch attached to
https://www.postgresql.org/message-id/06b4922193b80776a31e08a3809f2414b0d4bf90.camel%40crunchydata.com
--
Reid Thompson
Senior Software Engineer
Crunchy Data, Inc.

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