Subject: Re: Improve logging when using Huge Pages

From: Alvaro Herrera <[email-masked]>
Date: Thu, 9 Feb 2023 10:40:13 +0100

On 2023-Feb-08, Justin Pryzby wrote:

> I don't think it makes sense to run postgres -C huge_pages_active,
> however, so I see no issue that that would always returns "false".

Hmm, I would initialize it to return "unknown" rather than "off" — and
make sure it turns "off" at the appropriate time.  Otherwise you're just
moving the confusion elsewhere.

> If need be, maybe the documentation could say "indicates whether huge
> pages are active for the running server".

Dunno, that seems way too subtle.

> Does anybody else want to vote for a function rather than a
> RUNTIME_COMPUTED GUC ?

I don't think I'd like to have SELECT show_block_size() et al, so I'd
rather not go that way.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"¿Qué importan los años?  Lo que realmente importa es comprobar que
a fin de cuentas la mejor edad de la vida es estar vivo"  (Mafalda)

---

From: Nathan Bossart <[email-masked]>
Date: Thu, 9 Feb 2023 11:29:09 -0800

> moving the confusion elsewhere.

I think this approach would address my concerns about using a GUC.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

---

From: Justin Pryzby <[email-masked]>
Date: Mon, 13 Feb 2023 17:22:45 -0600

> I think this approach would address my concerns about using a GUC.

Done that way.  This also fixes the logic in win32_shmem.c.

-- 
Justin
>From 5ead7ba3711dd7a0bb9ec083ebd60049f50f9907 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <[email-masked]>
Date: Mon, 23 Jan 2023 18:33:51 -0600
Subject: [PATCH v3] 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/[email-masked]
---
 doc/src/sgml/config.sgml            | 17 ++++++++++++++++-
 src/backend/port/sysv_shmem.c       |  3 +++
 src/backend/port/win32_shmem.c      |  4 ++++
 src/backend/utils/misc/guc_tables.c | 12 ++++++++++++
 4 files changed, 35 insertions(+), 1 deletion(-)

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 8c56b134a84..3770c7dc254 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 the server variable <xref linkend="guc-huge-pages-active"/>.
        </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>
@@ -10679,22 +10680,36 @@ dynamic_library_path = 'C:\tools\postgresql;H:\my_project\lib;$libdir'
         with assertions enabled. That is the case if the
         macro <symbol>USE_ASSERT_CHECKING</symbol> is defined
         when <productname>PostgreSQL</productname> is built (accomplished
         e.g., by the <command>configure</command> option
         <option>--enable-cassert</option>). By
         default <productname>PostgreSQL</productname> is built without
         assertions.
        </para>
       </listitem>
      </varlistentry>
 
+     <varlistentry id="guc-huge-pages-active" xreflabel="huge_pages_active">
+      <term><varname>huge_pages_active</varname> (<type>boolean</type>)
+      <indexterm>
+       <primary><varname>huge_pages_active</varname> configuration parameter</primary>
+      </indexterm>
+      </term>
+      <listitem>
+       <para>
+        Reports whether huge pages are in use by the current process.
+        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>
        <primary><varname>integer_datetimes</varname> configuration parameter</primary>
       </indexterm>
       </term>
       <listitem>
        <para>
         Reports whether <productname>PostgreSQL</productname> was built with support for
         64-bit-integer dates and times.  As of <productname>PostgreSQL</productname> 10,
         this is always <literal>on</literal>.
diff --git a/src/backend/port/sysv_shmem.c b/src/backend/port/sysv_shmem.c
index eaba244bc9c..48ead4d27bf 100644
--- a/src/backend/port/sysv_shmem.c
+++ b/src/backend/port/sysv_shmem.c
@@ -619,22 +619,25 @@ 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
 
+	SetConfigOption("huge_pages_active", ptr == MAP_FAILED ? "off" : "on",
+					PGC_INTERNAL, PGC_S_DYNAMIC_DEFAULT);
+
 	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..0bf594c8bf9 100644
--- a/src/backend/port/win32_shmem.c
+++ b/src/backend/port/win32_shmem.c
@@ -319,22 +319,26 @@ 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;
 		}
+
+		SetConfigOption("huge_pages_active", (flProtect & SEC_LARGE_PAGES) ?
+						"on" : "off", PGC_INTERNAL, PGC_S_DYNAMIC_DEFAULT);
+
 		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/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c
index b21698934c8..1a298f16c81 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -554,22 +554,23 @@ static int	server_version_num;
 #define	DEFAULT_SYSLOG_FACILITY LOG_LOCAL0
 #else
 #define	DEFAULT_SYSLOG_FACILITY 0
 #endif
 static int	syslog_facility = DEFAULT_SYSLOG_FACILITY;
 
 static char *timezone_string;
 static char *log_timezone_string;
 static char *timezone_abbreviations_string;
 static char *data_directory;
 static char *session_authorization_string;
+static char *huge_pages_active = "unknown"; /* dynamically set */
 static int	max_function_args;
 static int	max_index_keys;
 static int	max_identifier_length;
 static int	block_size;
 static int	segment_size;
 static int	shared_memory_size_mb;
 static int	shared_memory_size_in_huge_pages;
 static int	wal_block_size;
 static bool data_checksums;
 static bool integer_datetimes;
 
@@ -4516,22 +4517,33 @@ struct config_string ConfigureNamesString[] =
 	{
 		{"backtrace_functions", PGC_SUSET, DEVELOPER_OPTIONS,
 			gettext_noop("Log backtrace for errors in these functions."),
 			NULL,
 			GUC_NOT_IN_SAMPLE
 		},
 		&backtrace_functions,
 		"",
 		check_backtrace_functions, assign_backtrace_functions, NULL
 	},
 
+	{
+		{"huge_pages_active", PGC_INTERNAL, PRESET_OPTIONS,
+			gettext_noop("Indicates whether huge pages are in use."),
+			NULL,
+			GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE | GUC_RUNTIME_COMPUTED
+		},
+		&huge_pages_active,
+		"unknown",
+		NULL, NULL, NULL
+	},
+
 	/* End-of-list marker */
 	{
 		{NULL, 0, 0, NULL, NULL}, NULL, NULL, NULL, NULL, NULL
 	}
 };

 struct config_enum ConfigureNamesEnum[] =
 {
 	{
 		{"backslash_quote", PGC_USERSET, COMPAT_OPTIONS_PREVIOUS,
-- 
2.34.1

---

From: Nathan Bossart <[email-masked]>
Date: Mon, 13 Feb 2023 20:18:52 -0800

> +        See <xref linkend="guc-huge-pages"/> for more information.

nitpick: Should this say "server" instead of "current process"?

> +static char *huge_pages_active = "unknown"; /* dynamically set */

nitpick: Does this need to be initialized here?

> +			GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE | GUC_RUNTIME_COMPUTED

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.

---

From: Justin Pryzby <[email-masked]>
Date: Tue, 14 Feb 2023 19:32:56 -0600

> nitpick: Should this say "server" instead of "current process"?

It should probably say "instance" :)

> nitpick: Does this need to be initialized here?

None of the GUCs' C vars need to be initialized, since the guc machinery
will do it. 

...but the convention is that they *are* initialized - and that's now
partially enforced.

See:
d9d873bac67047cfacc9f5ef96ee488f2cb0f1c3
7d25958453a60337bcb7bcc986e270792c007ea4
a73952b795632b2cf5acada8476e7cf75857e9be

> 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}...

-- 
Justin
>From 7bf67d398902570ffc68a6b78265a0e346503392 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <[email-masked]>
Date: Mon, 23 Jan 2023 18:33:51 -0600
Subject: [PATCH v4 1/2] add GUC: huge_pages_active

index 8c56b134a84..3ff301edf8a 100644
 
        <para>
 
        <para>
 
+      <term><varname>huge_pages_active</varname> (<type>string</type>)
+        Reports whether huge pages are in use by the current instance.
index eaba244bc9c..89ec5c30364 100644

+	SetConfigOption("huge_pages_active", ptr == MAP_FAILED ? "false" : "true",
index 62e08074770..fa72b3d1f8f 100644
+						"true" : "false", PGC_INTERNAL, PGC_S_DYNAMIC_DEFAULT);
+
 		break;
 	}
 
 	/*

 
+	{

>From 96ad5dcd77bb8d2e4c9c3ca06ae4dcf43017defe Mon Sep 17 00:00:00 2001
From: Justin Pryzby <[email-masked]>
Date: Tue, 14 Feb 2023 15:29:41 -0600
Subject: [PATCH v4 2/2] f! convert to an enum

---
 doc/src/sgml/config.sgml            |  2 +-
 src/backend/utils/misc/guc_tables.c | 32 ++++++++++++++++++-----------
 src/include/storage/pg_shmem.h      |  8 ++++++++
 3 files changed, 29 insertions(+), 13 deletions(-)

index 3ff301edf8a..1f6f71a2826 100644
@@ -10681,23 +10681,23 @@ dynamic_library_path = 'C:\tools\postgresql;H:\my_project\lib;$libdir'
 
      <varlistentry id="guc-huge-pages-active" xreflabel="huge_pages_active">
-      <term><varname>huge_pages_active</varname> (<type>string</type>)
+      <term><varname>huge_pages_active</varname> (<type>enum</type>)
       <indexterm>
        <primary><varname>huge_pages_active</varname> configuration parameter</primary>
       </indexterm>
       </term>
       <listitem>
        <para>
         Reports whether huge pages are in use by the current instance.
         See <xref linkend="guc-huge-pages"/> for more information.
        </para>
       </listitem>
      </varlistentry>
diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c
index 1a298f16c81..f9c98997492 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -339,22 +339,29 @@ static const struct config_enum_entry huge_pages_options[] = {
 	{"on", HUGE_PAGES_ON, false},
 	{"try", HUGE_PAGES_TRY, false},
 	{"true", HUGE_PAGES_ON, true},
 	{"false", HUGE_PAGES_OFF, true},
 	{"yes", HUGE_PAGES_ON, true},
 	{"no", HUGE_PAGES_OFF, true},
 	{"1", HUGE_PAGES_ON, true},
 	{"0", HUGE_PAGES_OFF, true},
 	{NULL, 0, false}
 };
 
+static const struct config_enum_entry huge_pages_active_options[] = {
+	{"unknown", HUGE_PAGES_ACTIVE_UNKNOWN, false},
+	{"false", HUGE_PAGES_ACTIVE_FALSE, false},
+	{"true", HUGE_PAGES_ACTIVE_TRUE, false},
+	{NULL, 0, false}
+};
+
 static const struct config_enum_entry recovery_prefetch_options[] = {
 	{"off", RECOVERY_PREFETCH_OFF, false},
 	{"on", RECOVERY_PREFETCH_ON, false},
 	{"try", RECOVERY_PREFETCH_TRY, false},
 	{"true", RECOVERY_PREFETCH_ON, true},
 	{"false", RECOVERY_PREFETCH_OFF, true},
 	{"yes", RECOVERY_PREFETCH_ON, true},
 	{"no", RECOVERY_PREFETCH_OFF, true},
 	{"1", RECOVERY_PREFETCH_ON, true},
 	{"0", RECOVERY_PREFETCH_OFF, true},
 	{NULL, 0, false}
@@ -527,22 +534,24 @@ int			tcp_user_timeout;
  * renegotiation and therefore always try to zero it.
  */
 int			ssl_renegotiation_limit;
 
 /*
  * This really belongs in pg_shmem.c, but is defined here so that it doesn't
  * need to be duplicated in all the different implementations of pg_shmem.c.
  */
 int			huge_pages = HUGE_PAGES_TRY;
 int			huge_page_size;
 
+int			huge_pages_active = HUGE_PAGES_ACTIVE_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
  * and is kept in sync by assign_hooks.
  */
 static char *syslog_ident_str;
 static double phony_random_seed;
 static char *client_encoding_string;
 static char *datestyle_string;
 static char *locale_collate;
 static char *locale_ctype;
@@ -554,23 +563,22 @@ static int	server_version_num;
 #define	DEFAULT_SYSLOG_FACILITY LOG_LOCAL0
 #else
 #define	DEFAULT_SYSLOG_FACILITY 0
 #endif
 static int	syslog_facility = DEFAULT_SYSLOG_FACILITY;
 
-static char *huge_pages_active = "unknown"; /* dynamically set */
 
@@ -4517,33 +4525,22 @@ struct config_string ConfigureNamesString[] =
 	{
 		{"backtrace_functions", PGC_SUSET, DEVELOPER_OPTIONS,
 			gettext_noop("Log backtrace for errors in these functions."),
 			NULL,
 			GUC_NOT_IN_SAMPLE
 		},
 		&backtrace_functions,
 		"",
 		check_backtrace_functions, assign_backtrace_functions, NULL
 	},
 
-	{
-		{"huge_pages_active", PGC_INTERNAL, PRESET_OPTIONS,
-			gettext_noop("Indicates whether huge pages are in use."),
-			NULL,
-			GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE | GUC_RUNTIME_COMPUTED
-		},
-		&huge_pages_active,
-		"unknown",
-		NULL, NULL, NULL
-	},
-
 	/* End-of-list marker */
 	{
 		{NULL, 0, 0, NULL, NULL}, NULL, NULL, NULL, NULL, NULL
 	}
 };

 struct config_enum ConfigureNamesEnum[] =
 {
 	{
 		{"backslash_quote", PGC_USERSET, COMPAT_OPTIONS_PREVIOUS,
@@ -4845,22 +4842,33 @@ struct config_enum ConfigureNamesEnum[] =
 
 	{
 		{"huge_pages", PGC_POSTMASTER, RESOURCES_MEM,
 			gettext_noop("Use of huge pages on Linux or Windows."),
 			NULL
 		},
 		&huge_pages,
 		HUGE_PAGES_TRY, huge_pages_options,
 		NULL, NULL, NULL
 	},
 
+	{
+		{"huge_pages_active", PGC_INTERNAL, PRESET_OPTIONS,
+			gettext_noop("Indicates whether huge pages are in use."),
+			NULL,
+			GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE | GUC_RUNTIME_COMPUTED
+		},
+		&huge_pages_active,
+		HUGE_PAGES_ACTIVE_UNKNOWN, huge_pages_active_options,
+		NULL, NULL, NULL
+	},
+
 	{
 		{"recovery_prefetch", PGC_SIGHUP, WAL_RECOVERY,
 			gettext_noop("Prefetch referenced blocks during recovery."),
 			gettext_noop("Look ahead in the WAL to find references to uncached data.")
 		},
 		&recovery_prefetch,
 		RECOVERY_PREFETCH_TRY, recovery_prefetch_options,
 		check_recovery_prefetch, assign_recovery_prefetch, NULL
 	},
 
 	{
diff --git a/src/include/storage/pg_shmem.h b/src/include/storage/pg_shmem.h
index 4dd05f156d5..1699bf64238 100644
--- a/src/include/storage/pg_shmem.h
+++ b/src/include/storage/pg_shmem.h
@@ -46,22 +46,30 @@ extern PGDLLIMPORT int shared_memory_type;
 extern PGDLLIMPORT int huge_pages;
 extern PGDLLIMPORT int huge_page_size;
 
 /* Possible values for huge_pages */
 typedef enum
 {
 	HUGE_PAGES_OFF,
 	HUGE_PAGES_ON,
 	HUGE_PAGES_TRY
 }			HugePagesType;
 
+/* Possible values for huge_pages_active */
+typedef enum
+{
+	HUGE_PAGES_ACTIVE_UNKNOWN,
+	HUGE_PAGES_ACTIVE_FALSE,
+	HUGE_PAGES_ACTIVE_TRUE,
+}			HugePagesActiveType;
+
 /* Possible values for shared_memory_type */
 typedef enum
 {
 	SHMEM_TYPE_WINDOWS,
 	SHMEM_TYPE_SYSV,
 	SHMEM_TYPE_MMAP
 }			PGShmemType;
 
 #ifndef WIN32
 extern PGDLLIMPORT unsigned long UsedShmemSegID;
 #else
-- 
2.34.1

---

From: Nathan Bossart <[email-masked]>
Date: Wed, 15 Feb 2023 10:13:17 -0800

> a73952b795632b2cf5acada8476e7cf75857e9be

I see.  This looked a little strange to me because many of the other
variables are uninitialized.  In a73952b, I see that we allow the variables
for string GUCs to be initialized to NULL.  Anyway, this is only a nitpick.
I don't feel strongly about it.

> 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).

---

From: Nathan Bossart <[email-masked]>
Date: Wed, 8 Mar 2023 14:16:56 -0800

> 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.

I still think we should say "server" in place of "current instance" here.

I don't think we need to use GUC_RUNTIME_COMPUTED.  'postgres -C' seems to
always report "unknown" for this GUC, so all this would do is cause that
command to error unnecessarily when the server is running.

It might be worth documenting exactly what "unknown" means.  IIUC you'll
only ever see "on" or "off" via SHOW or pg_settings, which doesn't seem
tremendously obvious.

---

From: Stephen Frost <[email-masked]>
Date: Thu, 9 Mar 2023 09:34:10 -0500

Greetings,

> There's been no response for this, so I guess we can proceed with a string

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 still think we should say "server" in place of "current instance" here.

We certainly use 'server' a lot more in config.sgml than we do
'instance'.  "currently running server" might be closer to how we
describe a running PG system in other parts (we talk about "currently
running server processes", "while the server is running", "When running
a standby server", "when the server is running"; "instance" is used much
less and seems to more typically refer to 'state of files on disk' in my
reading vs. 'actively running process' though there's some of each).

> command to error unnecessarily when the server is running.

... or we could consider adjusting things to actually try the mmap() and
find out if we'd end up with huge pages or not.  Naturally we'd only
want to do that if the server isn't running though and erroring if it is
would be perfectly correct.  Either that or just refusing to provide it
by an error or other approach (see below) seems entirely reasonable.

> only ever see "on" or "off" via SHOW or pg_settings, which doesn't seem

If we could get rid of that case and just make this a boolean, that
seems like it'd really be the best answer.

To that end- perhaps this isn't appropriate as a GUC at all?  Why not
have this just be a system information function?  Functionally it really
provides the same info- if the server is running then you get back
either true or false, if it's not then you can't call it but that's
hardly different from an unknown or error result.

Thanks,

Stephen

---

From: Justin Pryzby <[email-masked]>
Date: Thu, 9 Mar 2023 10:38:56 -0600

> 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 ?

> reading vs. 'actively running process' though there's some of each).

I called it "instance" since the GUC has no meaning when it's not
running.  I'm fine to rename it to "running server".

> find out if we'd end up with huge pages or not.

That seems like a bad idea, since it might work one moment and fail one
moment later.  If we could tell in advance whether it was going to work,
we wouldn't be here, and probably also wouldn't have invented
huge_pages=true.

> hardly different from an unknown or error result.

We talked about making it a function ~6 weeks ago.

Is there an agreement to use a function, instead ?

-- 
Justin

---

From: Stephen Frost <[email-masked]>
Date: Thu, 9 Mar 2023 15:02:29 -0500

Greetings,

> 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.
Sure, anything can be cast to text but when there's a data type that
fits better, that's almost uniformly better to use.

> running.  I'm fine to rename it to "running server".

Great, I do think that would match better with the rest of the
documentation.

> we wouldn't be here, and probably also wouldn't have invented

Sure it might ... but I tend to disagree that it's actually all that
likely for it to end up being as inconsistent as that and it'd be nice
to be able to see if the server will end up successfully starting (for
this part, at least), or not, when configured with huge pages set to on,
or if starting with 'try' is likely to result in it actually using huge
pages, or not.

> We talked about making it a function ~6 weeks ago.

Oh, good, glad I'm not the only one to have thought of that.

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

Looking back at the arguments for having it be a GUC ... I just don't
really see any of them as very strong.  Not that I feel super strongly
about it being a function either, but it's certainly not a configuration
variable and it also isn't really available with postgres -C (and
therefore doesn't actually go along with how the *size GUCs work).  It's
literally information about the running system that the user might be
curious about ... and that sure seems to fit pretty cleanly under
'System Information Functions'.

Thanks,

Stephen

---

From: Justin Pryzby <[email-masked]>
Date: Mon, 13 Mar 2023 15:03:50 -0500

> 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
>From cd171da2150e1ee8cfc6ca4bdee0a591df6f566b Mon Sep 17 00:00:00 2001
From: Justin Pryzby <[email-masked]>
Date: Mon, 23 Jan 2023 18:33:51 -0600
Subject: [PATCH v5] add pg_huge_pages_active()

https://www.postgresql.org/message-id/flat/[email-masked]
---
 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(-)

index e5c41cc6c6d..d66f73a494a 100644
+        indicated by <function>pg_huge_pages_active()</function>.
        </para>
 
        <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);

+	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

---

From: Stephen Frost <[email-masked]>
Date: Mon, 13 Mar 2023 21:33:31 +0100

Greetings,

> to the feedback that postgres -C huge_pages_active should return

I really don’t see how that’s at all useful.

> Alvaro was -1 on using a function

I don’t entirely get that argument (select thisfunc(); is much worse than
show thisguc; ..?   Also, the former is easier to use with other functions
and such, as you don’t have to do current_setting(‘thisguc’)…).

Andres is +0 (?)

Kinda felt like this was closer to +0.5 or more, for my part anyway.

Nathan is +1
> I'm -0.5,

Why..?

but I reimplemented it as a function.

Thanks!

  I hope that helps it to
> progress.  Please include a suggestion if there's better place for the

Will try to give it a look tomorrow.

Thanks again!

Stephen

---

From: Kyotaro Horiguchi <[email-masked]>
Date: Tue, 14 Mar 2023 14:02:19 +0900 (JST)

At Mon, 13 Mar 2023 21:33:31 +0100, Stephen Frost <[email-masked]> wrote in 
> Kinda felt like this was closer to +0.5 or more, for my part anyway.

IMHO, it appears that we use GUC "internal" variables to for the
lifespan-long numbers of a postmaster process or an instance.
Examples of such variables includes shared_memory_size and
s_m_s_in_huge_pages, integer_datetimes and data_checksums.  I'm
uncertain about in_hot_standby, as it can change during a postmaster's
lifetime. However, pg_is_in_recovery() provides essentially the same
information.

Regarding huge_page_active, its value remains constant throughout a
postmaster's lifespan. In this regard, GUC may be a better fit for
this information.  The issue with using GUC for this value is that the
postgres command cannot report the final value via the -C option,
which may be the reason for the third alternative "unknown".

I slightly prefer using a function for this, as if GUC is used, it can
only return "unknown" for the command "postgres -C
huge_page_active". However, apart from this advantage, I prefer using
a GUC for this information.

If we implement it as a function, I suggest naming it
"pg_huge_page_is_active" or something similar (the use of "is" is
signinficant here) to follow the naming convention used in
pg_is_in_recovery().

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

---

From: Michael Paquier <[email-masked]>
Date: Mon, 20 Mar 2023 13:54:46 +0900

> a GUC for this information.

The main advantage of a read-only GUC over a function is that users
would not need to start a postmaster to know if huge pages would be
active or not.  This is the main reason why a GUC would be a better
fit, in my opinion, because it makes for a cheaper check, while still
allowing a SQL query to check the value of the GUC.
--
Michael

---

From: Michael Paquier <[email-masked]>
Date: Mon, 20 Mar 2023 14:03:13 +0900

> allowing a SQL query to check the value of the GUC.

[ Should have read more carefully ]

..  Which is something you cannot do with -C because mmap() happens
after the runtime-computed logic for postgres -C.  It does not sound
right to do the mmap() for a GUC check, so indeed a function may be
more adapted rather than move mmap() call a bit earlier in the
postmaster startup sequence.
--
Michael

---

From: Tom Lane <[email-masked]>
Date: Mon, 20 Mar 2023 01:09:09 -0400

> active or not.

I'm confused here, because Horiguchi-san is saying that that
won't work.  I've not checked the code lately, but I think that
"postgres -C var" prints its results before actually attempting
to establish shared memory, so I suspect Horiguchi-san is right.

			regards, tom lane