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