Date: Tue, 20 Jun 2023 18:44:20 -0500
From: Justin Pryzby <pryzby@telsasoft.com>
To: Michael Paquier <michael@paquier.xyz>
Cc: Nathan Bossart <nathandbossart@gmail.com>,
	Kyotaro Horiguchi <horikyota.ntt@gmail.com>, sfrost@snowman.net,
	alvherre@alvh.no-ip.org, andres@anarazel.de,
	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: <ZJI51JzMSAyTkEjW@telsasoft.com>
References: <ZBfsbTEUnRcEyNao@paquier.xyz>
 <ZBplvQx7n0MGVr6O@telsasoft.com>
 <ZBqwHY7yZQATAp9N@paquier.xyz>
 <ZBt+tCS/Akj8BvHi@telsasoft.com>
 <ZBt/4EdbwLp2hJLx@paquier.xyz>
 <20230420211617.GA1435672@nathanxps13>
 <ZFByzj4BUTE+sdhs@paquier.xyz>
 <20230612213715.GA178071@nathanxps13>
 <ZIgDpuAaHvm7+e/7@paquier.xyz>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="VHnddftIsH1nzTLp"
Content-Disposition: inline
In-Reply-To: <ZIgDpuAaHvm7+e/7@paquier.xyz>
Archived-At: 
 <https://www.postgresql.org/message-id/ZJI51JzMSAyTkEjW%40telsasoft.com>
Precedence: bulk


--VHnddftIsH1nzTLp
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Tue, Jun 13, 2023 at 02:50:30PM +0900, Michael Paquier wrote:
> On Mon, Jun 12, 2023 at 02:37:15PM -0700, Nathan Bossart wrote:
> > Fair enough.  I know I've been waffling in the GUC versus function
> > discussion, but FWIW v7 of the patch looks reasonable to me.
> 
> +       Assert(strcmp("unknown", GetConfigOption("huge_pages_status", false, false)) != 0);
> 
> Not sure that there is anything to gain with this assertion in
> CreateSharedMemoryAndSemaphores(), because this is pretty much what
> check_GUC_init() looks after?

It seems like you misread the assertion, so I added a comment about it.
Indeed, the assertion addresses the other question you asked later.

That's what I already commented about, and the reason I found it
compelling not to use a boolean.

On Thu, Apr 06, 2023 at 04:57:58PM -0500, Justin Pryzby wrote:
> I added an assert to check that a running server won't output
> "unknown".

On Wed, Jun 14, 2023 at 09:15:35AM +0900, Michael Paquier wrote:
> There was a second thing that bugged me here.  Would it be worth
> adding some checks on huge_pages_status to make sure that it is never
> reported as unknown when the server is up and running?

-- 
Justin

--VHnddftIsH1nzTLp
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v8-0001-add-GUC-huge_pages_active.patch"

From 00234f5a0c14e2569ac1e7dab89907196f3ca9e1 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Mon, 23 Jan 2023 18:33:51 -0600
Subject: [PATCH v8] add GUC: huge_pages_active

This is useful to show the current state of huge pages when
huge_pages=try.  The effective status is not otherwise visible without
OS level tools like gdb or /proc/N/smaps.

https://www.postgresql.org/message-id/flat/TU4PR8401MB1152EBB0D271F827E2E37A01EECC9@TU4PR8401MB1152.NAMPRD84.PROD.OUTLOOK.COM
---
 doc/src/sgml/config.sgml            | 21 ++++++++++++++++++++-
 src/backend/port/sysv_shmem.c       | 10 ++++++++++
 src/backend/port/win32_shmem.c      |  5 +++++
 src/backend/storage/ipc/ipci.c      |  7 +++++++
 src/backend/utils/misc/guc_tables.c | 20 ++++++++++++++++++++
 src/include/storage/pg_shmem.h      |  5 +++--
 6 files changed, 65 insertions(+), 3 deletions(-)

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 6262cb7bb2f..e69afae71bf 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -1727,7 +1727,8 @@ include_dir 'conf.d'
         server will try to request huge pages, but fall back to the default if
         that fails. With <literal>on</literal>, failure to request huge pages
         will prevent the server from starting up. With <literal>off</literal>,
-        huge pages will not be requested.
+        huge pages will not be requested.  The actual state of huge pages is
+        indicated by the server variable <xref linkend="guc-huge-pages-status"/>.
        </para>
 
        <para>
@@ -10738,6 +10739,24 @@ dynamic_library_path = 'C:\tools\postgresql;H:\my_project\lib;$libdir'
       </listitem>
      </varlistentry>
 
+     <varlistentry id="guc-huge-pages-status" xreflabel="huge_pages_status">
+      <term><varname>huge_pages_status</varname> (<type>enum</type>)
+      <indexterm>
+       <primary><varname>huge_pages_status</varname> configuration parameter</primary>
+      </indexterm>
+      </term>
+      <listitem>
+       <para>
+        Reports the state of huge pages in the current instance:
+        <literal>on</literal>, <literal>off</literal>, or (if displayed with
+        <literal>postgres -C</literal>) <literal>unknown</literal>.
+        This parameter is useful to determine whether allocation of huge pages
+        was successful when <literal>huge_pages=try</literal>.
+        See <xref linkend="guc-huge-pages"/> for more information.
+       </para>
+      </listitem>
+     </varlistentry>
+
      <varlistentry id="guc-integer-datetimes" xreflabel="integer_datetimes">
       <term><varname>integer_datetimes</varname> (<type>boolean</type>)
       <indexterm>
diff --git a/src/backend/port/sysv_shmem.c b/src/backend/port/sysv_shmem.c
index eaba244bc9c..0e710237ea1 100644
--- a/src/backend/port/sysv_shmem.c
+++ b/src/backend/port/sysv_shmem.c
@@ -627,6 +627,10 @@ CreateAnonymousSegment(Size *size)
 	}
 #endif
 
+	/* Report whether huge pages are in use */
+	SetConfigOption("huge_pages_status", ptr == MAP_FAILED ? "off" : "on",
+					PGC_INTERNAL, PGC_S_DYNAMIC_DEFAULT);
+
 	if (ptr == MAP_FAILED && huge_pages != HUGE_PAGES_ON)
 	{
 		/*
@@ -737,8 +741,14 @@ PGSharedMemoryCreate(Size size,
 		sysvsize = sizeof(PGShmemHeader);
 	}
 	else
+	{
 		sysvsize = size;
 
+		/* huge pages are only available with mmap */
+		SetConfigOption("huge_pages_status", "off",
+						PGC_INTERNAL, PGC_S_DYNAMIC_DEFAULT);
+	}
+
 	/*
 	 * Loop till we find a free IPC key.  Trust CreateDataDirLockFile() to
 	 * ensure no more than one postmaster per data directory can enter this
diff --git a/src/backend/port/win32_shmem.c b/src/backend/port/win32_shmem.c
index 62e08074770..87f0b001915 100644
--- a/src/backend/port/win32_shmem.c
+++ b/src/backend/port/win32_shmem.c
@@ -327,6 +327,11 @@ retry:
 			Sleep(1000);
 			continue;
 		}
+
+		/* Report whether huge pages are in use */
+		SetConfigOption("huge_pages_status", (flProtect & SEC_LARGE_PAGES) ?
+						"on" : "off", PGC_INTERNAL, PGC_S_DYNAMIC_DEFAULT);
+
 		break;
 	}
 
diff --git a/src/backend/storage/ipc/ipci.c b/src/backend/storage/ipc/ipci.c
index 8f1ded7338f..5901a3bd8eb 100644
--- a/src/backend/storage/ipc/ipci.c
+++ b/src/backend/storage/ipc/ipci.c
@@ -190,6 +190,13 @@ CreateSharedMemoryAndSemaphores(void)
 		 */
 		seghdr = PGSharedMemoryCreate(size, &shim);
 
+		/*
+		 * Make sure that huge pages are never reported as "unknown"
+		 * while the server is running.
+		 */
+		Assert(strcmp("unknown",
+					GetConfigOption("huge_pages_status", false, false)) != 0);
+
 		InitShmemAccess(seghdr);
 
 		/*
diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c
index 977027ec8cb..37a97696dd5 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -365,6 +365,13 @@ static const struct config_enum_entry huge_pages_options[] = {
 	{NULL, 0, false}
 };
 
+static const struct config_enum_entry huge_pages_status_options[] = {
+	{"off", HUGE_PAGES_OFF, false},
+	{"on", HUGE_PAGES_ON, false},
+	{"unknown", HUGE_PAGES_UNKNOWN, false},
+	{NULL, 0, false}
+};
+
 static const struct config_enum_entry recovery_prefetch_options[] = {
 	{"off", RECOVERY_PREFETCH_OFF, false},
 	{"on", RECOVERY_PREFETCH_ON, false},
@@ -551,6 +558,8 @@ int			ssl_renegotiation_limit;
 int			huge_pages = HUGE_PAGES_TRY;
 int			huge_page_size;
 
+int			huge_pages_status = HUGE_PAGES_UNKNOWN;
+
 /*
  * These variables are all dummies that don't do anything, except in some
  * cases provide the value for SHOW to display.  The real state is elsewhere
@@ -4893,6 +4902,17 @@ struct config_enum ConfigureNamesEnum[] =
 		NULL, NULL, NULL
 	},
 
+	{
+		{"huge_pages_status", PGC_INTERNAL, PRESET_OPTIONS,
+			gettext_noop("Indicates the status of huge pages."),
+			NULL,
+			GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE
+		},
+		&huge_pages_status,
+		HUGE_PAGES_UNKNOWN, huge_pages_status_options,
+		NULL, NULL, NULL
+	},
+
 	{
 		{"recovery_prefetch", PGC_SIGHUP, WAL_RECOVERY,
 			gettext_noop("Prefetch referenced blocks during recovery."),
diff --git a/src/include/storage/pg_shmem.h b/src/include/storage/pg_shmem.h
index 4dd05f156d5..bfda42b0b26 100644
--- a/src/include/storage/pg_shmem.h
+++ b/src/include/storage/pg_shmem.h
@@ -46,12 +46,13 @@ extern PGDLLIMPORT int shared_memory_type;
 extern PGDLLIMPORT int huge_pages;
 extern PGDLLIMPORT int huge_page_size;
 
-/* Possible values for huge_pages */
+/* Possible values for huge_pages/huge_pages_status */
 typedef enum
 {
 	HUGE_PAGES_OFF,
 	HUGE_PAGES_ON,
-	HUGE_PAGES_TRY
+	HUGE_PAGES_TRY,		/* only for huge_pages */
+	HUGE_PAGES_UNKNOWN	/* only for huge_pages_status */
 }			HugePagesType;
 
 /* Possible values for shared_memory_type */
-- 
2.34.1


--VHnddftIsH1nzTLp--