Date: Mon, 13 Mar 2023 15:03:50 -0500
From: Justin Pryzby <pryzby@telsasoft.com>
To: Stephen Frost <sfrost@snowman.net>
Cc: Nathan Bossart <nathandbossart@gmail.com>,
	Alvaro Herrera <alvherre@alvh.no-ip.org>,
	Andres Freund <andres@anarazel.de>,
	Michael Paquier <michael@paquier.xyz>,
	Kyotaro Horiguchi <horikyota.ntt@gmail.com>, thomas.munro@gmail.com,
	tgl@sss.pgh.pa.us, john.naylor@enterprisedb.com,
	noriyoshi.shinoda@hpe.com, jchampion@timescale.com,
	sawada.mshk@gmail.com, masao.fujii@oss.nttdata.com,
	pgsql-hackers@postgresql.org, rjuju123@gmail.com
Subject: Re: Improve logging when using Huge Pages
Message-ID: <ZA+Bpk/6LcYiUXnh@telsasoft.com>
References: <20230209094013.67varf63bihops33@alvherre.pgsql>
 <20230209192909.GA606606@nathanxps13>
 <20230213232245.GI1653@telsasoft.com>
 <20230214041852.GA1190218@nathanxps13>
 <20230215013256.GP1653@telsasoft.com>
 <20230215181317.GA1353037@nathanxps13>
 <20230308221656.GA3621885@nathanxps13>
 <ZAnuYi/Cb8QjRSoN@tamriel.snowman.net>
 <ZAoLoCPRf6j/HyQp@telsasoft.com>
 <ZAo7VQCm1UBt9spR@tamriel.snowman.net>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="LWcYZZvFG7E9C4r6"
Content-Disposition: inline
In-Reply-To: <ZAo7VQCm1UBt9spR@tamriel.snowman.net>
Archived-At: 
 <https://www.postgresql.org/message-id/ZA%2BBpk/6LcYiUXnh%40telsasoft.com>
Precedence: bulk


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

On Thu, Mar 09, 2023 at 03:02:29PM -0500, Stephen Frost wrote:
> * Justin Pryzby (pryzby@telsasoft.com) wrote:
> > On Thu, Mar 09, 2023 at 09:34:10AM -0500, Stephen Frost wrote:
> > > * Nathan Bossart (nathandbossart@gmail.com) wrote:
> > > > On Wed, Feb 15, 2023 at 10:13:17AM -0800, Nathan Bossart wrote:
> > > > > On Tue, Feb 14, 2023 at 07:32:56PM -0600, Justin Pryzby wrote:
> > > > >> On Mon, Feb 13, 2023 at 08:18:52PM -0800, Nathan Bossart wrote:
> > > > >>> I'm curious why you chose to make this a string instead of an enum.  There
> > > > >>> might be little practical difference, but since there are only three
> > > > >>> possible values, I wonder if it'd be better form to make it an enum.
> > > > >> 
> > > > >> It takes more code to write as an enum - see 002.txt.  I'm not convinced
> > > > >> this is better.
> > > > >> 
> > > > >> But your comment made me fix its <type>, and reconsider the strings,
> > > > >> which I changed to active={unknown/true/false} rather than {unk/on/off}.
> > > > >> It could also be active={unknown/yes/no}...
> > > > > 
> > > > > I think unknown/true/false is fine.  I'm okay with using a string if no one
> > > > > else thinks it should be an enum (or a bool).
> > > > 
> > > > There's been no response for this, so I guess we can proceed with a string
> > > > GUC.
> > > 
> > > Just happened to see this and I'm not really a fan of this being a
> > > string when it's pretty clear that's not what it actually is.
> > 
> > I don't understand what you mean by that.
> > Why do you say it isn't a string ?
> 
> Because it's clearly a yes/no, either the server is currently running
> with huge pages, or it isn't.  That's the definition of a boolean.

I originally implemented it as a boolean, and I changed it in response
to the feedback that postgres -C huge_pages_active should return
"unknown".

> > Is there an agreement to use a function, instead ?

Alvaro was -1 on using a function
Andres is +0 (?)
Nathan is +1
Stephen is +1

I'm -0.5, but I reimplemented it as a function.  I hope that helps it to
progress.  Please include a suggestion if there's better place for the
function or global var.

-- 
Justin

--LWcYZZvFG7E9C4r6
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v5-0001-add-pg_huge_pages_active.patch"

From cd171da2150e1ee8cfc6ca4bdee0a591df6f566b Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Mon, 23 Jan 2023 18:33:51 -0600
Subject: [PATCH v5] add pg_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        |  3 ++-
 doc/src/sgml/func.sgml          | 14 ++++++++++++++
 src/backend/port/sysv_shmem.c   |  2 ++
 src/backend/port/win32_shmem.c  |  2 ++
 src/backend/utils/adt/misc.c    | 11 +++++++++++
 src/include/catalog/pg_proc.dat |  5 +++++
 src/include/miscadmin.h         |  3 +++
 7 files changed, 39 insertions(+), 1 deletion(-)

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index e5c41cc6c6d..d66f73a494a 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -1700,23 +1700,24 @@ include_dir 'conf.d'
       </indexterm>
       </term>
       <listitem>
        <para>
         Controls whether huge pages are requested for the main shared memory
         area. Valid values are <literal>try</literal> (the default),
         <literal>on</literal>, and <literal>off</literal>.  With
         <varname>huge_pages</varname> set to <literal>try</literal>, the
         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 <function>pg_huge_pages_active()</function>.
        </para>
 
        <para>
         At present, this setting is supported only on Linux and Windows. The
         setting is ignored on other systems when set to
         <literal>try</literal>.  On Linux, it is only supported when
         <varname>shared_memory_type</varname> is set to <literal>mmap</literal>
         (the default).
        </para>
 
        <para>
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index a5bf2f01b57..4e99d5aff5c 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -22516,22 +22516,36 @@ SELECT * FROM pg_ls_dir('.') WITH ORDINALITY AS t(ls,n);
         To request information about a specific log file format, supply
         either <literal>csvlog</literal>, <literal>jsonlog</literal> or
         <literal>stderr</literal> as the
         value of the optional parameter. The result is <literal>NULL</literal>
         if the log format requested is not configured in
         <xref linkend="guc-log-destination"/>.
         The result reflects the contents of
         the <filename>current_logfiles</filename> file.
        </para></entry>
       </row>
 
+      <row>
+       <entry role="func_table_entry"><para role="func_signature">
+        <indexterm>
+         <primary>pg_huge_pages_active</primary>
+        </indexterm>
+        <function>pg_huge_pages_active</function> ()
+        <returnvalue>bool</returnvalue>
+       </para>
+       <para>
+        Reports whether huge pages are in use by the current instance.
+        See <xref linkend="guc-huge-pages"/> for more information.
+       </para></entry>
+      </row>
+
       <row>
        <entry role="func_table_entry"><para role="func_signature">
         <indexterm>
          <primary>pg_my_temp_schema</primary>
         </indexterm>
         <function>pg_my_temp_schema</function> ()
         <returnvalue>oid</returnvalue>
        </para>
        <para>
         Returns the OID of the current session's temporary schema, or zero if
         it has none (because it has not created any temporary tables).
diff --git a/src/backend/port/sysv_shmem.c b/src/backend/port/sysv_shmem.c
index eaba244bc9c..9ff2d7b3d38 100644
--- a/src/backend/port/sysv_shmem.c
+++ b/src/backend/port/sysv_shmem.c
@@ -619,22 +619,24 @@ CreateAnonymousSegment(Size *size)
 			allocsize += hugepagesize - (allocsize % hugepagesize);
 
 		ptr = mmap(NULL, allocsize, PROT_READ | PROT_WRITE,
 				   PG_MMAP_FLAGS | mmap_flags, -1, 0);
 		mmap_errno = errno;
 		if (huge_pages == HUGE_PAGES_TRY && ptr == MAP_FAILED)
 			elog(DEBUG1, "mmap(%zu) with MAP_HUGETLB failed, huge pages disabled: %m",
 				 allocsize);
 	}
 #endif
 
+	huge_pages_active = (ptr != MAP_FAILED);
+
 	if (ptr == MAP_FAILED && huge_pages != HUGE_PAGES_ON)
 	{
 		/*
 		 * Use the original size, not the rounded-up value, when falling back
 		 * to non-huge pages.
 		 */
 		allocsize = *size;
 		ptr = mmap(NULL, allocsize, PROT_READ | PROT_WRITE,
 				   PG_MMAP_FLAGS, -1, 0);
 		mmap_errno = errno;
 	}
diff --git a/src/backend/port/win32_shmem.c b/src/backend/port/win32_shmem.c
index 62e08074770..f97971ef340 100644
--- a/src/backend/port/win32_shmem.c
+++ b/src/backend/port/win32_shmem.c
@@ -319,22 +319,24 @@ retry:
 		 * If the segment already existed, CreateFileMapping() will return a
 		 * handle to the existing one and set ERROR_ALREADY_EXISTS.
 		 */
 		if (GetLastError() == ERROR_ALREADY_EXISTS)
 		{
 			CloseHandle(hmap);	/* Close the handle, since we got a valid one
 								 * to the previous segment. */
 			hmap = NULL;
 			Sleep(1000);
 			continue;
 		}
+
+		huge_pages_active = ((flProtect & SEC_LARGE_PAGES) != 0);
 		break;
 	}
 
 	/*
 	 * If the last call in the loop still returned ERROR_ALREADY_EXISTS, this
 	 * shared memory segment exists and we assume it belongs to somebody else.
 	 */
 	if (!hmap)
 		ereport(FATAL,
 				(errmsg("pre-existing shared memory block is still in use"),
 				 errhint("Check if there are any old server processes still running, and terminate them.")));
diff --git a/src/backend/utils/adt/misc.c b/src/backend/utils/adt/misc.c
index 5d78d6dc060..3c175d88c59 100644
--- a/src/backend/utils/adt/misc.c
+++ b/src/backend/utils/adt/misc.c
@@ -1070,11 +1070,22 @@ pg_get_replica_identity_index(PG_FUNCTION_ARGS)
 		PG_RETURN_NULL();
 }
 
 /*
  * Transition function for the ANY_VALUE aggregate
  */
 Datum
 any_value_transfn(PG_FUNCTION_ARGS)
 {
 	PG_RETURN_DATUM(PG_GETARG_DATUM(0));
 }
+
+bool huge_pages_active = false;
+
+/*
+ * Return status of huge pages.
+ */
+Datum
+pg_huge_pages_active(PG_FUNCTION_ARGS)
+{
+	PG_RETURN_BOOL(huge_pages_active);
+}
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index ae658e721fd..7cfa11c994c 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -1811,22 +1811,27 @@
   proname => 'bpchar', prorettype => 'bpchar', proargtypes => 'char',
   prosrc => 'char_bpchar' },
 
 { oid => '861', descr => 'name of the current database',
   proname => 'current_database', provolatile => 's', prorettype => 'name',
   proargtypes => '', prosrc => 'current_database' },
 { oid => '817', descr => 'get the currently executing query',
   proname => 'current_query', proisstrict => 'f', provolatile => 'v',
   proparallel => 'r', prorettype => 'text', proargtypes => '',
   prosrc => 'current_query' },
 
+{ oid => '8218', descr => 'get the state of huge pages',
+  proname => 'pg_huge_pages_active', proisstrict => 't', provolatile => 's',
+  proparallel => 's', prorettype => 'bool', proargtypes => '',
+  prosrc => 'pg_huge_pages_active' },
+
 { oid => '3399',
   proname => 'int8_mul_cash', prorettype => 'money',
   proargtypes => 'int8 money', prosrc => 'int8_mul_cash' },
 { oid => '862',
   proname => 'int4_mul_cash', prorettype => 'money',
   proargtypes => 'int4 money', prosrc => 'int4_mul_cash' },
 { oid => '863',
   proname => 'int2_mul_cash', prorettype => 'money',
   proargtypes => 'int2 money', prosrc => 'int2_mul_cash' },
 { oid => '3344',
   proname => 'cash_mul_int8', prorettype => 'money',
diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h
index 06a86f9ac1f..8169e5bd6dd 100644
--- a/src/include/miscadmin.h
+++ b/src/include/miscadmin.h
@@ -268,22 +268,25 @@ extern PGDLLIMPORT int VacuumCostPageMiss;
 extern PGDLLIMPORT int VacuumCostPageDirty;
 extern PGDLLIMPORT int VacuumCostLimit;
 extern PGDLLIMPORT double VacuumCostDelay;
 
 extern PGDLLIMPORT int64 VacuumPageHit;
 extern PGDLLIMPORT int64 VacuumPageMiss;
 extern PGDLLIMPORT int64 VacuumPageDirty;
 
 extern PGDLLIMPORT int VacuumCostBalance;
 extern PGDLLIMPORT bool VacuumCostActive;
 
+/* in adt/misc.c */
+extern PGDLLIMPORT bool huge_pages_active;
+
 
 /* in tcop/postgres.c */
 
 typedef char *pg_stack_base_t;
 
 extern pg_stack_base_t set_stack_base(void);
 extern void restore_stack_base(pg_stack_base_t base);
 extern void check_stack_depth(void);
 extern bool stack_is_too_deep(void);
 
 /* in tcop/utility.c */
-- 
2.34.1


--LWcYZZvFG7E9C4r6--