Date: Thu, 19 Oct 2023 18:49:21 -0400
From: Stephen Frost <sfrost@snowman.net>
To: Andres Freund <andres@anarazel.de>
Cc: Andrei Lepikhov <a.lepikhov@postgrespro.ru>,
	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>
Subject: Re: Add the ability to limit the amount of memory that can be
 allocated to backends.
Message-ID: <ZTGycSYuFrsixv6q@tamriel.snowman.net>
References: <BE1P281MB313901E262719C4CF69B224BFE9C9@BE1P281MB3139.DEUP281.PROD.OUTLOOK.COM>
 <94184680049d6bda0c85f1759af24d8127cfe895.camel@crunchydata.com>
 <7912c911af51d5cf28c611190bf3d463b9209343.camel@crunchydata.com>
 <4edafedc0f8acb12a2979088ac1317bd7dd42145.camel@crunchydata.com>
 <f62b4107-227d-49e7-a9be-104d1ceb03d9@postgrespro.ru>
 <268e0ac7-8a81-4d65-8b40-b62c4b3f1bf9@postgrespro.ru>
 <ZTArWsctGn5fEVPR@tamriel.snowman.net>
 <48548d40-634b-4943-a737-3c9d95eacf06@postgrespro.ru>
 <ZTGoUt+pH8uPucfm@tamriel.snowman.net>
 <20231019222251.4qg6udcbtrwglogw@awork3.anarazel.de>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="FUkEYrCLnLL9itaa"
Content-Disposition: inline
In-Reply-To: <20231019222251.4qg6udcbtrwglogw@awork3.anarazel.de>
User-Agent: Mutt/2.1.4 (2021-12-11)
Archived-At: <https://www.postgresql.org/message-id/ZTGycSYuFrsixv6q%40tamriel.snowman.net>
Precedence: bulk


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

Greetings,

* Andres Freund (andres@anarazel.de) wrote:
> On 2023-10-19 18:06:10 -0400, Stephen Frost wrote:
> > Ignoring such would defeat much of the point of this effort- which is to
> > get to a position where we can say with some confidence that we're not
> > going to go over some limit that the user has set and therefore not
> > allow ourselves to end up getting OOM killed.
>=20
> I think that is a good medium to long term goal. I do however think that =
we'd
> be better off merging the visibility of memory allocations soon-ish and
> implement the limiting later. There's a lot of hairy details to get right=
 for
> the latter, and even just having visibility will be a huge improvement.

I agree that having the visibility will be a great improvement and
perhaps could go in separately, but I don't know that I agree that the
limits are going to be that much of an issue.  In any case, there's been
work ongoing on this and that'll be posted soon.  I was just trying to
address the general comment raised in this sub-thread here.

> I think even patch 1 is doing too much at once. I doubt the DSM stuff is
> quite right.

Getting DSM right has certainly been tricky, along with other things,
but we've been working towards, and continue to work towards, getting
everything to line up nicely between memory context allocations of
various types and the amounts which are being seen as malloc'd/free'd.
There's been parts of this also reworked to allow us to see per-backend
reservations as well as total reserved and to get those numbers able to
be matched up inside of a given transaction using the statistics system.

> I'm unconvinced it's a good idea to split the different types of memory
> contexts out. That just exposes too much implementation detail stuff with=
out a
> good reason.

DSM needs to be independent anyway ... as for the others, perhaps we
could combine them, though that's pretty easily done later and for now
it's been useful to see them split out as we've been working on the
patch.

> I think the overhead even just the tracking implies right now is likely t=
oo
> high and needs to be optimized. It should be a single math operation, not
> tracking things in multiple fields. I don't think pg_sub_u64_overflow() s=
hould
> be in the path either, that suddenly adds conditional branches.  You real=
ly
> ought to look at the difference in assembly for the hot functions.

This has been improved in the most recent work and we'll have that
posted soon, probably best to hold off from larger review of this right
now- as mentioned, I was just trying to address the specific question in
this sub-thread since a new patch is coming soon.

Thanks,

Stephen

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

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

iQIzBAABCgAdFiEEwf6gbxKhD863zrx/7WyKOINHZFUFAmUxsm4ACgkQ7WyKOINH
ZFXP+A/9ERIKeM0ToQFXfSMjPDCdmVJJocuNSbpICEY8JYmhZMr9hHcZvV8Q+gws
fVqPoD0S5IpuZ21TOipAFJatOtHVhDFpvlMpr//fmokIXyzJnGmxL6OVYkKsRrcS
FriVwFvB207owm2hJLctFi8F3EshD4lmGfkmfTui0svPu2+cZ7HLLf7b6T2iZ0tM
fNjPdENWM1qJUEp1ivpPYSoU3VOsuGIpvOo6C+njul3DQonX00KimwD7xI9Eirj7
Uc8CYRTLRmobykruWpZavyoK3T1gwMMxt04CB4k+7p8tbXYF9dhDx4KpjXivTF9o
sjvV9oN+5BlfCQIKMpMRtG5eLmJyMAlYY2W9hcWCGnbJkBLoKFdtCc51Bvnj2SDk
4HxXTmGPAdoyNEnuL5FIByJ2PkO/ydHsB+mBB43IKhHEKeyb68jzQH/QViGjqM6G
OyZk0mu+gcRLbsHmSpOGjt8RrtmF+HGz2BJLJPfxCLPMohcbjkxj3t04pN9yjfz+
YS+BuscjpmgInKdwgLmbOv6VGtqtoqRucirEraG7UWIdALgb6LjGNc4PGfFeDB2u
6MaCZAbPQNRtTY4iXpJnmRGuSB18uliVcLCU5NtScr3j4r/jfHu8p4+yitrQoWgV
pSAUvskN+PIY65N+rfrklR/LexO6I8oMHejheVgWwBjUPd4GscI=
=5jjw
-----END PGP SIGNATURE-----

--FUkEYrCLnLL9itaa--