Date: Mon, 6 Nov 2023 13:02:50 -0500
From: Stephen Frost <sfrost@snowman.net>
To: Andres Freund <andres@anarazel.de>
Cc: John Morris <john.morris@crunchydata.com>,
	Andrei Lepikhov <a.lepikhov@postgrespro.ru>,
	"reid.thompson@crunchydata.com" <reid.thompson@crunchydata.com>,
	Arne Roland <A.Roland@index.de>,
	"pgsql-hackers@lists.postgresql.org" <pgsql-hackers@lists.postgresql.org>,
	vignesh C <vignesh21@gmail.com>,
	Justin Pryzby <pryzby@telsasoft.com>,
	Ibrar Ahmed <ibrar.ahmad@gmail.com>,
	"stephen.frost" <stephen.frost@crunchydata.com>
Subject: Re: Add the ability to limit the amount of memory that can be
 allocated to backends.
Message-ID: <ZUkqSqdGcu4OLoOd@tamriel.snowman.net>
References: <BYAPR13MB26776A35AB57940680D4CE0EA0A0A@BYAPR13MB2677.namprd13.prod.outlook.com>
 <20231104041900.27pgqsknb3yl45zy@awork3.anarazel.de>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="tSUpkMQm6gcCwt6h"
Content-Disposition: inline
In-Reply-To: <20231104041900.27pgqsknb3yl45zy@awork3.anarazel.de>
User-Agent: Mutt/2.1.4 (2021-12-11)
Archived-At: <https://www.postgresql.org/message-id/ZUkqSqdGcu4OLoOd%40tamriel.snowman.net>
Precedence: bulk


--tSUpkMQm6gcCwt6h
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

Greetings,

* Andres Freund (andres@anarazel.de) wrote:
> On 2023-10-31 17:11:26 +0000, John Morris wrote:
> > Postgres memory reservations come from multiple sources.
> >=20
> >   *   Malloc calls made by the Postgres memory allocators.
> >   *   Static shared memory created by the postmaster at server startup,
> >   *   Dynamic shared memory created by the backends.
> >   *   A fixed amount (1Mb) of =E2=80=9Cinitial=E2=80=9D memory reserved=
 whenever a process starts up.
> >=20
> > Each process also maintains an accurate count of its actual memory
> > allocations. The process-private variable =E2=80=9Cmy_memory=E2=80=9D h=
olds the total
> > allocations for that process. Since there can be no contention, each pr=
ocess
> > updates its own counters very efficiently.
>=20
> I think this will introduce measurable overhead in low concurrency cases =
and
> very substantial overhead / contention when there is a decent amount of
> concurrency.  This makes all memory allocations > 1MB contend on a single
> atomic.  Massive amount of energy have been spent writing multi-threaded
> allocators that have far less contention than this - the current state is=
 to
> never contend on shared resources on any reasonably common path. This giv=
es
> away one of the few major advantages our process model has away.

We could certainly adjust the size of each reservation to reduce the
frequency of having to hit the atomic.  Specific suggestions about how
to benchmark and see the regression that's being worried about here
would be great.  Certainly my hope has generally been that when we do a
larger allocation, it's because we're about to go do a bunch of work,
meaning that hopefully the time spent updating the atomic is minor
overall.

> The patch doesn't just introduce contention when limiting is enabled - it
> introduces it even when memory usage is just tracked. It makes absolutely=
 no
> sense to have a single contended atomic in that case - just have a per-ba=
ckend
> variable in shared memory that's updated. It's *WAY* cheaper to compute t=
he
> overall memory usage during querying than to keep a running total in shar=
ed
> memory.

Agreed that we should avoid the contention when limiting isn't being
used, certainly easy to do so, and had actually intended to but that
seems to have gotten lost along the way.  Will fix.

Other than that change inside update_global_reservation though, the code
for reporting per-backend memory usage and querying it does work as
you're outlining above inside the stats system.

That said- I just want to confirm that you would agree that querying the
amount of memory used by every backend, to add it all up to enforce an
overall limit, surely isn't something we're going to want to do during
an allocation and that having a global atomic for that is better, right?

> > Pgstat now includes global memory counters. These shared memory counters
> > represent the sum of all reservations made by all Postgres processes. F=
or
> > efficiency, these global counters are only updated when new reservations
> > exceed a threshold, currently 1 Mb for each process. Consequently, the
> > global reservation counters are approximate totals which may differ fro=
m the
> > actual allocation totals by up to 1 Mb per process.
>=20
> I see that you added them to the "cumulative" stats system - that doesn't
> immediately makes sense to me - what you're tracking here isn't an
> accumulating counter, it's something showing the current state, right?

Yes, this is current state, not an accumulation.

> > The max_total_memory limit is checked whenever the global counters are
> > updated. There is no special error handling if a memory allocation exce=
eds
> > the global limit. That allocation returns a NULL for malloc style
> > allocations or an ENOMEM for shared memory allocations. Postgres has
> > existing mechanisms for dealing with out of memory conditions.
>=20
> I still think it's extremely unwise to do tracking of memory and limiting=
 of
> memory in one patch.  You should work towards and acceptable patch that j=
ust
> tracks memory usage in an as simple and low overhead way as possible. The=
n we
> later can build on that.

Frankly, while tracking is interesting, the limiting is the feature
that's needed more urgently imv.  We could possibly split it up but
there's an awful lot of the same code that would need to be changed and
that seems less than ideal.  Still, we'll look into this.

> > For sanity checking, pgstat now includes the pg_backend_memory_allocati=
on
> > view showing memory allocations made by the backend process. This view
> > includes a scan of the top memory context, so it compares memory alloca=
tions
> > reported through pgstat with actual allocations. The two should match.
>=20
> Can't you just do this using the existing pg_backend_memory_contexts view?

Not and get a number that you can compare to the local backend number
due to the query itself happening and performing allocations and
creating new contexts.  We wanted to be able to show that we are
accounting correctly and exactly matching to what the memory context
system is tracking.

> > - oid | proname | proargtypes | proallargtypes | proargmodes=20
> > ------+---------+-------------+----------------+-------------
> > -(0 rows)
> > + oid  |             proname              | proargtypes |      proallar=
gtypes       |    proargmodes   =20
> > +------+----------------------------------+-------------+--------------=
-------------+-------------------
> > + 9890 | pg_stat_get_memory_reservation   |             | {23,23,20,20,=
20,20,20,20} | {i,o,o,o,o,o,o,o}
> > + 9891 | pg_get_backend_memory_allocation |             | {23,23,20,20,=
20,20,20}    | {i,o,o,o,o,o,o}
> > +(2 rows)
>=20
> This indicates that your pg_proc entries are broken, they need to fixed r=
ather
> than allowed here.

Agreed, will fix.

Thanks!

Stephen

--tSUpkMQm6gcCwt6h
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEwf6gbxKhD863zrx/7WyKOINHZFUFAmVJKkgACgkQ7WyKOINH
ZFVvVhAAgZtNHKn9bTFgIteeNQoQohEdagjvD3rGVxQ9mVWQyWNaCFstDPpQ4QCE
JiXeBjVMbBx3aa/t/dbwHs0o1xvc4af+awux0qN7gwY5WNNagCix+yoA5bZlTnGR
ZCYWO/HNHg+hZkci6nfu9QLPyLV3gxBw+0C95hxUfAm8fmfqyy4y+6w3DmUqeuVo
YlnesbduluwxIT34Ch5viD5+X80l+vvipEYYF84o2Uhwk8LE5SQHzdHR/c+hkfaO
zg6uET6VKH8WG9XV9em/PGIWZ1awyqxsqu6IDQ9CtBVQ0urr6DrHUxxoPbeNYAFH
v2nARi6TpqWIwDunKibh6/QCKWjkc0jnmaoxprfP59BOeTyRiAS9g8lUrOl0Dqjn
j4tlRj6hyHTWGUAf/0stV1vhDVi9ptfBc6iOcdo4DlmT1/f2SzdzNZh8mWZLZ0Oq
74pO8mUPMluCekE8dLFz0N01ftuxHv+tH+hlfTXtjhsymPy2VSVc6i+Mz6JIbaYV
4cscaFj3btTVP0KY+9WVSCm0quuUIse0kZkfdrmZ3vgYkIA/5j1W3NfXSNP9ahWm
EpWEKkixF/UzIFHpdr/d/GJdGJo30kQ9ixm76y0rCbqerImITgs4+573mbVzX7fb
uBAlQUJsJMBTv8nr2l7rb+fSLcr0AxVKYDGqRBZtw5heDrDJtnk=
=/q5i
-----END PGP SIGNATURE-----

--tSUpkMQm6gcCwt6h--