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