Re: System views for versions reporting

Thanks for the detailed review! I generally agree with most of the
points and will try to incorporate them into the next version. Below are
few commentaries / questions for the rest.

> On Wed, Nov 19, 2025 at 09:57:12PM +0100, Laurenz Albe wrote:
> On Sun, 2025-11-16 at 17:45 +0100, Dmitry Dolgov wrote:
> > Better late than never, rebased and dropped 0004.
>
> I think that having a system view like that would be quite useful.
> It would be even more useful if it included other libraries that are
> optionally linked with PostgreSQL, like OpenSSL, OpenLDAP, lz4,
> zstd, GSSAPI etc.

Yeah, my plan is to extend the list of versions as soon as the
infrastructure is sorted out.

> > --- /dev/null
> > +++ b/src/backend/utils/misc/system_version.c
> > +void
> > +add_system_version(const char *name, SystemVersionCB cb, VersionType type)
> > +{
> > + SystemVersion *hentry;
> > + const char *key;
> > + bool found;
> > +
> > + if (!versions)
> > + {
> > + HASHCTL ctl;
> > +
> > + ctl.keysize = NAMEDATALEN;
> > + ctl.entrysize = sizeof(SystemVersion);
> > + 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.

> > +
> > + 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?

> > --- a/src/test/regress/sql/sysviews.sql
> > +++ b/src/test/regress/sql/sysviews.sql
> > @@ -101,3 +101,8 @@ select count(distinct utc_offset) >= 24 as ok from pg_timezone_abbrevs;
> > -- One specific case we can check without much fear of breakage
> > -- is the historical local-mean-time value used for America/Los_Angeles.
> > select * from pg_timezone_abbrevs where abbrev = 'LMT';
> > +
> > +-- 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?

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