Feedback-ID: i0fe9450f:Fastmail
Date: Tue, 28 Mar 2023 09:35:30 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Justin Pryzby <pryzby@telsasoft.com>
Cc: Kyotaro Horiguchi <horikyota.ntt@gmail.com>, sfrost@snowman.net,
	alvherre@alvh.no-ip.org, andres@anarazel.de,
	nathandbossart@gmail.com, jchampion@timescale.com,
	john.naylor@enterprisedb.com, masao.fujii@oss.nttdata.com,
	noriyoshi.shinoda@hpe.com, pgsql-hackers@postgresql.org,
	rjuju123@gmail.com, sawada.mshk@gmail.com, tgl@sss.pgh.pa.us,
	thomas.munro@gmail.com
Subject: Re: Improve logging when using Huge Pages
Message-ID: <ZCI2UlFUxFz80otI@paquier.xyz>
References: <ZBfsbTEUnRcEyNao@paquier.xyz>
 <ZBplvQx7n0MGVr6O@telsasoft.com>
 <ZBqwHY7yZQATAp9N@paquier.xyz>
 <ZBt+tCS/Akj8BvHi@telsasoft.com>
 <ZBt/4EdbwLp2hJLx@paquier.xyz>
 <ZB0B+u5RMN8g52kF@telsasoft.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="XFniX6+ZnviHOpCF"
Content-Disposition: inline
In-Reply-To: <ZB0B+u5RMN8g52kF@telsasoft.com>
Archived-At: 
 <https://www.postgresql.org/message-id/ZCI2UlFUxFz80otI%40paquier.xyz>
Precedence: bulk


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

On Thu, Mar 23, 2023 at 08:50:50PM -0500, Justin Pryzby wrote:
> I'm sure it's possible, but it's also not worth writing a special
> implementation just to handle huge_pages_active, which is better written
> in 30 lines than in 300 lines.
>=20
> If we needed to avoid using a GUC, maybe it'd work to add
> huge_pages_active to PGShmemHeader.  But "avoid using gucs at all costs"
> isn't the goal, and using a GUC parallels all the similar, and related
> things without needing to allocate extra bits of shared_memory.

Yeah, I kind of agree that the complications are not appealing
compared to the use case.  FWIW, I am fine with just using "unknown"
even under -C because we don't know the status without the mmpa()
call.  And we don't want to assign what would be potentially a bunch
of memory when running that.

> This goes back to using a GUC, and:
>=20
>  - removes GUC_RUNTIME_COMPUTED, since that causes a useless error when
>    using -C if the server is running, rather than successfully printing
>    "unknown".
>  - I also renamed it from huge_pages_active =3D {true,false,unknown} to
>    huge_pages_STATUS =3D {on,off,unknown}.  This parallels huge_pages,
>    which is documented to accept values on/off/try.  Also, true/false
>    isn't how bools are displayed.
>  - I realized that the rename suggested implementing it as an enum GUC,
>    and re-using the existing HUGE_PAGES_{ON,OFF} values (and adding an
>    "UNKNOWN").  Maybe this also avoids Stephen's earlier objection to
>    using a string ?

huge_pages_status is OK here, as is reusing the enum struct to avoid
an unnecessary duplication.

> I mis-used cirrusci to check that the GUC does work correctly under
> windows.

You mean that you abused of it in some custom branch running the CI on
github?  If I may ask, what did you do to make sure that huge pages
are set when re-attaching a backend to a shmem area?

> If there's continuing aversions to using a GUC, please say so, and try
> to suggest an alternate implementation you think would be justified.
> It'd need to work under windows with EXEC_BACKEND.

Looking at the patch, it seems like that this should work even for
EXEC_BACKEND on *nix when a backend reattaches..  It may be better to
be sure of that, as well, if it has not been tested.

+++ b/src/backend/port/win32_shmem.c
@@ -327,6 +327,10 @@ retry:
            Sleep(1000);
            continue;
        }
+
+       SetConfigOption("huge_pages_status", (flProtect & SEC_LARGE_PAGES) ?
+                       "on" : "off", PGC_INTERNAL, PGC_S_DYNAMIC_DEFAULT)

Perhaps better to just move that at the end of PGSharedMemoryCreate()
for WIN32?

+     <varlistentry id=3D"guc-huge-pages-status" xreflabel=3D"huge_pages_st=
atus">
+      <term><varname>huge_pages_status</varname> (<type>enum</type>)
+      <indexterm>
+       <primary><varname>huge_pages_status</varname> configuration paramet=
er</primary>
+      </indexterm>
+      </term>
+      <listitem>
+       <para>
+        Reports the state of huge pages in the current instance.
+        See <xref linkend=3D"guc-huge-pages"/> for more information.
+       </para>
+      </listitem>
+     </varlistentry>

The patch needs to provide more details about the unknown state, IMO,
to make it crystal-clear to the users what are the limitations of this
GUC, especially regarding the fact that this is useful when "try"-ing
to allocate huge pages, and that the value can only be consulted after
the server has been started.
--
Michael

--XFniX6+ZnviHOpCF
Content-Type: application/pgp-signature; name="signature.asc"

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmQiNlIACgkQnvQgOdby
QH2TZw/+PKVgsCqPl/2gpFihNkfpm0FvZdtgzEcpRTa5Ltj88Wk8K99ogFEr5M3E
16mKh3Vuo2AVirbwYQF1U3bEXE4RmVpq7H6ux8T+JpNX/D4mCQOs56DCnnZkO/iz
oM/RqgYJQZHfc0KffblSNM7BR0BZrXaqcpW1xHOC9bPqBdNNvgMsPPC1zPILlzo9
t1DpP/rrh0k/adfnQRlzelsxJGkjWfuzvnUyN8OZUZ8SS3nCHf4kysk7GSzk6E1K
JrGRuV9V2zkmpF78UzlYBHlbSXwmZ5jme0+qpAwFolmPGNwCuI4aHYCaspGbHr14
RkYZcuNzqljDIxSh5WYut17LE1KcsQpUtRBjd0p3zB+xGQSUMzKKeLCjHiTPZmIu
d9MiA0K8kU1cELy2OtYTtYDVHw0DBYFi2+azUSnF0wheVx92teCTrDlBzEcFsYK3
qX4q2otdSB1jdJ8kUS+72m7MD04KRa8WQ7xZ2VG65AABXU7Yb9cUqIFzGhDrOL9N
Tv89zvJb2yQsUX0jJws8m/xJWbl8qzN1kmsY+4+Q295UF04z3eI8sGcpaKyVgarX
yzGU5kWltEe+se+rmQVB6fLMYwGPZUFUety3CDqHLZo0zxr+fwaeWvjeYmnSkp7U
y3vCPz1/+mT6v88kHPsU7qcfnBP26asmfig607StbOiV91FzlI4=
=jfaw
-----END PGP SIGNATURE-----

--XFniX6+ZnviHOpCF--