Re: Improve logging when using Huge Pages

Greetings,

* Justin Pryzby (pryzby(at)telsasoft(dot)com) wrote:
> On Thu, Mar 09, 2023 at 09:34:10AM -0500, Stephen Frost wrote:
> > * Nathan Bossart (nathandbossart(at)gmail(dot)com) wrote:
> > > On Wed, Feb 15, 2023 at 10:13:17AM -0800, Nathan Bossart wrote:
> > > > On Tue, Feb 14, 2023 at 07:32:56PM -0600, Justin Pryzby wrote:
> > > >> On Mon, Feb 13, 2023 at 08:18:52PM -0800, Nathan Bossart wrote:
> > > >>> 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}...
> > > >
> > > > I think unknown/true/false is fine. I'm okay with using a string if no one
> > > > else thinks it should be an enum (or a bool).
> > >
> > > There's been no response for this, so I guess we can proceed with a string
> > > GUC.
> >
> > Just happened to see this and I'm not really a fan of this being a
> > string when it's pretty clear that's not what it actually is.
>
> I don't understand what you mean by that.
> Why do you say it isn't a string ?

Because it's clearly a yes/no, either the server is currently running
with huge pages, or it isn't. That's the definition of a boolean.
Sure, anything can be cast to text but when there's a data type that
fits better, that's almost uniformly better to use.

> > > + Reports whether huge pages are in use by the current instance.
> > > + See <xref linkend="guc-huge-pages"/> for more information.
> > >
> > > I still think we should say "server" in place of "current instance" here.
> >
> > We certainly use 'server' a lot more in config.sgml than we do
> > 'instance'. "currently running server" might be closer to how we
> > describe a running PG system in other parts (we talk about "currently
> > running server processes", "while the server is running", "When running
> > a standby server", "when the server is running"; "instance" is used much
> > less and seems to more typically refer to 'state of files on disk' in my
> > reading vs. 'actively running process' though there's some of each).
>
> I called it "instance" since the GUC has no meaning when it's not
> running. I'm fine to rename it to "running server".

Great, I do think that would match better with the rest of the
documentation.

> > > + {"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
> > > + },
> > >
> > > I don't think we need to use GUC_RUNTIME_COMPUTED. 'postgres -C' seems to
> > > always report "unknown" for this GUC, so all this would do is cause that
> > > command to error unnecessarily when the server is running.
> >
> > ... or we could consider adjusting things to actually try the mmap() and
> > find out if we'd end up with huge pages or not.
>
> That seems like a bad idea, since it might work one moment and fail one
> moment later. If we could tell in advance whether it was going to work,
> we wouldn't be here, and probably also wouldn't have invented
> huge_pages=true.

Sure it might ... but I tend to disagree that it's actually all that
likely for it to end up being as inconsistent as that and it'd be nice
to be able to see if the server will end up successfully starting (for
this part, at least), or not, when configured with huge pages set to on,
or if starting with 'try' is likely to result in it actually using huge
pages, or not.

> > > It might be worth documenting exactly what "unknown" means. IIUC you'll
> > > only ever see "on" or "off" via SHOW or pg_settings, which doesn't seem
> > > tremendously obvious.
> >
> > If we could get rid of that case and just make this a boolean, that
> > seems like it'd really be the best answer.
> >
> > To that end- perhaps this isn't appropriate as a GUC at all? Why not
> > have this just be a system information function? Functionally it really
> > provides the same info- if the server is running then you get back
> > either true or false, if it's not then you can't call it but that's
> > hardly different from an unknown or error result.
>
> We talked about making it a function ~6 weeks ago.

Oh, good, glad I'm not the only one to have thought of that.

> Is there an agreement to use a function, instead ?

Looking back at the arguments for having it be a GUC ... I just don't
really see any of them as very strong. Not that I feel super strongly
about it being a function either, but it's certainly not a configuration
variable and it also isn't really available with postgres -C (and
therefore doesn't actually go along with how the *size GUCs work). It's
literally information about the running system that the user might be
curious about ... and that sure seems to fit pretty cleanly under
'System Information Functions'.

Thanks,

Stephen