Re: Add the ability to limit the amount of memory that can be allocated to backends.

Hi,

On 2023-01-13 09:15:10 -0500, Reid Thompson wrote:
> On Mon, 2023-01-09 at 18:31 -0800, Andres Freund wrote:
> > > 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.
> >
> > As mentioned before, I don't think accounting DSM this way makes sense.
>
> Understood, previously you noted 'There are a few uses of DSMs that track
> shared resources, with the biggest likely being the stats for relations
> etc'. I'd like to come up with a solution to address this; identifying the
> long term allocations to shared state and accounting for them such that they
> don't get 'lost' when the allocating backend exits. Any guidance or
> direction would be appreciated.

Tracking it as backend memory usage doesn't seem helpful to me, particularly
because some of it is for server wide data tracking (pgstats, some
caches). But that doesn't mean you couldn't track and report it
separately.

> > > +/* ----------
> > > + * pgstat_get_all_memory_allocated() -
> > > + *
> > > + * Return a uint64 representing the current shared memory allocated to all
> > > + * backends. This looks directly at the BackendStatusArray, and so will
> > > + * provide current information regardless of the age of our transaction's
> > > + * snapshot of the status array.
> > > + * In the future we will likely utilize additional values - perhaps limit
> > > + * backend allocation by user/role, etc.
> > > + * ----------
> >
> > I think it's completely unfeasible to execute something as expensive as
> > pgstat_get_all_backend_memory_allocated() on every allocation. Like,
> > seriously, no.
>
> Ok. Do we check every nth allocation/try to implement a scheme of checking
> more often as we we get closer to the declared max_total_bkend_mem?

I think it's just not acceptable to do O(connections) work as part of
something critical as memory allocation. Even if amortized imo.

What you could do is to have a single, imprecise, shared counter for the total
memory allocation, and have a backend-local "allowance". When the allowance is
used up, refill it from the shared counter (a single atomic op).

But honestly, I think we first need to have the accounting for a while before
it makes sense to go for the memory limiting patch. And I doubt a single GUC
will suffice to make this usable.

> > And we absolutely definitely shouldn't just add CHECK_FOR_INTERRUPT() calls
> > into the middle of allocator code.
>
> I'm open to guidance/suggestions/pointers to remedying these.

Well, just don't have the CHECK_FOR_INTERRUPT(). Nor the O(N) operation.

You also can't do the ereport(WARNING) there, that itself allocates memory,
and could lead to recursion in some edge cases.

Greetings,

Andres Freund