On Mon, Jun 12, 2023 at 02:37:15PM -0700, Nathan Bossart wrote: > Fair enough. I know I've been waffling in the GUC versus function > discussion, but FWIW v7 of the patch looks reasonable to me.
+ Assert(strcmp("unknown", GetConfigOption("huge_pages_status", false, false)) != 0); Not sure that there is anything to gain with this assertion in CreateSharedMemoryAndSemaphores(), because this is pretty much what check_GUC_init() looks after? @@ -627,6 +627,9 @@ CreateAnonymousSegment(Size *size) } #endif + SetConfigOption("huge_pages_status", ptr == MAP_FAILED ? "off" : "on", + PGC_INTERNAL, PGC_S_DYNAMIC_DEFAULT); The choice of this location is critical, because this is just *after* we've tried huge pages, but *before* doing the fallback mmap() call when the huge page allocation was on "try". I think that this deserves a comment. @@ -327,6 +327,10 @@ retry: Sleep(1000); continue; } + + SetConfigOption("huge_pages_status", (flProtect & SEC_LARGE_PAGES) ? + "on" : "off", PGC_INTERNAL, PGC_S_DYNAMIC_DEFAULT); Hmm. I still think that it is cleaner to move that at the end of PGSharedMemoryCreate() for the WIN32 case. There are also few FATALs in-between, which would make SetConfigOption() useless if there is an in-flight problem. Don't we need to update save_backend_variables() and add an entry in BackendParameters to make other processes launched by EXEC_BACKEND inherit the status value set? -- Michael
signature.asc
Description: PGP signature