Re: System views for versions reporting

On Fri, 2025-11-21 at 15:21 +0100, Dmitry Dolgov wrote:
> > > + ctl.hcxt = CurrentMemoryContext;
> >
> > That hashtable should stick around for the entire life time of the backend.
> > Wouldn't it be better to explicitly name the correct memory context, rather
> > that to rely on being called with "CurrentMemoryContext" set correctly?
>
> Looking at this closely, I think it's not even necessary to specify a
> memory context here -- in this case hash_create will create a private
> memory context for this hash table from TopMemoryContext.

That is fine. I just think that using CurrentMemoryContext a) opens a window
for mistakes if you call the function from the wrong place and b) makes it
less clear to the reader where the hash table will be created.

> > > + if (found)
> > > + elog(ERROR, "duplicated system version");
> >
> > Is that a problem? Why throw an error?
>
> It was originally a problem for 0004, and propagated here as well. But I
> still see some value in it, because I can't imagine why registering the
> same version twice might be a reasonable use cases. Do you have some
> examples for such cases in mind?

No, I don't think that there are any use cases. But registering the same
version twice will just overwrite the old value, which is redundant, but no
problem. It would be bad coding though, so perhaps an Assert() would be the
better choice. That would avoid the (tiny) overhead of the check in
production builds.

> > > --- a/src/test/regress/sql/sysviews.sql
> > > +++ b/src/test/regress/sql/sysviews.sql
> > > +
> > > +-- 5 core versions should be present: architecture, ICU, core, compiler and
> > > +-- glibc. If built with JIT support, one more record will be displayed
> > > +-- containing LLVM version.
> > > +select count(*) >= 5 as ok FROM pg_system_versions;
> >
> > If you really think that a regression test for selecting from the view
> > is useful, use "SELECT EXISTS (TABLE pg_system_versions)".
> > Are you sure that the regression tests will always run with ICU enabled?
>
> Not with ICU, but at least the core and compiler version are always
> going to be present, hence the check for number of records. Any
> alternative suggestions for better testing coverage?

I made my suggestion in the paragraph above. I think it is sufficient to check
that you can select from the view at all, never mind how many columns there are.

> > > --- a/src/backend/jit/jit.c
> > > +++ b/src/backend/jit/jit.c
> > > +
> > > +/*
> > > + * Return JIT provider's version string for troubleshooting purposes.
> > > + */
> > > +const char *
> > > +jit_get_version(bool *available)
> > > +{
> > > + if (provider_init())
> > > + return provider.get_version(available);
> > > +
> > > + *available = false;
> > > + return "";
> > > +}
> >
> > I think it would be better to do do the "available" dance here rather than
> > delegating it to get_version(). get_version() can simply return NULL if there
> > is no version available.
>
> I don't have any preferences here and can move it. But just out of
> curiosity, can you elaborate what benefits do you see in moving the
> "available" logic in jit_get_version?

I don't have a hard technical argument for that. My gut feeling is the following:
jit_get_version() belongs to the pg_system_versions machinery, so it should deal
with the "available" that belongs to that API. jit_get_version() belongs to the
JIT implementation.

I won't object if you feel strongly that it should be the way it is now, since
that is more a matter of taste than anything else.

Yours,
Laurenz Albe