On Tue, Mar 28, 2023 at 09:35:30AM +0900, Michael Paquier wrote: > On Thu, Mar 23, 2023 at 08:50:50PM -0500, Justin Pryzby wrote: > > 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?
Yes. I hijacked a tap test to first run "show huge_pages_active" and then also caused it to fail, such that I could check its output. https://cirrus-ci.com/task/6309510885670912 https://cirrus-ci.com/task/6613427737591808 > > 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. Arg, no - it wasn't working. I added an assert to check that a running server won't output "unknown". > +++ 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? Maybe - I don't know. > + <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. > + See <xref linkend="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. I'm not sure I agree. It can be confusing (even harmful) to overspecify the documentation. I used the word "instance" specifically to refer to a running server. Anyone querying the status of huge pages is going to need to understand that it doesn't make sense to ask about the memory state of a server that's not running. Actually, I'm having trouble imagining that anybody would ever run postgres -C huge_page_status except if they wondered whether it might misbehave. If what I've written is inadequate, could you propose something ? -- Justin PS. I hadn't updated this thread because my last message from you (on another thread) indicated that you'd gotten busy. I'm hoping you'll respond to these other inquiries when you're able. https://www.postgresql.org/message-id/ZCUZLiCeXq4Im7OG%40telsasoft.com https://www.postgresql.org/message-id/ZCUKL22GutwGrrZk%40telsasoft.com
>From 078724d0ec411de1c52cb9a43a3a29c644a8d19a Mon Sep 17 00:00:00 2001 From: Justin Pryzby <pryz...@telsasoft.com> Date: Mon, 23 Jan 2023 18:33:51 -0600 Subject: [PATCH v7] 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/tu4pr8401mb1152ebb0d271f827e2e37a01ee...@tu4pr8401mb1152.namprd84.prod.outlook.com --- doc/src/sgml/config.sgml | 21 ++++++++++++++++++++- src/backend/port/sysv_shmem.c | 7 +++++++ src/backend/port/win32_shmem.c | 4 ++++ src/backend/storage/ipc/ipci.c | 2 ++ src/backend/utils/misc/guc_tables.c | 20 ++++++++++++++++++++ src/include/storage/pg_shmem.h | 5 +++-- 6 files changed, 56 insertions(+), 3 deletions(-) diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index 096be7cb8cc..de74d3d1a81 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -1728,7 +1728,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> @@ -10710,6 +10711,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..a958f75f7a4 100644 --- a/src/backend/port/sysv_shmem.c +++ b/src/backend/port/sysv_shmem.c @@ -627,6 +627,9 @@ CreateAnonymousSegment(Size *size) } #endif + 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,7 +740,11 @@ PGSharedMemoryCreate(Size size, sysvsize = sizeof(PGShmemHeader); } else + { sysvsize = size; + SetConfigOption("huge_pages_status", "off", + PGC_INTERNAL, PGC_S_DYNAMIC_DEFAULT); + } /* * Loop till we find a free IPC key. Trust CreateDataDirLockFile() to diff --git a/src/backend/port/win32_shmem.c b/src/backend/port/win32_shmem.c index 62e08074770..199f2e23a13 100644 --- a/src/backend/port/win32_shmem.c +++ 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); + break; } diff --git a/src/backend/storage/ipc/ipci.c b/src/backend/storage/ipc/ipci.c index 8f1ded7338f..f760675691a 100644 --- a/src/backend/storage/ipc/ipci.c +++ b/src/backend/storage/ipc/ipci.c @@ -190,6 +190,8 @@ CreateSharedMemoryAndSemaphores(void) */ seghdr = PGSharedMemoryCreate(size, &shim); + 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 0a399afa8ca..a6b7fd28e68 100644 --- a/src/backend/utils/misc/guc_tables.c +++ b/src/backend/utils/misc/guc_tables.c @@ -366,6 +366,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}, @@ -554,6 +561,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 @@ -4889,6 +4898,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
