Re: Improve logging when using Huge Pages

On Thu, Mar 09, 2023 at 09:34:10AM -0500, Stephen Frost wrote:
> Greetings,
> 
> * Nathan Bossart (nathandbossart@gmail.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 ?

> > +        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".

> > +		{"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.

> > 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.

Is there an agreement to use a function, instead ?

-- 
Justin