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

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