Date: Tue, 7 Nov 2023 15:55:48 -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: <ZUqkVEX9TBOkM8cq@tamriel.snowman.net>
References: <BYAPR13MB26776A35AB57940680D4CE0EA0A0A@BYAPR13MB2677.namprd13.prod.outlook.com>
 <20231104041900.27pgqsknb3yl45zy@awork3.anarazel.de>
 <ZUkqSqdGcu4OLoOd@tamriel.snowman.net>
 <20231107195506.4i2tk352cvfs44t6@awork3.anarazel.de>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="+UkiVdaXN5JI+TTH"
Content-Disposition: inline
In-Reply-To: <20231107195506.4i2tk352cvfs44t6@awork3.anarazel.de>
User-Agent: Mutt/2.1.4 (2021-12-11)
Archived-At: <https://www.postgresql.org/message-id/ZUqkVEX9TBOkM8cq%40tamriel.snowman.net>
Precedence: bulk


--+UkiVdaXN5JI+TTH
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

Greetings,

* Andres Freund (andres@anarazel.de) wrote:
> On 2023-11-06 13:02:50 -0500, Stephen Frost wrote:
> > > > The max_total_memory limit is checked whenever the global counters =
are
> > > > updated. There is no special error handling if a memory allocation =
exceeds
> > > > 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 limi=
ting of
> > > memory in one patch.  You should work towards and acceptable patch th=
at just
> > > tracks memory usage in an as simple and low overhead way as possible.=
 Then we
> > > later can build on that.
> >=20
> > Frankly, while tracking is interesting, the limiting is the feature
> > that's needed more urgently imv.
>=20
> I agree that we need limiting, but that the tracking needs to be very rob=
ust
> for that to be usable.

Is there an issue with the tracking in the patch that you saw?  That's
certainly an area that we've tried hard to get right and to match up to
numbers from the rest of the system, such as the memory context system.

> > 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.
>=20
> Shrug. IMO keeping them together just makes it very likely that neither g=
oes
> in.

I'm happy to hear your support for the limiting part of this- that's
encouraging.

> > > > For sanity checking, pgstat now includes the pg_backend_memory_allo=
cation
> > > > view showing memory allocations made by the backend process. This v=
iew
> > > > includes a scan of the top memory context, so it compares memory al=
locations
> > > > reported through pgstat with actual allocations. The two should mat=
ch.
> > >=20
> > > Can't you just do this using the existing pg_backend_memory_contexts =
view?
> >=20
> > 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.
>=20
> I think creating a separate view for this will be confusing for users, wi=
thout
> really much to show for. Excluding the current query would be useful for =
other
> cases as well, why don't we provide a way to do that with
> pg_backend_memory_contexts?

Both of these feel very much like power-user views, so I'm not terribly
concerned about users getting confused.  That said, we could possibly
drop this as a view and just have the functions which are then used in
the regression tests to catch things should the numbers start to
diverge.

Having a way to get the memory contexts which don't include the
currently running query might be interesting too but is rather
independent of what this patch is trying to do.  The only reason we
collected up the memory-context info is as a cross-check to the tracking
that we're doing and while the existing memory-context view is just fine
for a lot of other things, it doesn't work for that specific need.

Thanks,

Stephen

--+UkiVdaXN5JI+TTH
Content-Type: application/pgp-signature; name="signature.asc"

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

iQIzBAABCgAdFiEEwf6gbxKhD863zrx/7WyKOINHZFUFAmVKpFEACgkQ7WyKOINH
ZFU3pBAApMxMFxZRqxrQIXNHg+8BTUYSV7jbRasFKrIKiR1Z0nGcFnS1AMZwfhGP
g4Zmu1JgoUBz/fQIN7P/7a/NumMt2o8y+6tFAUzUD73JyalXg02bfZDFUT8u99JI
HJuBoIC23u6sxT1xKatPUWcrOY/8HyBpvHMwktBxbEumArpxbV6I9CqP2dQ67t5L
PoNuX7JsyQQjXFtOmgYXsCnc2ZmG4Y+sVFkBvzQt1SdddV7ZG0O4WrLBHTtwFr8M
lVCJqVh4SeU5EM2c2Im47cnZUqz1w9KXKNGcqNeqx2Cr3GTFBIH9058xgZGin5R0
WD2tMSYJYeCuelTCYBL8YFAD8OF0827lLm+BjCgtFOj3dpUKpMrzfhtOO69d5GCm
s1eAfCOHQkCyx4mOej9BOTmOdVF4LN6eDoI0dTwHH8F4mtqxRQ64j4Iq3RMNgvUL
le7AbLoYw/6GBSB1D2OS/pP7DTkztnpi2EDY40btZ3aKOICa0ykL6UgnQ91xB5og
iOcAsiZ8aC07VtBt1lAXqx3v2zQUj4y957OTLVJJWPqGoywTGoSkQQu7PVJCbVo6
4MPzD1dUDSYaZyBTpkYivWsT6DcU7MLfo1a+ornToxpsH+DQD5cLKyRokSaoiXqK
am1CMBWcOkgjWCngLNVm7XkVCJ2yAngi4XMwt2/qyOXdUq9/qyY=
=Cn05
-----END PGP SIGNATURE-----

--+UkiVdaXN5JI+TTH--