On Tue, Sep 07, 2021 at 05:08:43PM +0000, Bossart, Nathan wrote:
> On 9/6/21, 9:00 PM, "Michael Paquier" <michael@paquier.xyz> wrote:
>> + sprintf(buf, "%lu MB", size_mb);
>> + SetConfigOption("shared_memory_size", buf, PGC_INTERNAL, PGC_S_OVERRIDE);
>> One small-ish comment about 0002: there is no need to add the unit
>> into the buffer set as GUC_UNIT_MB would take care of that. The patch
>> looks fine.
>
> I fixed this in v7.
Switched the variable name to shared_memory_size_mb for easier
grepping, moved it to a more correct location with the other read-only
GUCS, and applied 0002. Well, 0001 here.
>> +#ifndef WIN32
>> +#include <sys/mman.h>
>> +#endif
>> So, this is needed in ipci.c to check for MAP_HUGETLB. I am not much
>> a fan of moving around platform-specific checks when these have
>> remained local to each shmem implementation. Could it be cleaner to
>> add GetHugePageSize() to win32_shmem.c and make it always declared in
>> the SysV implementation?
>
> I don't know if it's really all that much cleaner, but I did it this
> way in v7. IMO it's a little weird that GetHugePageSize() doesn't
> return the value from GetLargePageMinimum(), but that's what we'd need
> to do to avoid setting huge_pages_required for Windows without any
> platform-specific checks.
Thanks. Keeping MAP_HUGETLB within the SysV portions is an
improvement IMO. That's subject to one's taste, perhaps.
After sleeping on it, I'd be fine to live with the logic based on the
new GUC flag called GUC_RUNTIME_COMPUTED to control if a parameter can
be looked at either an earlier or a later stage of the startup
sequences, with the later stage meaning that such parameters cannot be
checked if a server is running. This case was originally broken
anyway, so it does not make it worse, just better.
+ This can be used on a running server for most parameters. However,
+ the server must be shut down for some runtime-computed parameters
+ (e.g., <xref linkend="guc-huge-pages-required"/>).
Perhaps we should add a couple of extra parameters here, like
shared_memory_size, and perhaps wal_segment_size? The list does not
have to be complete, just meaningful enough.
--
Michael