public inbox for [email protected]
help / color / mirror / Atom feed[PATCH] Use correct macro to check compressed-ness in amcheck
17+ messages / 7 participants
[nested] [flat]
* [PATCH] Use correct macro to check compressed-ness in amcheck
@ 2022-05-17 07:19 Kyotaro Horiguchi <[email protected]>
0 siblings, 0 replies; 17+ messages in thread
From: Kyotaro Horiguchi @ 2022-05-17 07:19 UTC (permalink / raw)
check_tuple_attribute uses VARATT_IS_COMPRESSED() for
varatt_external. This should be VARATT_EXTERNAL_IS_COMPRESSED()
instead. Remove the following cross check for data size since the
ycondition cannot be true.
---
contrib/amcheck/verify_heapam.c | 10 +---------
1 file changed, 1 insertion(+), 9 deletions(-)
diff --git a/contrib/amcheck/verify_heapam.c b/contrib/amcheck/verify_heapam.c
index c875f3e5a2..e488f5e234 100644
--- a/contrib/amcheck/verify_heapam.c
+++ b/contrib/amcheck/verify_heapam.c
@@ -1385,19 +1385,11 @@ check_tuple_attribute(HeapCheckContext *ctx)
toast_pointer.va_rawsize,
VARLENA_SIZE_LIMIT));
- if (VARATT_IS_COMPRESSED(&toast_pointer))
+ if (VARATT_EXTERNAL_IS_COMPRESSED(toast_pointer))
{
ToastCompressionId cmid;
bool valid = false;
- /* Compression should never expand the attribute */
- if (VARATT_EXTERNAL_GET_EXTSIZE(toast_pointer) > toast_pointer.va_rawsize - VARHDRSZ)
- report_corruption(ctx,
- psprintf("toast value %u external size %u exceeds maximum expected for rawsize %d",
- toast_pointer.va_valueid,
- VARATT_EXTERNAL_GET_EXTSIZE(toast_pointer),
- toast_pointer.va_rawsize));
-
/* Compressed attributes should have a valid compression method */
cmid = TOAST_COMPRESS_METHOD(&toast_pointer);
switch (cmid)
--
2.27.0
----Next_Part(Tue_May_17_16_27_19_2022_200)----
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: Improve logging when using Huge Pages
@ 2023-02-09 09:40 Alvaro Herrera <[email protected]>
2023-02-09 19:29 ` Re: Improve logging when using Huge Pages Nathan Bossart <[email protected]>
0 siblings, 1 reply; 17+ messages in thread
From: Alvaro Herrera @ 2023-02-09 09:40 UTC (permalink / raw)
To: Justin Pryzby <[email protected]>; +Cc: Andres Freund <[email protected]>; Michael Paquier <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; pgsql-hackers; [email protected]; Nathan Bossart <[email protected]>
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)
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: Improve logging when using Huge Pages
2023-02-09 09:40 Re: Improve logging when using Huge Pages Alvaro Herrera <[email protected]>
@ 2023-02-09 19:29 ` Nathan Bossart <[email protected]>
2023-02-13 23:22 ` Re: Improve logging when using Huge Pages Justin Pryzby <[email protected]>
0 siblings, 1 reply; 17+ messages in thread
From: Nathan Bossart @ 2023-02-09 19:29 UTC (permalink / raw)
To: Alvaro Herrera <[email protected]>; +Cc: Justin Pryzby <[email protected]>; Andres Freund <[email protected]>; Michael Paquier <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; pgsql-hackers; [email protected]
On Thu, Feb 09, 2023 at 10:40:13AM +0100, Alvaro Herrera wrote:
> 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.
I think this approach would address my concerns about using a GUC.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: Improve logging when using Huge Pages
2023-02-09 09:40 Re: Improve logging when using Huge Pages Alvaro Herrera <[email protected]>
2023-02-09 19:29 ` Re: Improve logging when using Huge Pages Nathan Bossart <[email protected]>
@ 2023-02-13 23:22 ` Justin Pryzby <[email protected]>
2023-02-14 04:18 ` Re: Improve logging when using Huge Pages Nathan Bossart <[email protected]>
0 siblings, 1 reply; 17+ messages in thread
From: Justin Pryzby @ 2023-02-13 23:22 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; +Cc: Alvaro Herrera <[email protected]>; Andres Freund <[email protected]>; Michael Paquier <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; pgsql-hackers; [email protected]
On Thu, Feb 09, 2023 at 11:29:09AM -0800, Nathan Bossart wrote:
> On Thu, Feb 09, 2023 at 10:40:13AM +0100, Alvaro Herrera wrote:
> > 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.
>
> 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
Attachments:
[text/x-diff] v3-0001-add-GUC-huge_pages_active.patch (7.5K, ../../[email protected]/2-v3-0001-add-GUC-huge_pages_active.patch)
download | inline diff:
From 5ead7ba3711dd7a0bb9ec083ebd60049f50f9907 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <[email protected]>
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/TU4PR8401MB1152EBB0D271F827E2E37A01EECC9@TU4PR8401MB1152.NAMPRD84.PROD.OUTLOOK.COM
---
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
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: Improve logging when using Huge Pages
2023-02-09 09:40 Re: Improve logging when using Huge Pages Alvaro Herrera <[email protected]>
2023-02-09 19:29 ` Re: Improve logging when using Huge Pages Nathan Bossart <[email protected]>
2023-02-13 23:22 ` Re: Improve logging when using Huge Pages Justin Pryzby <[email protected]>
@ 2023-02-14 04:18 ` Nathan Bossart <[email protected]>
2023-02-15 01:32 ` Re: Improve logging when using Huge Pages Justin Pryzby <[email protected]>
0 siblings, 1 reply; 17+ messages in thread
From: Nathan Bossart @ 2023-02-14 04:18 UTC (permalink / raw)
To: Justin Pryzby <[email protected]>; +Cc: Alvaro Herrera <[email protected]>; Andres Freund <[email protected]>; Michael Paquier <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; pgsql-hackers; [email protected]
On Mon, Feb 13, 2023 at 05:22:45PM -0600, Justin Pryzby wrote:
> + Reports whether huge pages are in use by the current process.
> + 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?
> + {
> + {"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
> + },
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.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: Improve logging when using Huge Pages
2023-02-09 09:40 Re: Improve logging when using Huge Pages Alvaro Herrera <[email protected]>
2023-02-09 19:29 ` Re: Improve logging when using Huge Pages Nathan Bossart <[email protected]>
2023-02-13 23:22 ` Re: Improve logging when using Huge Pages Justin Pryzby <[email protected]>
2023-02-14 04:18 ` Re: Improve logging when using Huge Pages Nathan Bossart <[email protected]>
@ 2023-02-15 01:32 ` Justin Pryzby <[email protected]>
2023-02-15 18:13 ` Re: Improve logging when using Huge Pages Nathan Bossart <[email protected]>
0 siblings, 1 reply; 17+ messages in thread
From: Justin Pryzby @ 2023-02-15 01:32 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; +Cc: Alvaro Herrera <[email protected]>; Andres Freund <[email protected]>; Michael Paquier <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; pgsql-hackers; [email protected]
On Mon, Feb 13, 2023 at 08:18:52PM -0800, Nathan Bossart wrote:
> On Mon, Feb 13, 2023 at 05:22:45PM -0600, Justin Pryzby wrote:
> > + Reports whether huge pages are in use by the current process.
> > + See <xref linkend="guc-huge-pages"/> for more information.
>
> nitpick: Should this say "server" instead of "current process"?
It should probably say "instance" :)
> > +static char *huge_pages_active = "unknown"; /* dynamically set */
>
> 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
> > + {
> > + {"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
> > + },
>
> 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}...
--
Justin
Attachments:
[text/x-diff] v4-0001-add-GUC-huge_pages_active.patch (7.5K, ../../[email protected]/2-v4-0001-add-GUC-huge_pages_active.patch)
download | inline diff:
From 7bf67d398902570ffc68a6b78265a0e346503392 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <[email protected]>
Date: Mon, 23 Jan 2023 18:33:51 -0600
Subject: [PATCH v4 1/2] 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 | 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..3ff301edf8a 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>string</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>
+
<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..89ec5c30364 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 ? "false" : "true",
+ 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..fa72b3d1f8f 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) ?
+ "true" : "false", 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
[text/x-diff] v4-0002-f-convert-to-an-enum.txt (7.0K, ../../[email protected]/3-v4-0002-f-convert-to-an-enum.txt)
download | inline diff:
From 96ad5dcd77bb8d2e4c9c3ca06ae4dcf43017defe Mon Sep 17 00:00:00 2001
From: Justin Pryzby <[email protected]>
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(-)
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 3ff301edf8a..1f6f71a2826 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -10681,23 +10681,23 @@ dynamic_library_path = 'C:\tools\postgresql;H:\my_project\lib;$libdir'
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>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 *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;
@@ -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
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: Improve logging when using Huge Pages
2023-02-09 09:40 Re: Improve logging when using Huge Pages Alvaro Herrera <[email protected]>
2023-02-09 19:29 ` Re: Improve logging when using Huge Pages Nathan Bossart <[email protected]>
2023-02-13 23:22 ` Re: Improve logging when using Huge Pages Justin Pryzby <[email protected]>
2023-02-14 04:18 ` Re: Improve logging when using Huge Pages Nathan Bossart <[email protected]>
2023-02-15 01:32 ` Re: Improve logging when using Huge Pages Justin Pryzby <[email protected]>
@ 2023-02-15 18:13 ` Nathan Bossart <[email protected]>
2023-03-08 22:16 ` Re: Improve logging when using Huge Pages Nathan Bossart <[email protected]>
0 siblings, 1 reply; 17+ messages in thread
From: Nathan Bossart @ 2023-02-15 18:13 UTC (permalink / raw)
To: Justin Pryzby <[email protected]>; +Cc: Alvaro Herrera <[email protected]>; Andres Freund <[email protected]>; Michael Paquier <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; pgsql-hackers; [email protected]
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:
>> On Mon, Feb 13, 2023 at 05:22:45PM -0600, Justin Pryzby wrote:
>> 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
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.
>> 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).
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: Improve logging when using Huge Pages
2023-02-09 09:40 Re: Improve logging when using Huge Pages Alvaro Herrera <[email protected]>
2023-02-09 19:29 ` Re: Improve logging when using Huge Pages Nathan Bossart <[email protected]>
2023-02-13 23:22 ` Re: Improve logging when using Huge Pages Justin Pryzby <[email protected]>
2023-02-14 04:18 ` Re: Improve logging when using Huge Pages Nathan Bossart <[email protected]>
2023-02-15 01:32 ` Re: Improve logging when using Huge Pages Justin Pryzby <[email protected]>
2023-02-15 18:13 ` Re: Improve logging when using Huge Pages Nathan Bossart <[email protected]>
@ 2023-03-08 22:16 ` Nathan Bossart <[email protected]>
2023-03-09 14:34 ` Re: Improve logging when using Huge Pages Stephen Frost <[email protected]>
0 siblings, 1 reply; 17+ messages in thread
From: Nathan Bossart @ 2023-03-08 22:16 UTC (permalink / raw)
To: Justin Pryzby <[email protected]>; +Cc: Alvaro Herrera <[email protected]>; Andres Freund <[email protected]>; Michael Paquier <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; pgsql-hackers; [email protected]
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.
+ Reports whether huge pages are in use by the current instance.
+ See <xref linkend="guc-huge-pages"/> for more information.
I still think we should say "server" in place of "current instance" here.
+ {"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
+ },
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.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: Improve logging when using Huge Pages
2023-02-09 09:40 Re: Improve logging when using Huge Pages Alvaro Herrera <[email protected]>
2023-02-09 19:29 ` Re: Improve logging when using Huge Pages Nathan Bossart <[email protected]>
2023-02-13 23:22 ` Re: Improve logging when using Huge Pages Justin Pryzby <[email protected]>
2023-02-14 04:18 ` Re: Improve logging when using Huge Pages Nathan Bossart <[email protected]>
2023-02-15 01:32 ` Re: Improve logging when using Huge Pages Justin Pryzby <[email protected]>
2023-02-15 18:13 ` Re: Improve logging when using Huge Pages Nathan Bossart <[email protected]>
2023-03-08 22:16 ` Re: Improve logging when using Huge Pages Nathan Bossart <[email protected]>
@ 2023-03-09 14:34 ` Stephen Frost <[email protected]>
2023-03-09 16:38 ` Re: Improve logging when using Huge Pages Justin Pryzby <[email protected]>
0 siblings, 1 reply; 17+ messages in thread
From: Stephen Frost @ 2023-03-09 14:34 UTC (permalink / raw)
To: Nathan Bossart <[email protected]>; +Cc: Justin Pryzby <[email protected]>; Alvaro Herrera <[email protected]>; Andres Freund <[email protected]>; Michael Paquier <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; pgsql-hackers; [email protected]
Greetings,
* Nathan Bossart ([email protected]) 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.
> + Reports whether huge pages are in use by the current instance.
> + See <xref linkend="guc-huge-pages"/> for more information.
>
> 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).
> + {"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
> + },
>
> 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.
... 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.
> 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.
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
Attachments:
[application/pgp-signature] signature.asc (833B, ../../ZAnuYi%[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: Improve logging when using Huge Pages
2023-02-09 09:40 Re: Improve logging when using Huge Pages Alvaro Herrera <[email protected]>
2023-02-09 19:29 ` Re: Improve logging when using Huge Pages Nathan Bossart <[email protected]>
2023-02-13 23:22 ` Re: Improve logging when using Huge Pages Justin Pryzby <[email protected]>
2023-02-14 04:18 ` Re: Improve logging when using Huge Pages Nathan Bossart <[email protected]>
2023-02-15 01:32 ` Re: Improve logging when using Huge Pages Justin Pryzby <[email protected]>
2023-02-15 18:13 ` Re: Improve logging when using Huge Pages Nathan Bossart <[email protected]>
2023-03-08 22:16 ` Re: Improve logging when using Huge Pages Nathan Bossart <[email protected]>
2023-03-09 14:34 ` Re: Improve logging when using Huge Pages Stephen Frost <[email protected]>
@ 2023-03-09 16:38 ` Justin Pryzby <[email protected]>
2023-03-09 20:02 ` Re: Improve logging when using Huge Pages Stephen Frost <[email protected]>
0 siblings, 1 reply; 17+ messages in thread
From: Justin Pryzby @ 2023-03-09 16:38 UTC (permalink / raw)
To: Stephen Frost <[email protected]>; +Cc: Nathan Bossart <[email protected]>; Alvaro Herrera <[email protected]>; Andres Freund <[email protected]>; Michael Paquier <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; pgsql-hackers; [email protected]
On Thu, Mar 09, 2023 at 09:34:10AM -0500, Stephen Frost wrote:
> Greetings,
>
> * Nathan Bossart ([email protected]) 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 ?
> > + Reports whether huge pages are in use by the current instance.
> > + See <xref linkend="guc-huge-pages"/> for more information.
> >
> > 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).
I called it "instance" since the GUC has no meaning when it's not
running. I'm fine to rename it to "running server".
> > + {"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
> > + },
> >
> > 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.
>
> ... or we could consider adjusting things to actually try the mmap() and
> 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.
> > 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.
>
> 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.
We talked about making it a function ~6 weeks ago.
Is there an agreement to use a function, instead ?
--
Justin
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: Improve logging when using Huge Pages
2023-02-09 09:40 Re: Improve logging when using Huge Pages Alvaro Herrera <[email protected]>
2023-02-09 19:29 ` Re: Improve logging when using Huge Pages Nathan Bossart <[email protected]>
2023-02-13 23:22 ` Re: Improve logging when using Huge Pages Justin Pryzby <[email protected]>
2023-02-14 04:18 ` Re: Improve logging when using Huge Pages Nathan Bossart <[email protected]>
2023-02-15 01:32 ` Re: Improve logging when using Huge Pages Justin Pryzby <[email protected]>
2023-02-15 18:13 ` Re: Improve logging when using Huge Pages Nathan Bossart <[email protected]>
2023-03-08 22:16 ` Re: Improve logging when using Huge Pages Nathan Bossart <[email protected]>
2023-03-09 14:34 ` Re: Improve logging when using Huge Pages Stephen Frost <[email protected]>
2023-03-09 16:38 ` Re: Improve logging when using Huge Pages Justin Pryzby <[email protected]>
@ 2023-03-09 20:02 ` Stephen Frost <[email protected]>
2023-03-13 20:03 ` Re: Improve logging when using Huge Pages Justin Pryzby <[email protected]>
0 siblings, 1 reply; 17+ messages in thread
From: Stephen Frost @ 2023-03-09 20:02 UTC (permalink / raw)
To: Justin Pryzby <[email protected]>; +Cc: Nathan Bossart <[email protected]>; Alvaro Herrera <[email protected]>; Andres Freund <[email protected]>; Michael Paquier <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; pgsql-hackers; [email protected]
Greetings,
* Justin Pryzby ([email protected]) wrote:
> On Thu, Mar 09, 2023 at 09:34:10AM -0500, Stephen Frost wrote:
> > * Nathan Bossart ([email protected]) 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.
Sure, anything can be cast to text but when there's a data type that
fits better, that's almost uniformly better to use.
> > > + Reports whether huge pages are in use by the current instance.
> > > + See <xref linkend="guc-huge-pages"/> for more information.
> > >
> > > 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).
>
> I called it "instance" since the GUC has no meaning when it's not
> running. I'm fine to rename it to "running server".
Great, I do think that would match better with the rest of the
documentation.
> > > + {"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
> > > + },
> > >
> > > 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.
> >
> > ... or we could consider adjusting things to actually try the mmap() and
> > 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.
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.
> > > 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.
> >
> > 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.
>
> 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
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: Improve logging when using Huge Pages
2023-02-09 09:40 Re: Improve logging when using Huge Pages Alvaro Herrera <[email protected]>
2023-02-09 19:29 ` Re: Improve logging when using Huge Pages Nathan Bossart <[email protected]>
2023-02-13 23:22 ` Re: Improve logging when using Huge Pages Justin Pryzby <[email protected]>
2023-02-14 04:18 ` Re: Improve logging when using Huge Pages Nathan Bossart <[email protected]>
2023-02-15 01:32 ` Re: Improve logging when using Huge Pages Justin Pryzby <[email protected]>
2023-02-15 18:13 ` Re: Improve logging when using Huge Pages Nathan Bossart <[email protected]>
2023-03-08 22:16 ` Re: Improve logging when using Huge Pages Nathan Bossart <[email protected]>
2023-03-09 14:34 ` Re: Improve logging when using Huge Pages Stephen Frost <[email protected]>
2023-03-09 16:38 ` Re: Improve logging when using Huge Pages Justin Pryzby <[email protected]>
2023-03-09 20:02 ` Re: Improve logging when using Huge Pages Stephen Frost <[email protected]>
@ 2023-03-13 20:03 ` Justin Pryzby <[email protected]>
2023-03-13 20:33 ` Re: Improve logging when using Huge Pages Stephen Frost <[email protected]>
0 siblings, 1 reply; 17+ messages in thread
From: Justin Pryzby @ 2023-03-13 20:03 UTC (permalink / raw)
To: Stephen Frost <[email protected]>; +Cc: Nathan Bossart <[email protected]>; Alvaro Herrera <[email protected]>; Andres Freund <[email protected]>; Michael Paquier <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; pgsql-hackers; [email protected]
On Thu, Mar 09, 2023 at 03:02:29PM -0500, Stephen Frost wrote:
> * Justin Pryzby ([email protected]) wrote:
> > On Thu, Mar 09, 2023 at 09:34:10AM -0500, Stephen Frost wrote:
> > > * Nathan Bossart ([email protected]) 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
Attachments:
[text/x-diff] v5-0001-add-pg_huge_pages_active.patch (8.6K, ../../ZA+Bpk%[email protected]/2-v5-0001-add-pg_huge_pages_active.patch)
download | inline diff:
From cd171da2150e1ee8cfc6ca4bdee0a591df6f566b Mon Sep 17 00:00:00 2001
From: Justin Pryzby <[email protected]>
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
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: Improve logging when using Huge Pages
2023-02-09 09:40 Re: Improve logging when using Huge Pages Alvaro Herrera <[email protected]>
2023-02-09 19:29 ` Re: Improve logging when using Huge Pages Nathan Bossart <[email protected]>
2023-02-13 23:22 ` Re: Improve logging when using Huge Pages Justin Pryzby <[email protected]>
2023-02-14 04:18 ` Re: Improve logging when using Huge Pages Nathan Bossart <[email protected]>
2023-02-15 01:32 ` Re: Improve logging when using Huge Pages Justin Pryzby <[email protected]>
2023-02-15 18:13 ` Re: Improve logging when using Huge Pages Nathan Bossart <[email protected]>
2023-03-08 22:16 ` Re: Improve logging when using Huge Pages Nathan Bossart <[email protected]>
2023-03-09 14:34 ` Re: Improve logging when using Huge Pages Stephen Frost <[email protected]>
2023-03-09 16:38 ` Re: Improve logging when using Huge Pages Justin Pryzby <[email protected]>
2023-03-09 20:02 ` Re: Improve logging when using Huge Pages Stephen Frost <[email protected]>
2023-03-13 20:03 ` Re: Improve logging when using Huge Pages Justin Pryzby <[email protected]>
@ 2023-03-13 20:33 ` Stephen Frost <[email protected]>
2023-03-14 05:02 ` Re: Improve logging when using Huge Pages Kyotaro Horiguchi <[email protected]>
0 siblings, 1 reply; 17+ messages in thread
From: Stephen Frost @ 2023-03-13 20:33 UTC (permalink / raw)
To: Justin Pryzby <[email protected]>; +Cc: Alvaro Herrera <[email protected]>; Andres Freund <[email protected]>; Kyotaro Horiguchi <[email protected]>; Michael Paquier <[email protected]>; Nathan Bossart <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; pgsql-hackers; [email protected]; [email protected]; [email protected]; [email protected]
Greetings,
On Mon, Mar 13, 2023 at 21:03 Justin Pryzby <[email protected]> wrote:
> On Thu, Mar 09, 2023 at 03:02:29PM -0500, Stephen Frost wrote:
> > * Justin Pryzby ([email protected]) wrote:
> > > On Thu, Mar 09, 2023 at 09:34:10AM -0500, Stephen Frost wrote:
> > > > * Nathan Bossart ([email protected]) 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".
I really don’t see how that’s at all useful.
> > Is there an agreement to use a function, instead ?
>
> 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
> Stephen 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
> function or global var.
Will try to give it a look tomorrow.
Thanks again!
Stephen
>
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: Improve logging when using Huge Pages
2023-02-09 09:40 Re: Improve logging when using Huge Pages Alvaro Herrera <[email protected]>
2023-02-09 19:29 ` Re: Improve logging when using Huge Pages Nathan Bossart <[email protected]>
2023-02-13 23:22 ` Re: Improve logging when using Huge Pages Justin Pryzby <[email protected]>
2023-02-14 04:18 ` Re: Improve logging when using Huge Pages Nathan Bossart <[email protected]>
2023-02-15 01:32 ` Re: Improve logging when using Huge Pages Justin Pryzby <[email protected]>
2023-02-15 18:13 ` Re: Improve logging when using Huge Pages Nathan Bossart <[email protected]>
2023-03-08 22:16 ` Re: Improve logging when using Huge Pages Nathan Bossart <[email protected]>
2023-03-09 14:34 ` Re: Improve logging when using Huge Pages Stephen Frost <[email protected]>
2023-03-09 16:38 ` Re: Improve logging when using Huge Pages Justin Pryzby <[email protected]>
2023-03-09 20:02 ` Re: Improve logging when using Huge Pages Stephen Frost <[email protected]>
2023-03-13 20:03 ` Re: Improve logging when using Huge Pages Justin Pryzby <[email protected]>
2023-03-13 20:33 ` Re: Improve logging when using Huge Pages Stephen Frost <[email protected]>
@ 2023-03-14 05:02 ` Kyotaro Horiguchi <[email protected]>
2023-03-20 04:54 ` Re: Improve logging when using Huge Pages Michael Paquier <[email protected]>
0 siblings, 1 reply; 17+ messages in thread
From: Kyotaro Horiguchi @ 2023-03-14 05:02 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; pgsql-hackers; [email protected]; [email protected]; [email protected]; [email protected]
At Mon, 13 Mar 2023 21:33:31 +0100, Stephen Frost <[email protected]> wrote in
> > On Thu, Mar 09, 2023 at 03:02:29PM -0500, Stephen Frost wrote:
> > > * Justin Pryzby ([email protected]) wrote:
> > > Is there an agreement to use a function, instead ?
> >
> > 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
> > Stephen is +1
> >
> > I'm -0.5,
>
>
> Why..?
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
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: Improve logging when using Huge Pages
2023-02-09 09:40 Re: Improve logging when using Huge Pages Alvaro Herrera <[email protected]>
2023-02-09 19:29 ` Re: Improve logging when using Huge Pages Nathan Bossart <[email protected]>
2023-02-13 23:22 ` Re: Improve logging when using Huge Pages Justin Pryzby <[email protected]>
2023-02-14 04:18 ` Re: Improve logging when using Huge Pages Nathan Bossart <[email protected]>
2023-02-15 01:32 ` Re: Improve logging when using Huge Pages Justin Pryzby <[email protected]>
2023-02-15 18:13 ` Re: Improve logging when using Huge Pages Nathan Bossart <[email protected]>
2023-03-08 22:16 ` Re: Improve logging when using Huge Pages Nathan Bossart <[email protected]>
2023-03-09 14:34 ` Re: Improve logging when using Huge Pages Stephen Frost <[email protected]>
2023-03-09 16:38 ` Re: Improve logging when using Huge Pages Justin Pryzby <[email protected]>
2023-03-09 20:02 ` Re: Improve logging when using Huge Pages Stephen Frost <[email protected]>
2023-03-13 20:03 ` Re: Improve logging when using Huge Pages Justin Pryzby <[email protected]>
2023-03-13 20:33 ` Re: Improve logging when using Huge Pages Stephen Frost <[email protected]>
2023-03-14 05:02 ` Re: Improve logging when using Huge Pages Kyotaro Horiguchi <[email protected]>
@ 2023-03-20 04:54 ` Michael Paquier <[email protected]>
2023-03-20 05:03 ` Re: Improve logging when using Huge Pages Michael Paquier <[email protected]>
2023-03-20 05:09 ` Re: Improve logging when using Huge Pages Tom Lane <[email protected]>
0 siblings, 2 replies; 17+ messages in thread
From: Michael Paquier @ 2023-03-20 04:54 UTC (permalink / raw)
To: Kyotaro Horiguchi <[email protected]>; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; pgsql-hackers; [email protected]; [email protected]; [email protected]; [email protected]
On Tue, Mar 14, 2023 at 02:02:19PM +0900, Kyotaro Horiguchi wrote:
> 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.
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
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: Improve logging when using Huge Pages
2023-02-09 09:40 Re: Improve logging when using Huge Pages Alvaro Herrera <[email protected]>
2023-02-09 19:29 ` Re: Improve logging when using Huge Pages Nathan Bossart <[email protected]>
2023-02-13 23:22 ` Re: Improve logging when using Huge Pages Justin Pryzby <[email protected]>
2023-02-14 04:18 ` Re: Improve logging when using Huge Pages Nathan Bossart <[email protected]>
2023-02-15 01:32 ` Re: Improve logging when using Huge Pages Justin Pryzby <[email protected]>
2023-02-15 18:13 ` Re: Improve logging when using Huge Pages Nathan Bossart <[email protected]>
2023-03-08 22:16 ` Re: Improve logging when using Huge Pages Nathan Bossart <[email protected]>
2023-03-09 14:34 ` Re: Improve logging when using Huge Pages Stephen Frost <[email protected]>
2023-03-09 16:38 ` Re: Improve logging when using Huge Pages Justin Pryzby <[email protected]>
2023-03-09 20:02 ` Re: Improve logging when using Huge Pages Stephen Frost <[email protected]>
2023-03-13 20:03 ` Re: Improve logging when using Huge Pages Justin Pryzby <[email protected]>
2023-03-13 20:33 ` Re: Improve logging when using Huge Pages Stephen Frost <[email protected]>
2023-03-14 05:02 ` Re: Improve logging when using Huge Pages Kyotaro Horiguchi <[email protected]>
2023-03-20 04:54 ` Re: Improve logging when using Huge Pages Michael Paquier <[email protected]>
@ 2023-03-20 05:03 ` Michael Paquier <[email protected]>
1 sibling, 0 replies; 17+ messages in thread
From: Michael Paquier @ 2023-03-20 05:03 UTC (permalink / raw)
To: Kyotaro Horiguchi <[email protected]>; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; pgsql-hackers; [email protected]; [email protected]; [email protected]; [email protected]
On Mon, Mar 20, 2023 at 01:54:46PM +0900, Michael Paquier wrote:
> 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.
[ 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
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: Improve logging when using Huge Pages
2023-02-09 09:40 Re: Improve logging when using Huge Pages Alvaro Herrera <[email protected]>
2023-02-09 19:29 ` Re: Improve logging when using Huge Pages Nathan Bossart <[email protected]>
2023-02-13 23:22 ` Re: Improve logging when using Huge Pages Justin Pryzby <[email protected]>
2023-02-14 04:18 ` Re: Improve logging when using Huge Pages Nathan Bossart <[email protected]>
2023-02-15 01:32 ` Re: Improve logging when using Huge Pages Justin Pryzby <[email protected]>
2023-02-15 18:13 ` Re: Improve logging when using Huge Pages Nathan Bossart <[email protected]>
2023-03-08 22:16 ` Re: Improve logging when using Huge Pages Nathan Bossart <[email protected]>
2023-03-09 14:34 ` Re: Improve logging when using Huge Pages Stephen Frost <[email protected]>
2023-03-09 16:38 ` Re: Improve logging when using Huge Pages Justin Pryzby <[email protected]>
2023-03-09 20:02 ` Re: Improve logging when using Huge Pages Stephen Frost <[email protected]>
2023-03-13 20:03 ` Re: Improve logging when using Huge Pages Justin Pryzby <[email protected]>
2023-03-13 20:33 ` Re: Improve logging when using Huge Pages Stephen Frost <[email protected]>
2023-03-14 05:02 ` Re: Improve logging when using Huge Pages Kyotaro Horiguchi <[email protected]>
2023-03-20 04:54 ` Re: Improve logging when using Huge Pages Michael Paquier <[email protected]>
@ 2023-03-20 05:09 ` Tom Lane <[email protected]>
1 sibling, 0 replies; 17+ messages in thread
From: Tom Lane @ 2023-03-20 05:09 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Kyotaro Horiguchi <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; pgsql-hackers; [email protected]; [email protected]; [email protected]
Michael Paquier <[email protected]> writes:
> On Tue, Mar 14, 2023 at 02:02:19PM +0900, Kyotaro Horiguchi wrote:
>> 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.
> 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.
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
^ permalink raw reply [nested|flat] 17+ messages in thread
end of thread, other threads:[~2023-03-20 05:09 UTC | newest]
Thread overview: 17+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2022-05-17 07:19 [PATCH] Use correct macro to check compressed-ness in amcheck Kyotaro Horiguchi <[email protected]>
2023-02-09 09:40 Re: Improve logging when using Huge Pages Alvaro Herrera <[email protected]>
2023-02-09 19:29 ` Re: Improve logging when using Huge Pages Nathan Bossart <[email protected]>
2023-02-13 23:22 ` Re: Improve logging when using Huge Pages Justin Pryzby <[email protected]>
2023-02-14 04:18 ` Re: Improve logging when using Huge Pages Nathan Bossart <[email protected]>
2023-02-15 01:32 ` Re: Improve logging when using Huge Pages Justin Pryzby <[email protected]>
2023-02-15 18:13 ` Re: Improve logging when using Huge Pages Nathan Bossart <[email protected]>
2023-03-08 22:16 ` Re: Improve logging when using Huge Pages Nathan Bossart <[email protected]>
2023-03-09 14:34 ` Re: Improve logging when using Huge Pages Stephen Frost <[email protected]>
2023-03-09 16:38 ` Re: Improve logging when using Huge Pages Justin Pryzby <[email protected]>
2023-03-09 20:02 ` Re: Improve logging when using Huge Pages Stephen Frost <[email protected]>
2023-03-13 20:03 ` Re: Improve logging when using Huge Pages Justin Pryzby <[email protected]>
2023-03-13 20:33 ` Re: Improve logging when using Huge Pages Stephen Frost <[email protected]>
2023-03-14 05:02 ` Re: Improve logging when using Huge Pages Kyotaro Horiguchi <[email protected]>
2023-03-20 04:54 ` Re: Improve logging when using Huge Pages Michael Paquier <[email protected]>
2023-03-20 05:03 ` Re: Improve logging when using Huge Pages Michael Paquier <[email protected]>
2023-03-20 05:09 ` Re: Improve logging when using Huge Pages Tom Lane <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox