On Thu, Mar 23, 2023 at 08:50:50PM -0500, Justin Pryzby wrote: > I'm sure it's possible, but it's also not worth writing a special > implementation just to handle huge_pages_active, which is better written > in 30 lines than in 300 lines. > > If we needed to avoid using a GUC, maybe it'd work to add > huge_pages_active to PGShmemHeader. But "avoid using gucs at all costs" > isn't the goal, and using a GUC parallels all the similar, and related > things without needing to allocate extra bits of shared_memory.
Yeah, I kind of agree that the complications are not appealing compared to the use case. FWIW, I am fine with just using "unknown" even under -C because we don't know the status without the mmpa() call. And we don't want to assign what would be potentially a bunch of memory when running that. > This goes back to using a GUC, and: > > - removes GUC_RUNTIME_COMPUTED, since that causes a useless error when > using -C if the server is running, rather than successfully printing > "unknown". > - I also renamed it from huge_pages_active = {true,false,unknown} to > huge_pages_STATUS = {on,off,unknown}. This parallels huge_pages, > which is documented to accept values on/off/try. Also, true/false > isn't how bools are displayed. > - I realized that the rename suggested implementing it as an enum GUC, > and re-using the existing HUGE_PAGES_{ON,OFF} values (and adding an > "UNKNOWN"). Maybe this also avoids Stephen's earlier objection to > using a string ? huge_pages_status is OK here, as is reusing the enum struct to avoid an unnecessary duplication. > I mis-used cirrusci to check that the GUC does work correctly under > windows. 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? > 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. +++ 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? + <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. -- Michael
signature.asc
Description: PGP signature