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 <jreidthomp...@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.thomp...@crunchydata.com
www.crunchydata.com
