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