Re: Improve logging when using Huge Pages

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