Date: Wed, 8 Sep 2021 12:50:08 +0900
From: Michael Paquier <michael@paquier.xyz>
To: "Bossart, Nathan" <bossartn@amazon.com>
Cc: Justin Pryzby <pryzby@telsasoft.com>,
	Andres Freund <andres@anarazel.de>,
	Magnus Hagander <magnus@hagander.net>,
	Mark Dilger <mark.dilger@enterprisedb.com>,
	Don Seiler <don@seiler.us>,
	"pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>
Subject: Re: Estimating HugePages Requirements?
Message-ID: <YTgy8PdP1+9s6NBz@paquier.xyz>
References: <YS8jgN37QvlrKnzQ@paquier.xyz>
 <298B6D88-7B71-4E1B-BFC4-E7BF3A32960D@amazon.com>
 <YTCCXOEdjbN9Sl+5@paquier.xyz>
 <23F67FC0-E432-4324-BEA4-F99B126510EA@amazon.com>
 <YTF+PTIwtDghavEc@paquier.xyz>
 <7ED221D0-0D75-46F8-8D6C-8AF70D8EABE7@amazon.com>
 <YTV8l8V+uKVcTVnV@paquier.xyz>
 <89EFD5FB-3C28-459C-9D6D-C9F93F7C2A54@amazon.com>
 <YTbjyHkTa2xBdzgf@paquier.xyz>
 <578A8F79-FD13-4408-865F-4D31EE5D8123@amazon.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
	protocol="application/pgp-signature"; boundary="0VgjkN/ZFaCFQmSp"
Content-Disposition: inline
In-Reply-To: <578A8F79-FD13-4408-865F-4D31EE5D8123@amazon.com>
Archived-At: 
 <https://www.postgresql.org/message-id/YTgy8PdP1%2B9s6NBz%40paquier.xyz>
Precedence: bulk


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

On Tue, Sep 07, 2021 at 05:08:43PM +0000, Bossart, Nathan wrote:
> On 9/6/21, 9:00 PM, "Michael Paquier" <michael@paquier.xyz> wrote:
>> +   sprintf(buf, "%lu MB", size_mb);
>> +   SetConfigOption("shared_memory_size", buf, PGC_INTERNAL, PGC_S_OVERR=
IDE);
>> One small-ish comment about 0002: there is no need to add the unit
>> into the buffer set as GUC_UNIT_MB would take care of that.  The patch
>> looks fine.
>=20
> I fixed this in v7.

Switched the variable name to shared_memory_size_mb for easier
grepping, moved it to a more correct location with the other read-only
GUCS, and applied 0002.  Well, 0001 here.

>> +#ifndef WIN32
>> +#include <sys/mman.h>
>> +#endif
>> So, this is needed in ipci.c to check for MAP_HUGETLB.  I am not much
>> a fan of moving around platform-specific checks when these have
>> remained local to each shmem implementation.  Could it be cleaner to
>> add GetHugePageSize() to win32_shmem.c and make it always declared in
>> the SysV implementation?
>=20
> I don't know if it's really all that much cleaner, but I did it this
> way in v7.  IMO it's a little weird that GetHugePageSize() doesn't
> return the value from GetLargePageMinimum(), but that's what we'd need
> to do to avoid setting huge_pages_required for Windows without any
> platform-specific checks.

Thanks.  Keeping MAP_HUGETLB within the SysV portions is an
improvement IMO.  That's subject to one's taste, perhaps.

After sleeping on it, I'd be fine to live with the logic based on the
new GUC flag called GUC_RUNTIME_COMPUTED to control if a parameter can
be looked at either an earlier or a later stage of the startup
sequences, with the later stage meaning that such parameters cannot be
checked if a server is running.  This case was originally broken
anyway, so it does not make it worse, just better.

+        This can be used on a running server for most parameters.  However,
+        the server must be shut down for some runtime-computed parameters
+        (e.g., <xref linkend=3D"guc-huge-pages-required"/>).
Perhaps we should add a couple of extra parameters here, like
shared_memory_size, and perhaps wal_segment_size?  The list does not
have to be complete, just meaningful enough.
--
Michael

--0VgjkN/ZFaCFQmSp
Content-Type: application/pgp-signature; name="signature.asc"

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

iQIzBAEBCAAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmE4MvAACgkQnvQgOdby
QH0pUhAAmQNffOrxpZ6R3Qk8AE+Zi1XS48Y/42TUEmAO/BPbICSApAbYkrGwWs9F
m3FnMrCPJIPnbFZa1o7TrIDnF1IJoMJAXVa2i+adXFb86tb2gp/upEYOqNjBaXSk
2WOPBbw+8o/AaHQsg2E5qHU5jlVN+oG+Nvax3/U+Xm8NtxR/WYZiADfknJcpIT7L
BylTi8OsJAv1GcSdIpDGYwhIRU31ntGHHlLrR1Qk3smSexH5rPggwO997/h8xDU4
0+lOUn7M3V8cY2AMKtcN3dAXWVhuaSc+W6pXwRRHAyfKde74P3NFJBv3fZOUMXNt
3wjDhU7vao3xU00Yk0WNqqi62DdU2XSk+Y0oiGmH/w85B0rtaVTMJ0fL6FO7yHgV
Yr44OKNMvZLRCS0mWjp8ws241me/FpkchdbJrGlwY2++CHXJ2vHwtQOIuPvX35lG
qCWUBztNoI0h/OM68Y5Oom8NSyQdjuB5L3Ap7QQqMDLLK4a9ax7LXRMEtW+q2o2D
jrRfxbnGqhAgpMQHNjM4JIU0j1VZh4IMYqU2qTVia5d/bh88e636YedJeE20Lr8l
CW9b8BGNqB2rtkTeRXALm/arlui7ws5I6RiBLvba56eP5F7y/hbAPo9852IOTmKc
j9DO4MlxgSeYkEKItk0cb67br0x4ZDkcmMn1UvtL2q7jEUnzoAs=
=pnzK
-----END PGP SIGNATURE-----

--0VgjkN/ZFaCFQmSp--