Re: Add tracking of backend memory allocated to pg_stat_activity

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(at)nc(dot)rr(dot)com>
> Date: Thu, 11 Aug 2022 12:01:25 -0400
> Subject: [PATCH] Add tracking of backend memory allocated to pg_stat_activity
>
> This new field displays the current bytes of memory allocated to the
> backend process. It is updated as memory for the process is
> malloc'd/free'd. Memory allocated to items on the freelist is included in
> the displayed value. Dynamic shared memory allocations are included
> only in the value displayed for the backend that created them, they are
> not included in the value for backends that are attached to them to
> avoid double counting. On occasion, orphaned memory segments may be
> cleaned up on postmaster startup. This may result in decreasing the sum
> without a prior increment. We limit the floor of backend_mem_allocated
> to zero. Updated pg_stat_activity documentation for the new column.
> ---
> doc/src/sgml/monitoring.sgml | 12 +++
> src/backend/catalog/system_views.sql | 1 +
> src/backend/storage/ipc/dsm_impl.c | 81 +++++++++++++++
> src/backend/utils/activity/backend_status.c | 105 ++++++++++++++++++++
> src/backend/utils/adt/pgstatfuncs.c | 4 +-
> src/backend/utils/mmgr/aset.c | 18 ++++
> src/backend/utils/mmgr/generation.c | 15 +++
> src/backend/utils/mmgr/slab.c | 21 ++++
> src/include/catalog/pg_proc.dat | 6 +-
> src/include/utils/backend_status.h | 7 +-
> src/test/regress/expected/rules.out | 9 +-
> 11 files changed, 270 insertions(+), 9 deletions(-)
>
> diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
> index e5d622d514..972805b85a 100644
> --- a/doc/src/sgml/monitoring.sgml
> +++ b/doc/src/sgml/monitoring.sgml
> @@ -947,6 +947,18 @@ postgres 27093 0.0 0.0 30096 2752 ? Ss 11:34 0:00 postgres: ser
> </para></entry>
> </row>
>
> + <row>
> + <entry role="catalog_table_entry"><para role="column_definition">
> + <structfield>backend_allocated_bytes</structfield> <type>bigint</type>
> + </para>
> + <para>
> + The byte count of memory allocated to this backend. Dynamic shared memory
> + allocations are included only in the value displayed for the backend that
> + created them, they are not included in the value for backends that are
> + attached to them to avoid double counting.
> + </para></entry>
> + </row>
> +

It doesn't seem like you need the backend_ prefix in the view since that
is implied by it being in pg_stat_activity.

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.

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

> +void
> +pgstat_report_backend_allocated_bytes_increase(uint64 allocation)
> +{
> + volatile PgBackendStatus *beentry = MyBEEntry;
> +
> + if (!beentry || !pgstat_track_activities)
> + {
> + /*
> + * Account for memory before pgstats is initialized. This will be
> + * migrated to pgstats on initialization.
> + */
> + backend_allocated_bytes += allocation;
> +
> + return;
> + }
> +
> + /*
> + * Update my status entry, following the protocol of bumping
> + * st_changecount before and after. We use a volatile pointer here to
> + * ensure the compiler doesn't try to get cute.
> + */
> + PGSTAT_BEGIN_WRITE_ACTIVITY(beentry);
> + beentry->backend_allocated_bytes += allocation;
> + PGSTAT_END_WRITE_ACTIVITY(beentry);
> +}
> +
> +/* --------
> + * pgstat_report_backend_allocated_bytes_decrease() -
> + *
> + * Called to report decrease in memory allocated for this backend
> + * --------
> + */
> +void
> +pgstat_report_backend_allocated_bytes_decrease(uint64 deallocation)
> +{
> + volatile PgBackendStatus *beentry = MyBEEntry;
> +
> + /*
> + * Cases may occur where shared memory from a previous postmaster
> + * invocation still exist. These are cleaned up at startup by
> + * dsm_cleanup_using_control_segment. Limit decreasing memory allocated to
> + * zero in case no corresponding prior increase exists or decrease has
> + * already been accounted for.
> + */
> +
> + if (!beentry || !pgstat_track_activities)
> + {
> + /*
> + * Account for memory before pgstats is initialized. This will be
> + * migrated to pgstats on initialization. Do not allow
> + * backend_allocated_bytes to go below zero. If pgstats has not been
> + * initialized, we are in startup and we set backend_allocated_bytes
> + * to zero in cases where it would go negative and skip generating an
> + * ereport.
> + */
> + if (deallocation > backend_allocated_bytes)
> + backend_allocated_bytes = 0;
> + else
> + backend_allocated_bytes -= deallocation;
> +
> + return;
> + }
> +
> + /*
> + * 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.

> 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 = pgstat_fetch_stat_numbackends();
> int curr_backend;
> int pid = PG_ARGISNULL(0) ? -1 : PG_GETARG_INT32(0);
> @@ -609,6 +609,8 @@ pg_stat_get_activity(PG_FUNCTION_ARGS)
> else
> nulls[16] = true;
>
> + 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.

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

- Melanie

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