Re: Add tracking of backend memory allocated to pg_stat_activity
On Thu, Aug 31, 2023 at 9:19 AM John Morris
wrote:
> Here is an updated version of the earlier work.
>
> This version:
>1) Tracks memory as requested by the backend.
>2) Includes allocations made during program startup.
>3) Optimizes the "fast path" to only update two local variables.
>4) Places a cluster wide limit on total memory allocated.
>
> The cluster wide limit is useful for multi-hosting. One greedy cluster
> doesn't starve
> the other clusters of memory.
>
> Note there isn't a good way to track actual memory used by a cluster.
> Ideally, we like to get the working set size of each memory segment along
> with
> the size of the associated kernel data structures.
> Gathering that info in a portable way is a "can of worms".
> Instead, we're managing memory as requested by the application.
> While not identical, the two approaches are strongly correlated.
>
> The memory model used is
>1) Each process is assumed to use a certain amount of memory
>simply by existing.
>2) All pg memory allocations are counted, including those before
>the process is fully initialized.
> 3) Each process maintains its own local counters. These are the
> "truth".
> 4) Periodically,
> - local counters are added into the global, shared memory
> counters.
> - pgstats is updated
> - total memory is checked.
>
> For efficiency, the global total is an approximation, not a precise number.
> It can be off by as much as 1 MB per process. Memory limiting
> doesn't need precision, just a consistent and reasonable approximation.
>
> Repeating the earlier benchmark test, there is no measurable loss of
> performance.
>
> Hi,
In `InitProcGlobal`:
+elog(WARNING, "proc init: max_total=%d result=%d\n",
max_total_bkend_mem, result);
Is the above log for debugging purposes ? Maybe the log level should be
changed.
+
errmsg("max_total_backend_memory %dMB - shared_memory_size %dMB is <=
100MB",
The `<=` in the error message is inconsistent with the `max_total_bkend_mem
< result + 100` check.
Please modify one of them.
For update_global_allocation :
+
Assert((int64)pg_atomic_read_u64(>total_bkend_mem_bytes) >= 0);
+
Assert((int64)pg_atomic_read_u64(>global_dsm_allocation) >= 0);
Should the assertions be done earlier in the function?
For reserve_backend_memory:
+ return true;
+
+ /* CASE: the new allocation is within bounds. Take the fast path. */
+ else if (my_memory.allocated_bytes + size <= allocation_upper_bound)
The `else` can be omitted (the preceding if block returns).
For `atomic_add_within_bounds_i64`
+ newval = oldval + add;
+
+ /* check if we are out of bounds */
+ if (newval < lower_bound || newval > upper_bound ||
addition_overflow(oldval, add))
Since the summation is stored in `newval`, you can pass newval to
`addition_overflow` so that `addition_overflow` doesn't add them again.
There are debug statements, such as:
+ debug("done\n");
you can drop them in the next patch.
Thanks
Re: Add tracking of backend memory allocated to pg_stat_activity
On Thu, 2023-01-05 at 14:13 -0600, Justin Pryzby wrote: > > I suggest to close the associated CF entry. Closed with status of Withdrawn. If that is invalid, please advise. > (Also, the people who participated in this thread may want to be > included in the other thread going forward.) Explicitly adding previously missed participants to Cc: - that conversation/patches are being consolidated to the thread "Add the ability to limit the amount of memory that can be allocated to backends" https://www.postgresql.org/message-id/bd57d9a4c219cc1392665fd5fba61dde8027b3da.ca...@crunchydata.com Thanks, Reid -- Reid Thompson Senior Software Engineer Crunchy Data, Inc. reid.thomp...@crunchydata.com www.crunchydata.com
Re: Add tracking of backend memory allocated to pg_stat_activity
On Thu, Jan 05, 2023 at 01:58:33PM -0500, Reid Thompson wrote: > On Tue, 2023-01-03 at 16:25 +0530, vignesh C wrote: > > ... > > The patch does not apply on top of HEAD as in [1], please post a > > rebased patch: > >... > > Regards, > > Vignesh > > Per conversation in thread listed below, patches have been submitted to the > "Add the ability to limit the amount of memory that can be allocated to > backends" thread > https://www.postgresql.org/message-id/bd57d9a4c219cc1392665fd5fba61dde8027b3da.ca...@crunchydata.com I suggest to close the associated CF entry. (Also, the people who participated in this thread may want to be included in the other thread going forward.) > 0001-Add-tracking-of-backend-memory-allocated-to-pg_stat_.patch > 0002-Add-the-ability-to-limit-the-amount-of-memory-that-c.patch > > On Thu, 8 Dec 2022 at 19:44, Reid Thompson > wrote: > > > > On Sun, 2022-11-27 at 09:40 -0600, Justin Pryzby wrote: > > > > ... > > > > I still wonder whether there needs to be a separate CF entry for > > > > the 0001 patch. One issue is that there's two different lists of > > > > people involved in the threads. > > > > > > > > I'm OK with containing the conversation to one thread if everyone else > > is. If there's no argument against, then patches after today will go > > to the "Add the ability to limit the amount of memory that can be > > allocated to backends" thread > > https://www.postgresql.org/message-id/bd57d9a4c219cc1392665fd5fba61dde8027b3da.ca...@crunchydata.com
Re: Add tracking of backend memory allocated to pg_stat_activity
On Tue, 2023-01-03 at 16:25 +0530, vignesh C wrote: > ... > The patch does not apply on top of HEAD as in [1], please post a > rebased patch: >... > Regards, > Vignesh Per conversation in thread listed below, patches have been submitted to the "Add the ability to limit the amount of memory that can be allocated to backends" thread https://www.postgresql.org/message-id/bd57d9a4c219cc1392665fd5fba61dde8027b3da.ca...@crunchydata.com 0001-Add-tracking-of-backend-memory-allocated-to-pg_stat_.patch 0002-Add-the-ability-to-limit-the-amount-of-memory-that-c.patch On Thu, 8 Dec 2022 at 19:44, Reid Thompson wrote: > > On Sun, 2022-11-27 at 09:40 -0600, Justin Pryzby wrote: > > > ... > > > I still wonder whether there needs to be a separate CF entry for > > > the 0001 patch. One issue is that there's two different lists of > > > people involved in the threads. > > > > > I'm OK with containing the conversation to one thread if everyone else > is. If there's no argument against, then patches after today will go > to the "Add the ability to limit the amount of memory that can be > allocated to backends" thread > https://www.postgresql.org/message-id/bd57d9a4c219cc1392665fd5fba61dde8027b3da.ca...@crunchydata.com -- Reid Thompson Senior Software Engineer Crunchy Data, Inc. reid.thomp...@crunchydata.com www.crunchydata.com
Re: Add tracking of backend memory allocated to pg_stat_activity
On Thu, 8 Dec 2022 at 19:44, Reid Thompson wrote: > > On Sun, 2022-11-27 at 09:40 -0600, Justin Pryzby wrote: > > > BTW, these should have some kind of prefix, like PG_ALLOC_* to > > > avoid causing the same kind of problem for someone else that > > > another header caused for you by defining something somewhere > > > called IGNORE (ignore what, I don't know). The other problem was > > > probably due to a define, though. Maybe instead of an enum, the > > > function should take a boolean. > > > > > Patch updated to current master and includes above prefix > recommendation and combining of two function calls to one recommended > by Ted Yu. > > > > > > > I still wonder whether there needs to be a separate CF entry for > > > the 0001 patch. One issue is that there's two different lists of > > > people involved in the threads. > > > > > I'm OK with containing the conversation to one thread if everyone else > is. If there's no argument against, then patches after today will go > to the "Add the ability to limit the amount of memory that can be > allocated to backends" thread > https://www.postgresql.org/message-id/bd57d9a4c219cc1392665fd5fba61dde8027b3da.ca...@crunchydata.com The patch does not apply on top of HEAD as in [1], please post a rebased patch: === Applying patches on top of PostgreSQL commit ID e351f85418313e97c203c73181757a007dfda6d0 === === applying patch ./0001-Add-tracking-of-backend-memory-allocated-to-pg_stat_.patch patching file src/backend/utils/mmgr/slab.c Hunk #1 succeeded at 69 (offset 16 lines). Hunk #2 succeeded at 414 (offset 175 lines). Hunk #3 succeeded at 436 with fuzz 2 (offset 176 lines). Hunk #4 FAILED at 286. Hunk #5 succeeded at 488 (offset 186 lines). Hunk #6 FAILED at 381. Hunk #7 FAILED at 554. 3 out of 7 hunks FAILED -- saving rejects to file src/backend/utils/mmgr/slab.c.rej [1] - http://cfbot.cputube.org/patch_41_3865.log Regards, Vignesh
Re: Add tracking of backend memory allocated to pg_stat_activity
On Sun, 2022-11-27 at 09:40 -0600, Justin Pryzby wrote: > > BTW, these should have some kind of prefix, like PG_ALLOC_* to > > avoid causing the same kind of problem for someone else that > > another header caused for you by defining something somewhere > > called IGNORE (ignore what, I don't know). The other problem was > > probably due to a define, though. Maybe instead of an enum, the > > function should take a boolean. > > Patch updated to current master and includes above prefix recommendation and combining of two function calls to one recommended by Ted Yu. > > > > I still wonder whether there needs to be a separate CF entry for > > the 0001 patch. One issue is that there's two different lists of > > people involved in the threads. > > I'm OK with containing the conversation to one thread if everyone else is. If there's no argument against, then patches after today will go to the "Add the ability to limit the amount of memory that can be allocated to backends" thread https://www.postgresql.org/message-id/bd57d9a4c219cc1392665fd5fba61dde8027b3da.ca...@crunchydata.com -- Reid Thompson Senior Software Engineer Crunchy Data, Inc. reid.thomp...@crunchydata.com www.crunchydata.com From fdb7e6d5bb653e9c5031fd058bf168bdf80a20eb Mon Sep 17 00:00:00 2001 From: Reid Thompson 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| 15 src/backend/catalog/system_views.sql| 1 + src/backend/postmaster/autovacuum.c | 6 ++ src/backend/postmaster/postmaster.c | 13 src/backend/postmaster/syslogger.c | 3 + src/backend/storage/ipc/dsm_impl.c | 81 + src/backend/utils/activity/backend_status.c | 45 src/backend/utils/adt/pgstatfuncs.c | 4 +- src/backend/utils/mmgr/aset.c | 17 + 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 | 59 ++- src/test/regress/expected/rules.out | 9 ++- src/test/regress/expected/stats.out | 11 +++ src/test/regress/sql/stats.sql | 3 + 16 files changed, 300 insertions(+), 9 deletions(-) diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml index 11a8ebe5ec..13ecbe5877 100644 --- a/doc/src/sgml/monitoring.sgml +++ b/doc/src/sgml/monitoring.sgml @@ -948,6 +948,21 @@ postgres 27093 0.0 0.0 30096 2752 ?Ss 11:34 0:00 postgres: ser + + + allocated_bytes bigint + + + Memory currently allocated to this backend in bytes. This is the balance + of bytes allocated and freed by 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. Use pg_size_pretty + described in to make this value + more easily readable. + + + query text diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql index 2d8104b090..9ea8f78c95 100644 --- a/src/backend/catalog/system_views.sql +++ b/src/backend/catalog/system_views.sql @@ -864,6 +864,7 @@ CREATE VIEW pg_stat_activity AS S.state, S.backend_xid, s.backend_xmin, +S.allocated_bytes, S.query_id, S.query, S.backend_type diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c index 0746d80224..f6b6f71cdb 100644 --- a/src/backend/postmaster/autovacuum.c +++ b/src/backend/postmaster/autovacuum.c @@ -407,6 +407,9 @@ StartAutoVacLauncher(void) #ifndef EXEC_BACKEND case 0: + /* Zero allocated bytes to avoid double counting parent allocation */ + pgstat_zero_my_allocated_bytes(); + /* in postmaster child ... */ InitPostmasterChild(); @@ -1485,6 +1488,9 @@ StartAutoVacWorker(void) #ifndef EXEC_BACKEND case 0: + /* Zero allocated bytes to avoid double counting parent allocation
Re: Add tracking of backend memory allocated to pg_stat_activity
Hi, On 2022-12-06 08:47:55 -0500, Reid Thompson wrote: > The intent was to capture and display all the memory allocated to the > backends, for admins/users/max_total_backend_memory/others to utilize. It's going to be far less accurate than that. For one, there's a lot of operating system resources, like the page table, that are going to be ignored. We're also not going to capture allocations done directly via malloc(). There's also copy-on-write effects that we're ignoring. If we just want to show an accurate picture of the current memory usage, we need to go to operating system specific interfaces. > Why should we ignore the allocations prior to backend_status.c? It seems to add complexity without a real increase in accuracy to me. But I'm not going to push harder on it than I already have. Greetings, Andres Freund
Re: Add tracking of backend memory allocated to pg_stat_activity
On Fri, 2022-12-02 at 09:18 -0800, Andres Freund wrote: > Hi, > > On 2022-12-02 11:09:30 -0500, Reid Thompson wrote: > > It appears to me that Postmaster populates the local variable that > > *my_allocated_bytes points to. That allocation is passed to forked > > children, and if not zeroed out, will be double counted as part of > > the child allocation. Is this invalid? > > I don't think we should count allocations made before > backend_status.c has > been initialized. > > Greetings, > > Andres Freund Hi, The intent was to capture and display all the memory allocated to the backends, for admins/users/max_total_backend_memory/others to utilize. Why should we ignore the allocations prior to backend_status.c? Thanks, Reid -- Reid Thompson Senior Software Engineer Crunchy Data, Inc. reid.thomp...@crunchydata.com www.crunchydata.com
Re: Add tracking of backend memory allocated to pg_stat_activity
Hi, On 2022-12-02 11:09:30 -0500, Reid Thompson wrote: > It appears to me that Postmaster populates the local variable that > *my_allocated_bytes points to. That allocation is passed to forked > children, and if not zeroed out, will be double counted as part of > the child allocation. Is this invalid? I don't think we should count allocations made before backend_status.c has been initialized. Greetings, Andres Freund
Re: Add tracking of backend memory allocated to pg_stat_activity
On Mon, 2022-11-28 at 10:59 -0800, Andres Freund wrote: > On 2022-11-26 22:10:06 -0500, Reid Thompson wrote: > > - zero allocated bytes after fork to avoid double counting > > postmaster allocations > > I still don't understand this - postmaster shouldn't be counted at > all. It > doesn't have a PgBackendStatus. There simply shouldn't be any tracked > allocations from it. > > Greetings, > > Andres Freund Hi Andres, I based this on the following. It appears to me that Postmaster populates the local variable that *my_allocated_bytes points to. That allocation is passed to forked children, and if not zeroed out, will be double counted as part of the child allocation. Is this invalid? $ ps -ef|grep postgres postgres6389 1 0 Dec01 ?00:00:17 /usr/sbin/pgbouncer -d /etc/pgbouncer/pgbouncer.ini rthompso 2937799 1 0 09:45 ?00:00:00 /tmp/postgres/install/pg-stats-memory/bin/postgres -D /var/tmp/pg-stats-memory -p 5433 rthompso 2937812 2937799 0 09:45 ?00:00:00 postgres: checkpointer rthompso 2937813 2937799 0 09:45 ?00:00:00 postgres: background writer rthompso 2937816 2937799 0 09:45 ?00:00:00 postgres: walwriter rthompso 2937817 2937799 0 09:45 ?00:00:00 postgres: autovacuum launcher rthompso 2937818 2937799 0 09:45 ?00:00:00 postgres: logical replication launcher rthompso 2938877 2636586 0 09:46 pts/400:00:00 /usr/lib/postgresql/12/bin/psql -h localhost -p 5433 postgres rthompso 2938909 2937799 0 09:46 ?00:00:00 postgres: rthompso postgres 127.0.0.1(44532) idle rthompso 2942164 1987403 0 09:49 pts/300:00:00 grep postgres Bracketing fork_process() calls with logging to print *my_allocated_bytes immediately prior and after fork_process... To me, this indicates that the forked children for this invocation (checkpointer, walwriter, autovac launcher, client backend, autovac worker, etc) are inheriting 240672 bytes from postmaster. $ ccat logfile 2022-12-02 09:45:05.871 EST [2937799] LOG: starting PostgreSQL 16devel on x86_64-pc-linux-gnu, compiled by gcc (Ubuntu 9.4.0-1ubuntu1~20.04.1) 9.4.0, 64-bit 2022-12-02 09:45:05.872 EST [2937799] LOG: listening on IPv4 address "127.0.0.1", port 5433 2022-12-02 09:45:05.874 EST [2937799] LOG: listening on Unix socket "/tmp/.s.PGSQL.5433" parent StartChildProcess process. pid: 2937799 *my_allocated_bytes: 240672 parent StartChildProcess process. pid: 2937799 *my_allocated_bytes: 240672 parent StartChildProcess process. pid: 2937799 *my_allocated_bytes: 240672 child StartChildProcess process. pid: 2937812 *my_allocated_bytes: 240672 child StartChildProcess process. pid: 2937813 *my_allocated_bytes: 240672 child StartChildProcess process. pid: 2937814 *my_allocated_bytes: 240672 2022-12-02 09:45:05.884 EST [2937814] LOG: database system was shut down at 2022-12-02 09:41:13 EST parent StartChildProcess process. pid: 2937799 *my_allocated_bytes: 240672 parent StartAutoVacLauncher process. pid: 2937799 *my_allocated_bytes: 240672 child StartChildProcess process. pid: 2937816 *my_allocated_bytes: 240672 parent do_start_bgworker process. pid: 2937799 *my_allocated_bytes: 240672 child StartAutoVacLauncher process. pid: 2937817 *my_allocated_bytes: 240672 2022-12-02 09:45:05.889 EST [2937799] LOG: database system is ready to accept connections child do_start_bgworker process. pid: 2937818 *my_allocated_bytes: 240672 parent StartAutoVacWorker process. pid: 2937799 *my_allocated_bytes: 240672 child StartAutoVacWorker process. pid: 2938417 *my_allocated_bytes: 240672 parent BackendStartup process. pid: 2937799 *my_allocated_bytes: 240672 child BackendStartup process. pid: 2938909 *my_allocated_bytes: 240672 parent StartAutoVacWorker process. pid: 2937799 *my_allocated_bytes: 240672 child StartAutoVacWorker process. pid: 2938910 *my_allocated_bytes: 240672 parent StartAutoVacWorker process. pid: 2937799 *my_allocated_bytes: 240672 child StartAutoVacWorker process. pid: 2939340 *my_allocated_bytes: 240672 parent StartAutoVacWorker process. pid: 2937799 *my_allocated_bytes: 240672 child StartAutoVacWorker process. pid: 2939665 *my_allocated_bytes: 240672 parent StartAutoVacWorker process. pid: 2937799 *my_allocated_bytes: 240672 child StartAutoVacWorker process. pid: 2940038 *my_allocated_bytes: 240672 parent StartAutoVacWorker process. pid: 2937799 *my_allocated_bytes: 240672 child StartAutoVacWorker process. pid: 2940364 *my_allocated_bytes: 240672 parent StartAutoVacWorker process. pid: 2937799 *my_allocated_bytes: 240672 child StartAutoVacWorker process. pid: 2940698 *my_allocated_bytes: 240672 parent StartAutoVacWorker process. pid: 2937799 *my_allocated_bytes: 240672 child StartAutoVacWorker process. pid: 2941317 *my_allocated_bytes: 240672 parent StartAutoVacWorker process. pid: 2937799 *my_allocated_bytes: 240672 child StartAutoVacWorker process. pid: 2941825 *my_allocated_bytes: 240672 Reid Thompson Senior Software Engineer Crunchy Data, Inc.
Re: Add tracking of backend memory allocated to pg_stat_activity
On 2022-11-26 22:10:06 -0500, Reid Thompson wrote: >- zero allocated bytes after fork to avoid double counting postmaster > allocations I still don't understand this - postmaster shouldn't be counted at all. It doesn't have a PgBackendStatus. There simply shouldn't be any tracked allocations from it. Greetings, Andres Freund
Re: Add tracking of backend memory allocated to pg_stat_activity
On Sun, Nov 27, 2022 at 7:41 AM Justin Pryzby wrote:
> On Sun, Nov 27, 2022 at 12:32:19AM -0500, Reid Thompson wrote:
> > @@ -32,6 +33,12 @@ typedef enum BackendState
> > STATE_DISABLED
> > } BackendState;
> >
> > +/* Enum helper for reporting memory allocated bytes */
> > +enum allocation_direction
> > +{
> > + DECREASE = -1,
> > + INCREASE = 1,
> > +};
>
> BTW, these should have some kind of prefix, like PG_ALLOC_* to avoid
> causing the same kind of problem for someone else that another header
> caused for you by defining something somewhere called IGNORE (ignore
> what, I don't know). The other problem was probably due to a define,
> though. Maybe instead of an enum, the function should take a boolean.
>
> I still wonder whether there needs to be a separate CF entry for the
> 0001 patch. One issue is that there's two different lists of people
> involved in the threads.
>
> --
> Justin
>
>
> I am a bit curious: why is the allocation_direction enum needed ?
pgstat_report_allocated_bytes() can be given the amount (either negative or
positive) to adjust directly.
Cheers
Re: Add tracking of backend memory allocated to pg_stat_activity
On Sun, Nov 27, 2022 at 12:32:19AM -0500, Reid Thompson wrote:
> @@ -32,6 +33,12 @@ typedef enum BackendState
> STATE_DISABLED
> } BackendState;
>
> +/* Enum helper for reporting memory allocated bytes */
> +enum allocation_direction
> +{
> + DECREASE = -1,
> + INCREASE = 1,
> +};
BTW, these should have some kind of prefix, like PG_ALLOC_* to avoid
causing the same kind of problem for someone else that another header
caused for you by defining something somewhere called IGNORE (ignore
what, I don't know). The other problem was probably due to a define,
though. Maybe instead of an enum, the function should take a boolean.
I still wonder whether there needs to be a separate CF entry for the
0001 patch. One issue is that there's two different lists of people
involved in the threads.
--
Justin
Re: Add tracking of backend memory allocated to pg_stat_activity
On Sat, Nov 26, 2022 at 9:32 PM Reid Thompson
wrote:
> On Sat, 2022-11-26 at 22:10 -0500, Reid Thompson wrote:
> > Code rebased to current master.
> > Updated to incorporate additional recommendations from the the list
> >- add units to variables in view
> >- remove 'backend_' prefix from variables/functions
> >- update documentation
> >- add functional test for allocated_bytes
> >- refactor allocation reporting to reduce number of functions and
> > branches/reduce performance hit
> >- zero allocated bytes after fork to avoid double counting
> > postmaster allocations
> >
> >
> >
> >
>
> attempt to remedy cfbot windows build issues
>
>
> Hi,
+ if (request_size > *mapped_size)
+ {
+ pgstat_report_allocated_bytes(*mapped_size,
DECREASE);
+ pgstat_report_allocated_bytes(request_size,
INCREASE);
pgstat_report_allocated_bytes is called twice for this case. Can the two
calls be combined into one (with request_size - *mapped_size, INCREASE) ?
Cheers
Re: Add tracking of backend memory allocated to pg_stat_activity
On Sun, Nov 27, 2022 at 12:32:19AM -0500, Reid Thompson wrote: > attempt to remedy cfbot windows build issues You can trigger those tests under your own/private repo by pushing a branch to github. See src/tools/ci/README I suppose the cfbot page ought to point that out. -- Justin
Re: Add tracking of backend memory allocated to pg_stat_activity
On Sat, 2022-11-26 at 22:10 -0500, Reid Thompson wrote:
> Code rebased to current master.
> Updated to incorporate additional recommendations from the the list
> - add units to variables in view
> - remove 'backend_' prefix from variables/functions
> - update documentation
> - add functional test for allocated_bytes
> - refactor allocation reporting to reduce number of functions and
> branches/reduce performance hit
> - zero allocated bytes after fork to avoid double counting
> postmaster allocations
>
>
>
>
attempt to remedy cfbot windows build issues
--
Reid Thompson
Senior Software Engineer
Crunchy Data, Inc.
reid.thomp...@crunchydata.com
www.crunchydata.com
--
Reid Thompson
Senior Software Engineer
Crunchy Data, Inc.
reid.thomp...@crunchydata.com
www.crunchydata.com
From fb43761181925a87cb0674b6744b009fea614796 Mon Sep 17 00:00:00 2001
From: Reid Thompson
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| 15
src/backend/catalog/system_views.sql| 1 +
src/backend/postmaster/autovacuum.c | 6 ++
src/backend/postmaster/postmaster.c | 13
src/backend/postmaster/syslogger.c | 3 +
src/backend/storage/ipc/dsm_impl.c | 81 +
src/backend/utils/activity/backend_status.c | 45
src/backend/utils/adt/pgstatfuncs.c | 4 +-
src/backend/utils/mmgr/aset.c | 17 +
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 | 58 ++-
src/test/regress/expected/rules.out | 9 ++-
src/test/regress/expected/stats.out | 11 +++
src/test/regress/sql/stats.sql | 3 +
16 files changed, 299 insertions(+), 9 deletions(-)
diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index 5579b8b9e0..ffe7d2566c 100644
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -947,6 +947,21 @@ postgres 27093 0.0 0.0 30096 2752 ?Ss 11:34 0:00 postgres: ser
+
+
+ allocated_bytes bigint
+
+
+ Memory currently allocated to this backend in bytes. This is the balance
+ of bytes allocated and freed by 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. Use pg_size_pretty
+ described in to make this value
+ more easily readable.
+
+
+
query text
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index 2d8104b090..9ea8f78c95 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -864,6 +864,7 @@ CREATE VIEW pg_stat_activity AS
S.state,
S.backend_xid,
s.backend_xmin,
+S.allocated_bytes,
S.query_id,
S.query,
S.backend_type
diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
index 601834d4b4..f54606104d 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -407,6 +407,9 @@ StartAutoVacLauncher(void)
#ifndef EXEC_BACKEND
case 0:
+ /* Zero allocated bytes to avoid double counting parent allocation */
+ pgstat_zero_my_allocated_bytes();
+
/* in postmaster child ... */
InitPostmasterChild();
@@ -1485,6 +1488,9 @@ StartAutoVacWorker(void)
#ifndef EXEC_BACKEND
case 0:
+ /* Zero allocated bytes to avoid double counting parent allocation */
+ pgstat_zero_my_allocated_bytes();
+
/* in postmaster child ... */
InitPostmasterChild();
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index a8a246921f..89a6caec78 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -4102,6 +4102,9 @@ BackendStartup(Port *port)
{
free(bn);
+ /* Zero allocated
Re: Add tracking of backend memory allocated to pg_stat_activity
On Wed, 2022-11-09 at 08:54 -0500, Reid Thompson wrote: > Hi Andres, > Thanks for looking at this and for the feedback. Responses inline > below. > >> On Fri, 2022-11-04 at 19:41 -0700, Andres Freund wrote: > > Hi, > > > On 2022-11-04 08:56:13 -0400, Reid Thompson wrote: > > > > I'm not convinced that counting DSM values this way is quite right. > > There are a few uses of DSMs that track shared resources, with the biggest > > likely being the stats for relations etc. I suspect that tracking that via > > backend memory usage will be quite confusing, because fairly random > > backends that > > had to grow the shared state end up being blamed with the memory usage in > > perpituity - and then suddenly that memory usage will vanish when that > > backend exits, > > despite the memory continuing to exist. > > Ok, I'll make an attempt to identify these allocations and manage > them elsewhere. still TBD > > > > > > > @@ -734,6 +747,7 @@ AllocSetAlloc(MemoryContext context, Size > > > size) > > > return NULL; > > > > > > context->mem_allocated += blksize; > > > + pgstat_report_backend_allocated_bytes_increase(bl > > > ksize); > > > > I suspect this will be noticable cost-wise. Even though these paths aren't > > the > > hottest memory allocation paths, by nature of going down into malloc, adding > > an external function call that then does a bunch of branching etc. seems > > likely to add up to some. See below for how I think we can deal with > > that... > > > > This is quite a few branches, including write/read barriers. > > > > It doesn't really make sense to use the > > PGSTAT_BEGIN_WRITE_ACTIVITY() pattern > > here - you're just updating a single value, there's nothing to be gained by > > it. The point of PGSTAT_BEGIN_*ACTIVITY() stuff is to allow readers to get a > > consistent view of multiple values - but there aren't multiple values > > here! > > I'll remove the barriers - initially I copied how prior functions were barriers removed > > > > To avoid the overhead of checking (!beentry || !pgstat_track_activities) and > > the external function call, I think you'd be best off copying the trickery I > > introduced for pgstat_report_wait_start(), in 225a22b19ed. > > > > I.e. make pgstat_report_backend_allocated_bytes_increase() a static inline > > function that unconditionally updates something like > > *my_backend_allocated_memory. To deal with the case of (!beentry || > > !pgstat_track_activities), that variable initially points to some backend > > local state and is set to the shared state in pgstat_bestart(). > > > > This additionally has the nice benefit that you can track memory usage from > > before pgstat_bestart(), it'll be in the local variable. > > OK, I think I can mimic the code you reference. done patch attached to https://www.postgresql.org/message-id/06b4922193b80776a31e08a3809f2414b0d4bf90.camel%40crunchydata.com -- Reid Thompson Senior Software Engineer Crunchy Data, Inc. reid.thomp...@crunchydata.com www.crunchydata.com
Re: Add tracking of backend memory allocated to pg_stat_activity
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.thomp...@crunchydata.com
www.crunchydata.com
Re: Add tracking of backend memory allocated to pg_stat_activity
Code rebased to current master.
Updated to incorporate additional recommendations from the the list
- add units to variables in view
- remove 'backend_' prefix from variables/functions
- update documentation
- add functional test for allocated_bytes
- refactor allocation reporting to reduce number of functions and
branches/reduce performance hit
- zero allocated bytes after fork to avoid double counting postmaster
allocations
--
Reid Thompson
Senior Software Engineer
Crunchy Data, Inc.
reid.thomp...@crunchydata.com
www.crunchydata.com
From 3d772d8620faba4bd4e091d6618c63557fbf6749 Mon Sep 17 00:00:00 2001
From: Reid Thompson
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| 15
src/backend/catalog/system_views.sql| 1 +
src/backend/postmaster/autovacuum.c | 6 ++
src/backend/postmaster/postmaster.c | 13
src/backend/postmaster/syslogger.c | 3 +
src/backend/storage/ipc/dsm_impl.c | 81 +
src/backend/utils/activity/backend_status.c | 45
src/backend/utils/adt/pgstatfuncs.c | 4 +-
src/backend/utils/mmgr/aset.c | 17 +
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 | 59 ++-
src/test/regress/expected/rules.out | 9 ++-
src/test/regress/expected/stats.out | 11 +++
src/test/regress/sql/stats.sql | 3 +
16 files changed, 300 insertions(+), 9 deletions(-)
diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index 5579b8b9e0..ffe7d2566c 100644
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -947,6 +947,21 @@ postgres 27093 0.0 0.0 30096 2752 ?Ss 11:34 0:00 postgres: ser
+
+
+ allocated_bytes bigint
+
+
+ Memory currently allocated to this backend in bytes. This is the balance
+ of bytes allocated and freed by 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. Use pg_size_pretty
+ described in to make this value
+ more easily readable.
+
+
+
query text
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index 2d8104b090..9ea8f78c95 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -864,6 +864,7 @@ CREATE VIEW pg_stat_activity AS
S.state,
S.backend_xid,
s.backend_xmin,
+S.allocated_bytes,
S.query_id,
S.query,
S.backend_type
diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
index 601834d4b4..f54606104d 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -407,6 +407,9 @@ StartAutoVacLauncher(void)
#ifndef EXEC_BACKEND
case 0:
+ /* Zero allocated bytes to avoid double counting parent allocation */
+ pgstat_zero_my_allocated_bytes();
+
/* in postmaster child ... */
InitPostmasterChild();
@@ -1485,6 +1488,9 @@ StartAutoVacWorker(void)
#ifndef EXEC_BACKEND
case 0:
+ /* Zero allocated bytes to avoid double counting parent allocation */
+ pgstat_zero_my_allocated_bytes();
+
/* in postmaster child ... */
InitPostmasterChild();
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index a8a246921f..89a6caec78 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -4102,6 +4102,9 @@ BackendStartup(Port *port)
{
free(bn);
+ /* Zero allocated bytes to avoid double counting parent allocation */
+ pgstat_zero_my_allocated_bytes();
+
/* Detangle from postmaster */
InitPostmasterChild();
@@ -5307,6 +5310,11 @@ StartChildProcess(AuxProcType type)
Re: Add tracking of backend memory allocated to pg_stat_activity
Hi,
2022-11-09 08:54:54 -0500, Reid Thompson wrote:
> Thanks for looking at this and for the feedback. Responses inline below.
> > > +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.
> > > + */
> >
> > I don't really follow - postmaster won't ever have a backend status
> > array, so how would they be tracked here?
>
> On startup, a check is made for leftover dsm control segments in the
> DataDir. It appears possible that in certain situations on startup we
> may find and destroy stale segments and thus decrement the allocation
> variable.
>
> I based this off of:
> /ipc/dsm.c
>
> dsm_postmaster_startup:
> 150 dsm_postmaster_startup(PGShmemHeader *shim)
> {
> ...
> 281 }
I don't think we should account for memory allocations done in postmaster in
this patch. They'll otherwise be counted again in each of the forked
backends. As this cleanup happens during postmaster startup, we'll have to
make sure accounting is reset during backend startup.
Greetings,
Andres Freund
Re: Add tracking of backend memory allocated to pg_stat_activity
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
> > 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
> >
Re: Add tracking of backend memory allocated to pg_stat_activity
Hi Andres,
Thanks for looking at this and for the feedback. Responses inline below.
On Fri, 2022-11-04 at 19:41 -0700, Andres Freund wrote:
> Hi,
>
> On 2022-11-04 08:56:13 -0400, Reid Thompson wrote:
>
> I'm not convinced that counting DSM values this way is quite right.
> There are a few uses of DSMs that track shared resources, with the biggest
> likely being the stats for relations etc. I suspect that tracking that via
> backend memory usage will be quite confusing, because fairly random backends
> that
> had to grow the shared state end up being blamed with the memory usage in
> perpituity - and then suddenly that memory usage will vanish when that
> backend exits,
> despite the memory continuing to exist.
Ok, I'll make an attempt to identify these allocations and manage them
elsewhere.
>
>
> > @@ -734,6 +747,7 @@ AllocSetAlloc(MemoryContext context, Size size)
> > return NULL;
> >
> > context->mem_allocated += blksize;
> > + pgstat_report_backend_allocated_bytes_increase(blksize);
> >
> > block->aset = set;
> > block->freeptr = block->endptr = ((char *) block) + blksize;
> > @@ -944,6 +958,7 @@ AllocSetAlloc(MemoryContext context, Size size)
> > return NULL;
> >
> > context->mem_allocated += blksize;
> > + pgstat_report_backend_allocated_bytes_increase(blksize);
> >
> > block->aset = set;
> > block->freeptr = ((char *) block) + ALLOC_BLOCKHDRSZ;
> > @@ -1043,6 +1058,7 @@ AllocSetFree(void *pointer)
> > block->next->prev = block->prev;
> >
> > set->header.mem_allocated -= block->endptr - ((char *)
> > block);
> > +
> > pgstat_report_backend_allocated_bytes_decrease(block->endptr - ((char *)
> > block));
> >
> > #ifdef CLOBBER_FREED_MEMORY
> > wipe_mem(block, block->freeptr - ((char *) block));
>
> I suspect this will be noticable cost-wise. Even though these paths aren't the
> hottest memory allocation paths, by nature of going down into malloc, adding
> an external function call that then does a bunch of branching etc. seems
> likely to add up to some. See below for how I think we can deal with
> that...
>
>
> > +
> > +/*
> > + * pgstat_report_backend_allocated_bytes_increase() -
> > + *
> > + * Called to report increase in memory allocated for this backend
> > + *
> > + */
> > +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);
> > +}
>
> This is quite a few branches, including write/read barriers.
>
> It doesn't really make sense to use the PGSTAT_BEGIN_WRITE_ACTIVITY() pattern
> here - you're just updating a single value, there's nothing to be gained by
> it. The point of PGSTAT_BEGIN_*ACTIVITY() stuff is to allow readers to get a
> consistent view of multiple values - but there aren't multiple values
> here!
I'll remove the barriers - initially I copied how prior functions were
coded as my template ala
pgstat_report_query_id, pgstat_report_xact_timestamp.
>
> To avoid the overhead of checking (!beentry || !pgstat_track_activities) and
> the external function call, I think you'd be best off copying the trickery I
> introduced for pgstat_report_wait_start(), in 225a22b19ed.
>
> I.e. make pgstat_report_backend_allocated_bytes_increase() a static inline
> function that unconditionally updates something like
> *my_backend_allocated_memory. To deal with the case of (!beentry ||
> !pgstat_track_activities), that variable initially points to some backend
> local state and is set to the shared state in pgstat_bestart().
>
> This additionally has the nice benefit that you can track memory usage from
> before pgstat_bestart(), it'll be in the local variable.
OK, I think I can mimic the code you reference.
>
> > +void
> > +pgstat_report_backend_allocated_bytes_decrease(uint64
> > deallocation)
> > +{
> > + volatile PgBackendStatus *beentry = MyBEEntry;
> > +
> > + /*
> > + * Cases may occur where shared memory from a
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
> 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
>
>
>
> +
> +
> + backend_allocated_bytes bigint
> +
> +
> + 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.
> +
> +
> +
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
Re: Add tracking of backend memory allocated to pg_stat_activity
Hi,
On 2022-11-04 08:56:13 -0400, Reid Thompson wrote:
> From a8de5d29c0c6f10962181926a49ad4fec1e52bd1 Mon Sep 17 00:00:00 2001
> From: Reid Thompson
> 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.
I'm not convinced that counting DSM values this way is quite right. There are
a few uses of DSMs that track shared resources, with the biggest likely being
the stats for relations etc. I suspect that tracking that via backend memory
usage will be quite confusing, because fairly random backends that had to grow
the shared state end up being blamed with the memory usage in perpituity - and
then suddenly that memory usage will vanish when that backend exits, despite
the memory continuing to exist.
> @@ -734,6 +747,7 @@ AllocSetAlloc(MemoryContext context, Size size)
> return NULL;
>
> context->mem_allocated += blksize;
> + pgstat_report_backend_allocated_bytes_increase(blksize);
>
> block->aset = set;
> block->freeptr = block->endptr = ((char *) block) + blksize;
> @@ -944,6 +958,7 @@ AllocSetAlloc(MemoryContext context, Size size)
> return NULL;
>
> context->mem_allocated += blksize;
> + pgstat_report_backend_allocated_bytes_increase(blksize);
>
> block->aset = set;
> block->freeptr = ((char *) block) + ALLOC_BLOCKHDRSZ;
> @@ -1043,6 +1058,7 @@ AllocSetFree(void *pointer)
> block->next->prev = block->prev;
>
> set->header.mem_allocated -= block->endptr - ((char *) block);
> + pgstat_report_backend_allocated_bytes_decrease(block->endptr -
> ((char *) block));
>
> #ifdef CLOBBER_FREED_MEMORY
> wipe_mem(block, block->freeptr - ((char *) block));
I suspect this will be noticable cost-wise. Even though these paths aren't the
hottest memory allocation paths, by nature of going down into malloc, adding
an external function call that then does a bunch of branching etc. seems
likely to add up to some. See below for how I think we can deal with that...
> +
> +/*
> + * pgstat_report_backend_allocated_bytes_increase() -
> + *
> + * Called to report increase in memory allocated for this backend
> + *
> + */
> +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);
> +}
This is quite a few branches, including write/read barriers.
It doesn't really make sense to use the PGSTAT_BEGIN_WRITE_ACTIVITY() pattern
here - you're just updating a single value, there's nothing to be gained by
it. The point of PGSTAT_BEGIN_*ACTIVITY() stuff is to allow readers to get a
consistent view of multiple values - but there aren't multiple values here!
To avoid the overhead of checking (!beentry || !pgstat_track_activities) and
the external function call, I think you'd be best off copying the trickery I
introduced for pgstat_report_wait_start(), in 225a22b19ed.
I.e. make pgstat_report_backend_allocated_bytes_increase() a static inline
function that unconditionally updates something like
*my_backend_allocated_memory. To deal with the case of (!beentry ||
!pgstat_track_activities), that variable initially points to some backend
local state and is set to the shared state in pgstat_bestart().
This additionally has the nice benefit that you can track memory usage from
before pgstat_bestart(), it'll be in the local variable.
> +void
>
Re: Add tracking of backend memory allocated to pg_stat_activity
On Fri, 2022-11-04 at 11:06 +0100, Drouvot, Bertrand wrote:
> Hi,
>
> It looks like the patch does not apply anymore since b1099eca8f.
>
> Regards,
>
Thanks,
rebased to current master attached.
--
Reid Thompson
Senior Software Engineer
Crunchy Data, Inc.
reid.thomp...@crunchydata.com
www.crunchydata.com
From a8de5d29c0c6f10962181926a49ad4fec1e52bd1 Mon Sep 17 00:00:00 2001
From: Reid Thompson
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
+
+
+ backend_allocated_bytes bigint
+
+
+ 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.
+
+
+
query text
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index 2d8104b090..84d462aa97 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -864,6 +864,7 @@ CREATE VIEW pg_stat_activity AS
S.state,
S.backend_xid,
s.backend_xmin,
+S.backend_allocated_bytes,
S.query_id,
S.query,
S.backend_type
diff --git a/src/backend/storage/ipc/dsm_impl.c b/src/backend/storage/ipc/dsm_impl.c
index 6ddd46a4e7..eae03159c3 100644
--- a/src/backend/storage/ipc/dsm_impl.c
+++ b/src/backend/storage/ipc/dsm_impl.c
@@ -66,6 +66,7 @@
#include "postmaster/postmaster.h"
#include "storage/dsm_impl.h"
#include "storage/fd.h"
+#include "utils/backend_status.h"
#include "utils/guc.h"
#include "utils/memutils.h"
@@ -232,6 +233,14 @@ dsm_impl_posix(dsm_op op, dsm_handle handle, Size request_size,
name)));
return false;
}
+
+ /*
+ * Detach and destroy pass through here, only decrease the memory
+ * shown allocated in pg_stat_activity when the creator destroys the
+ * allocation.
+ */
+ if (op == DSM_OP_DESTROY)
+ pgstat_report_backend_allocated_bytes_decrease(*mapped_size);
*mapped_address = NULL;
*mapped_size = 0;
if (op == DSM_OP_DESTROY && shm_unlink(name) != 0)
@@ -332,6 +341,36 @@ dsm_impl_posix(dsm_op op, dsm_handle handle, Size request_size,
name)));
return false;
}
+
+ /*
+ * Attach and create pass through here, only update backend memory
+ * allocated in pg_stat_activity for the creator process.
+ */
+ if (op == DSM_OP_CREATE)
+ {
+ /*
+ * Posix creation calls dsm_impl_posix_resize implying that resizing
+ * occurs or may be added in the future. As implemented
+ * dsm_impl_posix_resize utilizes fallocate or truncate, passing the
+ * whole new size as input, growing the allocation as needed (only
+ * truncate supports shrinking). We update by replacing the old
+ * allocation with the new.
+ */
+#if defined(HAVE_POSIX_FALLOCATE) && defined(__linux__)
+ /*
+ * posix_fallocate does not shrink allocations, adjust only on
+ * allocation increase.
+ */
+ if (request_size > *mapped_size)
+ {
+ pgstat_report_backend_allocated_bytes_decrease(*mapped_size);
+ pgstat_report_backend_allocated_bytes_increase(request_size);
+ }
+#else
+
Re: Add tracking of backend memory allocated to pg_stat_activity
Hi, On 10/25/22 8:59 PM, Reid Thompson wrote: On Tue, 2022-10-25 at 14:51 -0400, Reid Thompson wrote: patch rebased to current master actually attach the patch It looks like the patch does not apply anymore since b1099eca8f. Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Re: Add tracking of backend memory allocated to pg_stat_activity
On Tue, 2022-10-25 at 14:51 -0400, Reid Thompson wrote:
> patch rebased to current master
>
actually attach the patch
--
Reid Thompson
Senior Software Engineer
Crunchy Data, Inc.
reid.thomp...@crunchydata.com
www.crunchydata.com
From ab654a48ec7bfbc3bc377c5757a04f1756e72e79 Mon Sep 17 00:00:00 2001
From: Reid Thompson
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 | 80 +++
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, 269 insertions(+), 9 deletions(-)
diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index e5d622d514..4983bbc814 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
+
+
+ backend_mem_allocated bigint
+
+
+ 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.
+
+
+
query text
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index 2d8104b090..cbf804625c 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -865,6 +865,7 @@ CREATE VIEW pg_stat_activity AS
S.backend_xid,
s.backend_xmin,
S.query_id,
+S.backend_mem_allocated,
S.query,
S.backend_type
FROM pg_stat_get_activity(NULL) AS S
diff --git a/src/backend/storage/ipc/dsm_impl.c b/src/backend/storage/ipc/dsm_impl.c
index e1b90c5de4..3356bb65b5 100644
--- a/src/backend/storage/ipc/dsm_impl.c
+++ b/src/backend/storage/ipc/dsm_impl.c
@@ -66,6 +66,7 @@
#include "postmaster/postmaster.h"
#include "storage/dsm_impl.h"
#include "storage/fd.h"
+#include "utils/backend_status.h"
#include "utils/guc.h"
#include "utils/memutils.h"
@@ -232,6 +233,13 @@ dsm_impl_posix(dsm_op op, dsm_handle handle, Size request_size,
name)));
return false;
}
+ /*
+ * Detach and destroy pass through here, only decrease the memory
+ * shown allocated in pg_stat_activity when the creator destroys the
+ * allocation.
+ */
+ if (op == DSM_OP_DESTROY)
+ pgstat_report_backend_mem_allocated_decrease(*mapped_size);
*mapped_address = NULL;
*mapped_size = 0;
if (op == DSM_OP_DESTROY && shm_unlink(name) != 0)
@@ -332,6 +340,36 @@ dsm_impl_posix(dsm_op op, dsm_handle handle, Size request_size,
name)));
return false;
}
+
+ /*
+ * Attach and create pass through here, only update backend memory
+ * allocated in pg_stat_activity for the creator process.
+ */
+ if (op == DSM_OP_CREATE)
+ {
+ /*
+ * Posix creation calls dsm_impl_posix_resize implying that resizing
+ * occurs or may be added in the future. As implemented
+ * dsm_impl_posix_resize utilizes fallocate or truncate, passing the
+ * whole new size as input, growing the allocation as needed (only
+ * truncate supports shrinking). We update by replacing the old
+ * allocation with the new.
+ */
+#if defined(HAVE_POSIX_FALLOCATE) && defined(__linux__)
+ /*
+ * posix_fallocate does not shrink allocations, adjust only on
+ * allocation increase.
+ */
+ if (request_size > *mapped_size)
+ {
+ pgstat_report_backend_mem_allocated_decrease(*mapped_size);
+ pgstat_report_backend_mem_allocated_increase(request_size);
+ }
+#else
+ pgstat_report_backend_mem_allocated_decrease(*mapped_size);
+
Re: Add tracking of backend memory allocated to pg_stat_activity
patch rebased to current master -- Reid Thompson Senior Software Engineer Crunchy Data, Inc. reid.thomp...@crunchydata.com www.crunchydata.com
Re: Add tracking of backend memory allocated to pg_stat_activity
Greetings, * Drouvot, Bertrand (bdrou...@amazon.com) wrote: > On 9/9/22 7:08 PM, Justin Pryzby wrote: > >On Fri, Sep 09, 2022 at 12:34:15PM -0400, Stephen Frost wrote: > >>>While we are at it, what do you think about also recording the max memory > >>>allocated by a backend? (could be useful and would avoid sampling for which > >>>there is no guarantee to sample the max anyway). > >>What would you do with that information..? By itself, it doesn't strike > >>me as useful. Perhaps it'd be interesting to grab the max required for > >>a particular query in pg_stat_statements or such but again, that's a > >>very different thing. > > >Storing the maxrss per backend somewhere would be useful (and avoid the > >issue of "sampling" with top), after I agree that it ought to be exposed > >to a view. For example, it might help to determine whether (and which!) > >backends are using large multiple of work_mem, and then whether that can > >be increased. If/when we had a "memory budget allocator", this would > >help to determine how to set its GUCs, maybe to see "which backends are > >using the work_mem that are precluding this other backend from using > >efficient query plan". > > +1. I still have a hard time seeing the value in tracking which backends are using the most memory over the course of a backend's entire lifetime, which would involve lots of different queries, some of which might use many multiples of work_mem and others not. Much more interesting would be to track this as part of pg_stat_statements and associated with queries. Either way, this looks like an independent feature which someone who has interest in could work on but generally doesn't impact what the feature of this thread is about; a feature which has already shown merit in finding a recently introduced memory leak and is the basis of another feature being contemplated to help avoid OOM-killer introduced crashes. Thanks, Stephen signature.asc Description: PGP signature
Re: Add tracking of backend memory allocated to pg_stat_activity
On Fri, Sep 09, 2022 at 12:34:15PM -0400, Stephen Frost wrote: > > While we are at it, what do you think about also recording the max memory > > allocated by a backend? (could be useful and would avoid sampling for which > > there is no guarantee to sample the max anyway). FYI, that's already kind-of available from getrusage: $ psql ts -c "SET log_executor_stats=on; SET client_min_messages=debug; SELECT a, COUNT(1) FROM generate_series(1,99) a GROUP BY 1;" |wc LOG: EXECUTOR STATISTICS ... ! 194568 kB max resident size Note that max rss counts things allocated outside postgres (like linked libraries). > What would you do with that information..? By itself, it doesn't strike > me as useful. Perhaps it'd be interesting to grab the max required for > a particular query in pg_stat_statements or such but again, that's a > very different thing. log_executor_stats is at level "debug", so it's not great to enable it for a single session, and painful to think about enabling it globally. This would be a lot friendlier. Storing the maxrss per backend somewhere would be useful (and avoid the issue of "sampling" with top), after I agree that it ought to be exposed to a view. For example, it might help to determine whether (and which!) backends are using large multiple of work_mem, and then whether that can be increased. If/when we had a "memory budget allocator", this would help to determine how to set its GUCs, maybe to see "which backends are using the work_mem that are precluding this other backend from using efficient query plan". I wonder if it's better to combine these two threads into one. The 0001 patch of course can be considered independently from the 0002 patch, as usual. Right now, there's different parties on both threads ... -- Justin
Re: Add tracking of backend memory allocated to pg_stat_activity
Greetings, * Kyotaro Horiguchi (horikyota@gmail.com) wrote: > At Tue, 06 Sep 2022 17:10:49 -0400, Reid Thompson > wrote in > > I'm open to guidance on testing for performance degradation. I did > > note some basic pgbench comparison numbers in the thread regarding > > limiting backend memory allocations. > > Yeah.. That sounds good.. > > (I have a patch that is stuck at benchmarking on slight possible > degradation caused by a branch (or indirect call) on a hot path > similary to this one. The test showed fluctuation that is not clearly > distinguishable between noise and degradation by running the target > functions in a busy loop..) Just to be clear- this path is (hopefully) not *super* hot as we're only tracking actual allocations (that is- malloc() calls), this isn't changing anything for palloc() calls that aren't also needing to do a malloc(), and we already try to reduce the amount of malloc() calls we're doing by allocating more and more each time we run out in a given context. While I'm generally supportive of doing some benchmarking around this, I don't think the bar is as high as it would be if we were actually changing the cost of routine palloc() or such calls. Thanks, Stephen signature.asc Description: PGP signature
Re: Add tracking of backend memory allocated to pg_stat_activity
Greetings,
* Drouvot, Bertrand (bdrou...@amazon.com) wrote:
> On 9/1/22 3:28 AM, Kyotaro Horiguchi wrote:
> >At Wed, 31 Aug 2022 12:05:55 -0500, Justin Pryzby
> >wrote in
> >>On Wed, Aug 31, 2022 at 12:03:06PM -0400, Reid Thompson wrote:
> >>>Attached is a patch to
> >>>Add tracking of backend memory allocated
>
> Thanks for the patch.
>
> + 1 on the idea.
Glad folks are in support of the general idea.
> >>>+ proargmodes =>
> >>>'{i,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o}',
> >>In the past, there was concern about making pg_stat_activity wider by
> >>adding information that's less-essential than what's been there for
> >>years. This is only an int64, so it's not "wide", but I wonder if
> >>there's another way to expose this information? Like adding backends to
> >The view looks already too wide to me. I don't want the numbers for
> >metrics are added to the view.
>
> +1 for a dedicated view.
A dedicated view with a single column in it hardly seems sensible. I'd
also argue that this particular bit of information is extremely useful
and therefore worthy of being put directly into pg_stat_activity. I
could see a dedicated view possibly *also* being added later if/when we
provide a more detailed break-down of how the memory is being used but
that's a whole other thing and I'm not even 100% sure we'll ever
actually get there, as you can already poke a backend and have it dump
out the memory context-level information on an as-needed basis.
> While we are at it, what do you think about also recording the max memory
> allocated by a backend? (could be useful and would avoid sampling for which
> there is no guarantee to sample the max anyway).
What would you do with that information..? By itself, it doesn't strike
me as useful. Perhaps it'd be interesting to grab the max required for
a particular query in pg_stat_statements or such but again, that's a
very different thing.
Thanks,
Stephen
signature.asc
Description: PGP signature
Re: Add tracking of backend memory allocated to pg_stat_activity
At Wed, 07 Sep 2022 17:08:41 +0900 (JST), Kyotaro Horiguchi wrote in > At Tue, 06 Sep 2022 17:10:49 -0400, Reid Thompson > wrote in > > On Thu, 2022-09-01 at 13:43 +0900, Kyotaro Horiguchi wrote: > > > > > > > @@ -916,6 +930,7 @@ AllocSetAlloc(MemoryContext context, Size size) > > > > return NULL; > > > > > > > > context->mem_allocated += blksize; > > > > + pgstat_report_backend_mem_allocated_increase(blksi > > > > ze); > > > > > > I'm not sure this is acceptable. The function adds a branch even when > > > the feature is turned off, which I think may cause a certain extent > > > of > > > performance degradation. A past threads [1], [2] and [3] might be > > > informative. > > > > Stated above is '...even when the feature is turned off...', I want to > > note that this feature/patch (for tracking memory allocated) doesn't > > have an 'on/off'. Tracking would always occur. > > In the patch, I see that > pgstat_report_backend_mem_allocated_increase() runs the following > code, which seems like to me to be a branch.. Ah.. sorry. > pgstat_report_backend_mem_allocated_increase() runs the following - code, which seems like to me to be a branch.. + code, which seems like to me to be a branch that can turn of the feature.. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Re: Add tracking of backend memory allocated to pg_stat_activity
At Tue, 06 Sep 2022 17:10:49 -0400, Reid Thompson
wrote in
> On Thu, 2022-09-01 at 13:43 +0900, Kyotaro Horiguchi wrote:
> >
> > > @@ -916,6 +930,7 @@ AllocSetAlloc(MemoryContext context, Size size)
> > > return NULL;
> > >
> > > context->mem_allocated += blksize;
> > > + pgstat_report_backend_mem_allocated_increase(blksi
> > > ze);
> >
> > I'm not sure this is acceptable. The function adds a branch even when
> > the feature is turned off, which I think may cause a certain extent
> > of
> > performance degradation. A past threads [1], [2] and [3] might be
> > informative.
>
> Stated above is '...even when the feature is turned off...', I want to
> note that this feature/patch (for tracking memory allocated) doesn't
> have an 'on/off'. Tracking would always occur.
In the patch, I see that
pgstat_report_backend_mem_allocated_increase() runs the following
code, which seems like to me to be a branch..
+ if (!beentry || !pgstat_track_activities)
+ {
+ /*
+* Account for memory before pgstats is initialized. This will
be
+* migrated to pgstats on initialization.
+*/
+ backend_mem_allocated += allocation;
+
+ return;
+ }
> I'm open to guidance on testing for performance degradation. I did
> note some basic pgbench comparison numbers in the thread regarding
> limiting backend memory allocations.
Yeah.. That sounds good..
(I have a patch that is stuck at benchmarking on slight possible
degradation caused by a branch (or indirect call) on a hot path
similary to this one. The test showed fluctuation that is not clearly
distinguishable between noise and degradation by running the target
functions in a busy loop..)
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
Re: Add tracking of backend memory allocated to pg_stat_activity
On Thu, 2022-09-01 at 13:43 +0900, Kyotaro Horiguchi wrote: > > > @@ -916,6 +930,7 @@ AllocSetAlloc(MemoryContext context, Size size) > > return NULL; > > > > context->mem_allocated += blksize; > > + pgstat_report_backend_mem_allocated_increase(blksi > > ze); > > I'm not sure this is acceptable. The function adds a branch even when > the feature is turned off, which I think may cause a certain extent > of > performance degradation. A past threads [1], [2] and [3] might be > informative. Stated above is '...even when the feature is turned off...', I want to note that this feature/patch (for tracking memory allocated) doesn't have an 'on/off'. Tracking would always occur. I'm open to guidance on testing for performance degradation. I did note some basic pgbench comparison numbers in the thread regarding limiting backend memory allocations. > -- > Kyotaro Horiguchi > NTT Open Source Software Center
Re: Add tracking of backend memory allocated to pg_stat_activity
On Wed, 2022-08-31 at 12:05 -0500, Justin Pryzby wrote:
> > + proargmodes =>
> > '{i,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o}'
> > ,
>
> In the past, there was concern about making pg_stat_activity wider by
> adding information that's less-essential than what's been there for
> years. This is only an int64, so it's not "wide", but I wonder if
> there's another way to expose this information? Like adding backends
> to
> pg_get_backend_memory_contexts() , maybe with another view on top of
I will take a look at pg_get_backend_memory_contexts. I will also look
at the other suggestions in the thread.
> + * shown allocated in pgstat_activity when the
>
> pg_stat
Corrected,
> > replacing the * old
>
> wrapping caused extraneous stars
Corrected
> > here asit's
>
> as it's
Corrected
> errmsg() doesn't require the outside paranthesis since a couple years
> ago.
Corrected
> > > mem_allocated cast to
> add a comma before "cast" ?
Corrected
Patch with the corrections attached
--
Reid Thompson
Senior Software Engineer
Crunchy Data, Inc.
reid.thomp...@crunchydata.com
www.crunchydata.com
--
Reid Thompson
Senior Software Engineer
Crunchy Data, Inc.
reid.thomp...@crunchydata.com
www.crunchydata.com
--
Reid Thompson
Senior Software Engineer
Crunchy Data, Inc.
reid.thomp...@crunchydata.com
www.crunchydata.com
From 584a04f1b53948049e73165a4ffdd544c950ab0d Mon Sep 17 00:00:00 2001
From: Reid Thompson
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 | 80 +++
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, 269 insertions(+), 9 deletions(-)
diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index 1d9509a2f6..40ae638f25 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
+
+
+ backend_mem_allocated bigint
+
+
+ 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.
+
+
+
query text
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index 5a844b63a1..d23f0e9dbb 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -863,6 +863,7 @@ CREATE VIEW pg_stat_activity AS
S.backend_xid,
s.backend_xmin,
S.query_id,
+S.backend_mem_allocated,
S.query,
S.backend_type
FROM pg_stat_get_activity(NULL) AS S
diff --git a/src/backend/storage/ipc/dsm_impl.c b/src/backend/storage/ipc/dsm_impl.c
index e1b90c5de4..3356bb65b5 100644
--- a/src/backend/storage/ipc/dsm_impl.c
+++ b/src/backend/storage/ipc/dsm_impl.c
@@ -66,6 +66,7 @@
#include "postmaster/postmaster.h"
#include "storage/dsm_impl.h"
#include "storage/fd.h"
+#include "utils/backend_status.h"
#include "utils/guc.h"
#include "utils/memutils.h"
@@ -232,6 +233,13 @@ dsm_impl_posix(dsm_op op, dsm_handle handle, Size request_size,
name)));
return false;
}
+ /*
+ * Detach and destroy pass through here, only decrease the memory
+ * shown allocated in pg_stat_activity when the creator destroys the
+ * allocation.
+ */
+ if (op == DSM_OP_DESTROY)
+ pgstat_report_backend_mem_allocated_decrease(*mapped_size);
*mapped_address = NULL;
*mapped_size = 0;
if (op ==
Re: Add tracking of backend memory allocated to pg_stat_activity
At Wed, 31 Aug 2022 12:03:06 -0400, Reid Thompson wrote in > Attached is a patch to > Add tracking of backend memory allocated to pg_stat_activity > @@ -916,6 +930,7 @@ AllocSetAlloc(MemoryContext context, Size size) > return NULL; > > context->mem_allocated += blksize; > + pgstat_report_backend_mem_allocated_increase(blksize); I'm not sure this is acceptable. The function adds a branch even when the feature is turned off, which I think may cause a certain extent of performance degradation. A past threads [1], [2] and [3] might be informative. [1] https://www.postgresql.org/message-id/1434311039.4369.39.camel%40jeff-desktop [2] https://www.postgresql.org/message-id/72a656e0f71d0860161e0b3f67e4d771%40oss.nttdata.com [3] https://www.postgresql.org/message-id/0271f440ac77f2a4180e0e56ebd944d1%40oss.nttdata.com regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Re: Add tracking of backend memory allocated to pg_stat_activity
At Wed, 31 Aug 2022 12:05:55 -0500, Justin Pryzby wrote
in
> On Wed, Aug 31, 2022 at 12:03:06PM -0400, Reid Thompson wrote:
> > Hi Hackers,
> >
> > Attached is a patch to
> > Add tracking of backend memory allocated to pg_stat_activity
>
> > + proargmodes =>
> > '{i,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o}',
>
> In the past, there was concern about making pg_stat_activity wider by
> adding information that's less-essential than what's been there for
> years. This is only an int64, so it's not "wide", but I wonder if
> there's another way to expose this information? Like adding backends to
The view looks already too wide to me. I don't want the numbers for
metrics are added to the view.
> pg_get_backend_memory_contexts() , maybe with another view on top of
> that ?
+1
> +* shown allocated in pgstat_activity when the creator
> destroys the
>
>
> pg_stat
>
> > +* Posix creation calls dsm_impl_posix_resize implying that
> > resizing
> > +* occurs or may be added in the future. As implemented
> > +* dsm_impl_posix_resize utilizes fallocate or truncate,
> > passing the
> > +* whole new size as input, growing the allocation as needed *
> > (only
> > +* truncate supports shrinking). We update by replacing the *
> > old
>
> wrapping caused extraneous stars
>
> > +* Do not allow backend_mem_allocated to go below zero. ereport if we
> > +* would have. There's no need for a lock around the read here asit's
>
> as it's
>
> > + ereport(LOG, (errmsg("decrease reduces reported backend memory
> > allocated below zero; setting reported to 0")));
>
> errmsg() doesn't require the outside paranthesis since a couple years
> ago.
+1
> > + /*
> > +* Until header allocation is included in context->mem_allocated cast to
> > +* slab and decrement the headerSize
>
> add a comma before "cast" ?
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
Re: Add tracking of backend memory allocated to pg_stat_activity
On Wed, Aug 31, 2022 at 12:03:06PM -0400, Reid Thompson wrote:
> Hi Hackers,
>
> Attached is a patch to
> Add tracking of backend memory allocated to pg_stat_activity
> + proargmodes =>
> '{i,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o}',
In the past, there was concern about making pg_stat_activity wider by
adding information that's less-essential than what's been there for
years. This is only an int64, so it's not "wide", but I wonder if
there's another way to expose this information? Like adding backends to
pg_get_backend_memory_contexts() , maybe with another view on top of
that ?
+* shown allocated in pgstat_activity when the creator destroys
the
pg_stat
> + * Posix creation calls dsm_impl_posix_resize implying that
> resizing
> + * occurs or may be added in the future. As implemented
> + * dsm_impl_posix_resize utilizes fallocate or truncate,
> passing the
> + * whole new size as input, growing the allocation as needed *
> (only
> + * truncate supports shrinking). We update by replacing the *
> old
wrapping caused extraneous stars
> + * Do not allow backend_mem_allocated to go below zero. ereport if we
> + * would have. There's no need for a lock around the read here asit's
as it's
> + ereport(LOG, (errmsg("decrease reduces reported backend memory
> allocated below zero; setting reported to 0")));
errmsg() doesn't require the outside paranthesis since a couple years
ago.
> + /*
> + * Until header allocation is included in context->mem_allocated cast to
> + * slab and decrement the headerSize
add a comma before "cast" ?
--
Justin
