Thread

System views for versions reporting

Dmitry Dolgov <9erthalion6@gmail.com> — 2024-10-06T15:36:49Z

Hi,

Based on the feedback in [1], here is my attempt at implementing system
views for versions reporting. It adds pg_system_versions for showing
things like core version, compiler, LLVM, etc, and pg_system_libraries
for showing linked shared objects. I think everyone has ageed that the
first was a good idea, where the second was only suggested -- after some
thinking I find shared obects useful enough to include here as well.

The main idea is to facilitate bug reporting. In particular, in many JIT
related bug reports one of the first questions is often "which LLVM
version is used?". Gathering such information is a manual process,
mistakes could be made when veryfing llvm-config output or anywhere
else. Having a system view for such purposes makes the process a bit
more robust.

The first three patches are essential for this purpose, the fourth one
is somewhat independent and could be concidered in isolation. The output
looks like this :

=# select * from pg_system_versions;
name | version | type
----------+--------------+--------------
Arch | x86_64-linux | Compile Time
ICU | 15.1 | Compile Time
Core | 18devel | Compile Time
Compiler | gcc-14.0.1 | Compile Time
LLVM | 18.1.6 | Run Time

=# select * from pg_system_libraries;
name
-----------------------------
/lib64/libkrb5.so.3
/lib64/libz.so.1
linux-vdso.so.1
/lib64/libxml2.so.2
[...]

Any feedback is appreciated.

0001-Add-infrastructure-for-pg_system_versions-view

Prepares the infrastructure, adds the view without populating it with
the data just yet. The view is backed by a hash table, which contains
callbacks returning a version string for a particular component. The
idea is to allow some flexibility in reporting, making components
responsible for how and when the information is exposed. E.g. it allows
to say that the version is not available.

0002-Add-core-versions-to-pg_system_versions.patch

Actually populates the view with some predefined compile time versions.
The versions are registered in PostgresMain, right after
BeginReportingGUCOptions -- there is no strong reasoning for that except
that it looks fine to me, so feel free to suggest a better place.

0003-Add-JIT-provider-version-to-pg_system_versions.patch

Finally adds LLVM version into the view via extending set of JIT
provider callbacks. The registration is happening together with the core
versions from the previous patch, because the JIT provider itself is
initialized only when a first expression is getting compiled.

0004-Add-pg_system_libraries-view.patch

Strictly speaking independent from the above patches. Adds the view to
show linked shared objects by iterating dl_phdr_info with
dl_iterate_phdr. It belongs to the standard C library, so I assume it's
portable enough.

[1]: https://www.postgresql.org/message-id/flat/znc72ymyoelvk5rjk5ub254v3qvcczfrk6autygjdobfvx2e7p%40s3dssvf34twa

Re: System views for versions reporting

Joe Conway <mail@joeconway.com> — 2024-10-06T16:01:29Z

On 10/6/24 11:36, Dmitry Dolgov wrote:
> Hi,
> 
> Based on the feedback in [1], here is my attempt at implementing system
> views for versions reporting. It adds pg_system_versions for showing
> things like core version, compiler, LLVM, etc, and pg_system_libraries
> for showing linked shared objects. I think everyone has ageed that the
> first was a good idea, where the second was only suggested -- after some
> thinking I find shared obects useful enough to include here as well.
> 
> The main idea is to facilitate bug reporting. In particular, in many JIT
> related bug reports one of the first questions is often "which LLVM
> version is used?". Gathering such information is a manual process,
> mistakes could be made when veryfing llvm-config output or anywhere
> else. Having a system view for such purposes makes the process a bit
> more robust.
> 
> The first three patches are essential for this purpose, the fourth one
> is somewhat independent and could be concidered in isolation. The output
> looks like this :
> 
> 	=# select * from pg_system_versions;
> 	   name   |   version    |     type
> 	----------+--------------+--------------
> 	 Arch     | x86_64-linux | Compile Time
> 	 ICU      | 15.1         | Compile Time
> 	 Core     | 18devel      | Compile Time
> 	 Compiler | gcc-14.0.1   | Compile Time
> 	 LLVM     | 18.1.6       | Run Time

I'm not sure why ICU is "Compile Time" rather than "Run Time" when it is 
not statically linked. Also, if we are going to include ICU here, 
shouldn't we also include libc version?

> 	=# select * from pg_system_libraries;
> 		name
> 	-----------------------------
> 	 /lib64/libkrb5.so.3
> 	 /lib64/libz.so.1
> 	 linux-vdso.so.1
> 	 /lib64/libxml2.so.2
> 	 [...]
> 
> Any feedback is appreciated.

I think it would be nice to include a sha256 hash (or something similar) 
of the libraries as well, so that they can be checked against known good 
values.

-- 
Joe Conway
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

Re: System views for versions reporting

Tom Lane <tgl@sss.pgh.pa.us> — 2024-10-06T16:16:47Z

Joe Conway <mail@joeconway.com> writes:
> I think it would be nice to include a sha256 hash (or something similar) 
> of the libraries as well, so that they can be checked against known good 
> values.

That seems well outside the charter of this patch.  Also, how would
we even get that information?  A typical application doesn't know
exactly what libraries it's linked with or where they came from on
the filesystem.  Maybe one could find that out with sufficient
platform-specific hackery, but I don't believe we could do it
portably.

			regards, tom lane

Re: System views for versions reporting

Dmitry Dolgov <9erthalion6@gmail.com> — 2024-10-07T09:26:41Z

> On Sun, Oct 06, 2024 at 12:01:29PM GMT, Joe Conway wrote:
> On 10/6/24 11:36, Dmitry Dolgov wrote:
> > Hi,
> >
> > Based on the feedback in [1], here is my attempt at implementing system
> > views for versions reporting. It adds pg_system_versions for showing
> > things like core version, compiler, LLVM, etc, and pg_system_libraries
> > for showing linked shared objects. I think everyone has ageed that the
> > first was a good idea, where the second was only suggested -- after some
> > thinking I find shared obects useful enough to include here as well.
> >
> > The main idea is to facilitate bug reporting. In particular, in many JIT
> > related bug reports one of the first questions is often "which LLVM
> > version is used?". Gathering such information is a manual process,
> > mistakes could be made when veryfing llvm-config output or anywhere
> > else. Having a system view for such purposes makes the process a bit
> > more robust.
> >
> > The first three patches are essential for this purpose, the fourth one
> > is somewhat independent and could be concidered in isolation. The output
> > looks like this :
> >
> > 	=# select * from pg_system_versions;
> > 	   name   |   version    |     type
> > 	----------+--------------+--------------
> > 	 Arch     | x86_64-linux | Compile Time
> > 	 ICU      | 15.1         | Compile Time
> > 	 Core     | 18devel      | Compile Time
> > 	 Compiler | gcc-14.0.1   | Compile Time
> > 	 LLVM     | 18.1.6       | Run Time
>
> I'm not sure why ICU is "Compile Time" rather than "Run Time" when it is not
> statically linked.

It reports U_UNICODE_VERSION at compile time. It's not necessarily
correct though, I can try to replace it with the runtime version. I
think there was some ICU functionality (something like
u_getUnicodeVersion), which is maybe a better fit.

> Also, if we are going to include ICU here, shouldn't we
> also include libc version?

Yeah, why not. One of my goals here is to identify a balanced set of
useful versions to report.

> > 	=# select * from pg_system_libraries;
> > 		name
> > 	-----------------------------
> > 	 /lib64/libkrb5.so.3
> > 	 /lib64/libz.so.1
> > 	 linux-vdso.so.1
> > 	 /lib64/libxml2.so.2
> > 	 [...]
> >
> > Any feedback is appreciated.
>
> I think it would be nice to include a sha256 hash (or something similar) of
> the libraries as well, so that they can be checked against known good
> values.

I was thinking about getting more info to show in this view, but haven't
found any reasonable way to achieve that. So I would agree with Tom on
that.

Re: System views for versions reporting

Peter Eisentraut <peter@eisentraut.org> — 2024-10-16T12:47:34Z

On 06.10.24 17:36, Dmitry Dolgov wrote:
> Based on the feedback in [1], here is my attempt at implementing system
> views for versions reporting. It adds pg_system_versions for showing
> things like core version, compiler, LLVM, etc, and pg_system_libraries
> for showing linked shared objects.

Is a system view the right interface?  For example, in pgbouncer we just 
added it to the version output:

$ pgbouncer --version
PgBouncer 1.18.0
libevent 2.1.12-stable
adns: c-ares 1.19.0
tls: OpenSSL 3.3.2 3 Sep 2024

That way, you can get this information without having to start a server 
instance.  (Maybe you can't start a server instance because it just 
crashed because of some library version issue ...)

Re: System views for versions reporting

Joe Conway <mail@joeconway.com> — 2024-10-16T13:31:58Z

On 10/16/24 08:47, Peter Eisentraut wrote:
> On 06.10.24 17:36, Dmitry Dolgov wrote:
>> Based on the feedback in [1], here is my attempt at implementing system
>> views for versions reporting. It adds pg_system_versions for showing
>> things like core version, compiler, LLVM, etc, and pg_system_libraries
>> for showing linked shared objects.
> 
> Is a system view the right interface?  For example, in pgbouncer we just
> added it to the version output:
> 
> $ pgbouncer --version
> PgBouncer 1.18.0
> libevent 2.1.12-stable
> adns: c-ares 1.19.0
> tls: OpenSSL 3.3.2 3 Sep 2024
> 
> That way, you can get this information without having to start a server
> instance.  (Maybe you can't start a server instance because it just
> crashed because of some library version issue ...)


While it is also useful to be able to get the info without being able to 
start the server, I think that would be an addition not a replacement. 
When you have a fleet with no direct access to run shell commands, being 
able to get this info via SQL is valuable.

-- 
Joe Conway
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

Re: System views for versions reporting

Tom Lane <tgl@sss.pgh.pa.us> — 2024-10-16T14:35:40Z

Joe Conway <mail@joeconway.com> writes:
> On 10/16/24 08:47, Peter Eisentraut wrote:
>> That way, you can get this information without having to start a server
>> instance.  (Maybe you can't start a server instance because it just
>> crashed because of some library version issue ...)

> While it is also useful to be able to get the info without being able to 
> start the server, I think that would be an addition not a replacement. 
> When you have a fleet with no direct access to run shell commands, being 
> able to get this info via SQL is valuable.

Yeah.  In addition, I envisioned that this might include information
that's only readily available at runtime.  Don't have a concrete
example at hand (-ENOCAFFEINE) but I think that --version is
necessarily going to be exceedingly constrained in what it can do.

Another problem is that, just like with version(), there is already
code making assumptions about what --version will output.  Most of
that is under our control, but perhaps not all.  The main value
of a new system view, IMV, is that it's a completely green field
for us to define the contents of.

			regards, tom lane

Re: System views for versions reporting

Dmitry Dolgov <9erthalion6@gmail.com> — 2024-10-19T17:31:56Z

> On Mon, Oct 07, 2024 at 11:26:41AM GMT, Dmitry Dolgov wrote:
> > On Sun, Oct 06, 2024 at 12:01:29PM GMT, Joe Conway wrote:
> > I'm not sure why ICU is "Compile Time" rather than "Run Time" when it is not
> > statically linked.
>
> It reports U_UNICODE_VERSION at compile time. It's not necessarily
> correct though, I can try to replace it with the runtime version. I
> think there was some ICU functionality (something like
> u_getUnicodeVersion), which is maybe a better fit.
>
> > Also, if we are going to include ICU here, shouldn't we
> > also include libc version?
>
> Yeah, why not. One of my goals here is to identify a balanced set of
> useful versions to report.

Here is how it would look like, I've added icu and glibc runtime
versions into the second patch.

Re: System views for versions reporting

jian he <jian.universality@gmail.com> — 2025-01-02T02:36:48Z

hi.
https://cirrus-ci.com/github/postgresql-cfbot/postgresql/cf%2F5318
shows lots of failures, but it doesn't seem to tell you about doc build failure.

+ <sect1 id="view-pg-system-versions">
+  <title><structname>pg_system_versions</structname></title>
+
+  <indexterm zone="view-pg-system-version">
+   <primary>pg_system_versions</primary>
+  </indexterm>

+  <indexterm zone="view-pg-system-version">
should change to
+  <indexterm zone="view-pg-system-versions">
otherwise cannot build doc.


+  <table>
+   <title><structname>pg_system_versions</structname> Columns</title>
+   <tgroup cols="1">
...
column "type" of view pg_system_versions is missing in the doc entry
?


+ if (found)
+ ereport(ERROR,
+ (errcode(ERRCODE_DUPLICATE_OBJECT),
+ errmsg("duplicated system version")));
this is unlikely to happen (not user visible error), normally we
should just use elog(ERROR...)
?


+typedef enum VersionType
+{
+ CompileTime,
+ RunTime,
+} VersionType;
+
+typedef struct SystemVersion
+{
+ char name[NAMEDATALEN]; /* Unique component name, used as a key
+ * for versions HTAB */
+ VersionType type;
+ SystemVersionCB callback; /* Callback to fetch the version string */
+} SystemVersion;
these two structs also need to be added into src/tools/pgindent/typedefs.list?


--- a/src/include/utils/system_version.h
+++ b/src/include/utils/system_version.h
@@ -11,6 +11,7 @@
 #ifndef SYSTEM_VERSION_H
 #define SYSTEM_VERSION_H

+#include <gnu/libc-version.h>
 #include <link.h>

"gnu/libc-version.h" does not exist in the clang compiler?
will "link.h" everywhere?




Currently, only a few rows are displayed for pg_system_versions. If I have
linked a dependency, I should be able to retrieve its version—for example, I
should be able to determine the version of zstd (i have linked the
zstd dependency).

Re: System views for versions reporting

Andres Freund <andres@anarazel.de> — 2025-01-02T02:45:57Z

On 2025-01-02 10:36:48 +0800, jian he wrote:
> https://cirrus-ci.com/github/postgresql-cfbot/postgresql/cf%2F5318
> shows lots of failures, but it doesn't seem to tell you about doc build
> failure.

It does:
https://cirrus-ci.com/task/6472750665039872?logs=docs_build#L0

[15:26:26.443] time make -s -j${BUILD_JOBS} -C doc
[15:26:28.759] postgres.sgml:5082: element indexterm: validity error : IDREFS attribute zone references an unknown ID "view-pg-system-version"

Re: System views for versions reporting

Dmitry Dolgov <9erthalion6@gmail.com> — 2025-01-25T19:46:58Z

> On Thu, Jan 02, 2025 at 10:36:48AM GMT, jian he wrote:
> hi.
> https://cirrus-ci.com/github/postgresql-cfbot/postgresql/cf%2F5318
> shows lots of failures, but it doesn't seem to tell you about doc build failure.

Thanks for checking this out. Here is the updated version with applied
changes. The tests were failing due to injection_points library
apparently linking something twice, triggering a duplication error I
didn't expect. Since apparently it can happen, I'm just overwriting
duplicates.

> --- a/src/include/utils/system_version.h
> +++ b/src/include/utils/system_version.h
> @@ -11,6 +11,7 @@
>  #ifndef SYSTEM_VERSION_H
>  #define SYSTEM_VERSION_H
>
> +#include <gnu/libc-version.h>
>  #include <link.h>
>
> "gnu/libc-version.h" does not exist in the clang compiler?
> will "link.h" everywhere?

This one I don't follow, what do you mean by "link.h" everywhere? I
think those two are coming from glibc and compiler independent. But it
indeed makes sense to wrap them into __GLIBC__.

> Currently, only a few rows are displayed for pg_system_versions. If I have
> linked a dependency, I should be able to retrieve its version—for example, I
> should be able to determine the version of zstd (i have linked the
> zstd dependency).

Maybe I'm missing something, but that's the plan -- pg_system_versions
contains only some selected components, that have to be registered
manually before showing up there. This of course means that it's hard to
get all the linked libraries, but opens a possibility to show any kind
of objects, not only what is linked. The actual list of versions is open
for discussion of course, but showing everything is not feasible I
think. For things like zstd pg_system_libraries is a better fit.

Re: System views for versions reporting

Tom Lane <tgl@sss.pgh.pa.us> — 2025-03-23T22:21:33Z

Dmitry Dolgov <9erthalion6@gmail.com> writes:
> Thanks for checking this out. Here is the updated version with applied
> changes. The tests were failing due to injection_points library
> apparently linking something twice, triggering a duplication error I
> didn't expect. Since apparently it can happen, I'm just overwriting
> duplicates.

FWIW, I think the 0004 patch is about to be mostly obsoleted by
Andrei's proposal at [1].  To the extent that it's not obsoleted,
I question whether it's something we want at all, given the ground
rule that unprivileged users are not supposed to have access to info
about the server's filesystem.

			regards, tom lane

[1] https://www.postgresql.org/message-id/flat/dd4d1b59-d0fe-49d5-b28f-1e463b68fa32@gmail.com

Re: System views for versions reporting

Dmitry Dolgov <9erthalion6@gmail.com> — 2025-04-01T19:27:23Z

> On Sun, Mar 23, 2025 at 06:21:33PM GMT, Tom Lane wrote:
>
> FWIW, I think the 0004 patch is about to be mostly obsoleted by
> Andrei's proposal at [1].  To the extent that it's not obsoleted,
> I question whether it's something we want at all, given the ground
> rule that unprivileged users are not supposed to have access to info
> about the server's filesystem.

To be clear -- I don't have a case for 0004 myself, except some vague
expectation that in certain situations it could be useful to know which
shared objects are loaded, even if they are not Postgres modules. Based
on the feedback from the original thread [2], there were couple similar
opinions, maybe folks could reply here whether [1] would be sufficient
for them.

I agree with the argument about the privileges. If the 0004 patch will
be found useful, it would make sense to allow only superuser to access
this view. I assume "revoke all on pg_system_libraries from public"
should be enough, would this address the concern?

[2]: https://www.postgresql.org/message-id/flat/znc72ymyoelvk5rjk5ub254v3qvcczfrk6autygjdobfvx2e7p%40s3dssvf34twa

Re: System views for versions reporting

Dmitry Dolgov <9erthalion6@gmail.com> — 2025-11-16T16:45:15Z

> On Tue, Apr 01, 2025 at 09:27:23PM +0200, Dmitry Dolgov wrote:
> > On Sun, Mar 23, 2025 at 06:21:33PM GMT, Tom Lane wrote:
> >
> > FWIW, I think the 0004 patch is about to be mostly obsoleted by
> > Andrei's proposal at [1].  To the extent that it's not obsoleted,
> > I question whether it's something we want at all, given the ground
> > rule that unprivileged users are not supposed to have access to info
> > about the server's filesystem.
> 
> To be clear -- I don't have a case for 0004 myself, except some vague
> expectation that in certain situations it could be useful to know which
> shared objects are loaded, even if they are not Postgres modules. Based
> on the feedback from the original thread [2], there were couple similar
> opinions, maybe folks could reply here whether [1] would be sufficient
> for them.

Better late than never, rebased and dropped 0004.

Re: System views for versions reporting

Laurenz Albe <laurenz.albe@cybertec.at> — 2025-11-19T20:57:12Z

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.

Building the server worked fine on my Linux box (but see my comments
about Glibc below), and the view behaves like it should.

Building the documentation fails:
element indexterm: validity error : IDREFS attribute zone references an unknown ID "view-pg-system-version"
The problem is here:

> --- a/doc/src/sgml/system-views.sgml
> +++ b/doc/src/sgml/system-views.sgml
> + <sect1 id="view-pg-system-versions">
> +  <title><structname>pg_system_versions</structname></title>
> +
> +  <indexterm zone="view-pg-system-version">
> +   <primary>pg_system_versions</primary>
> +  </indexterm>

"view-pg-system-version" is missing the "s" at the end.

Comments on the code:
=====================

Patch 0001:
-----------

> --- a/doc/src/sgml/system-views.sgml
> +++ b/doc/src/sgml/system-views.sgml
> +     <row>
> +      <entry><link linkend="view-pg-system-versions"><structname>pg_system_versions</structname></link></entry>
> +      <entry>system versions</entry>
> +     </row>
> +
>      </tbody>
>     </tgroup>
>    </table>

That list is alphabetically sorted by function name.  You should add the function at the
correct place, not at the end.

The same applies to the next hunk.  It gets rendered in its own page, so the order is
not user-visible, but keeping these sections in the same alphabetical order will ease
maintenance.

> + <sect1 id="view-pg-system-versions">
> +  <title><structname>pg_system_versions</structname></title>
> +
> +  <indexterm zone="view-pg-system-version">
> +   <primary>pg_system_versions</primary>
> +  </indexterm>
> +
> +  <para>
> +   The view <structname>pg_system_versions</structname> provides description
> +   about versions of various system components, e.g. PostgreSQL itself,
> +   compiler used to build it, dependencies, etc.
> +  </para>
> +
> [...]
> +     <row>
> +      <entry role="catalog_table_entry"><para role="column_definition">
> +       <structfield>type</structfield> <type>text</type>
> +      </para>
> +      <para>
> +       Component type (compile time or Run time)
> +      </para></entry>
> +     </row>

I'm not happy with the English in "The view ... provides description about ...",
but all the other views use the same wording, so we should stick with it.
But in "description about versions of various system components", there should
be a "the" before "versions".  It should also be "the compiler".  Since you start
the list with "e.g.", I think the "etc." is superfluous.

In "compile time or Run time", either use upper case everywhere (except in "or")
or lower case everywhere.  I suggest to use upper case like in the actual view
output.

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

> +
> +       versions = hash_create("System versions table",
> +                              MAX_SYSTEM_VERSIONS,
> +                              &ctl,
> +                              HASH_ELEM | HASH_STRINGS);
> +   }
> +
> +   key = pstrdup(name);
> +   hentry = (SystemVersion *) hash_search(versions, key,
> +                                          HASH_ENTER, &found);

I think you should at least add an Assert() that makes sure that the key
is not longer than NAMEDATALEN.

> +
> +   if (found)
> +       elog(ERROR, "duplicated system version");

Is that a problem?  Why throw an error?

> +Datum
> +pg_get_system_versions(PG_FUNCTION_ARGS)
> +{
> [...]
> +   while ((hentry = (SystemVersion *) hash_seq_search(&status)) != NULL)
> +   {
> +       Datum       values[PG_GET_SYS_VERSIONS_COLS] = {0};
> +       bool        nulls[PG_GET_SYS_VERSIONS_COLS] = {0};
> +       bool        available = false;
> +       const char *version = hentry->callback(&available);
> +
> +       if (!available)
> +           continue;
> +
> +       values[0] = CStringGetTextDatum(hentry->name);
> +       values[1] = CStringGetTextDatum(version);
> +       values[2] = hentry->type;
> +
> +       tuplestore_putvalues(rsinfo->setResult, rsinfo->setDesc, values, nulls);
> +   }

I think the assignment for the third column should be

  values[2] = Int32GetDatum((int32) hentry->type);

since hentry->type is an enum type and hence an "int".

Patch 0002:
-----------

> --- a/src/backend/utils/misc/system_version.c
> +++ b/src/backend/utils/misc/system_version.c
> +/*
> + * Register versions that describe core components and do not correspond to any
> + * individual component.
> + */
> +void
> +register_core_versions()
> +{
> +   add_system_version("Core", core_get_version, CompileTime);
> +   add_system_version("Arch", core_get_arch, CompileTime);
> +   add_system_version("Compiler", core_get_compiler, CompileTime);
> +   add_system_version("ICU", icu_get_version, RunTime);
> +   add_system_version("Glibc", glibc_get_version, RunTime);
> +}
> [...]
> +const char *
> +glibc_get_version(bool *available)
> +{
> +   *available = true;
> +   return (const char *) gnu_get_libc_version();
> +}

Not all operating systems use Glibc, not even all Linux distributions.
If you look at the commitfest entry, you'll see the the CI build fails on
most platforms.

At the very least, this calls for conditional compilation.
Really, there should be an add_system_version() call for all types of
C libraries that exist out there, but that might border on the impossible.

> --- a/src/include/utils/system_version.h
> +++ b/src/include/utils/system_version.h
> @@ -11,6 +11,10 @@
>  #ifndef SYSTEM_VERSION_H
>  #define SYSTEM_VERSION_H
>  
> +#ifdef __GLIBC__
> +#include <gnu/libc-version.h>
> +#endif
> +

That should be included where glibc_get_version() is defined, not here.

>  #define MAX_SYSTEM_VERSIONS 100
>  
>  typedef enum VersionType
> @@ -34,5 +38,13 @@ typedef struct SystemVersion
>  } SystemVersion;
>  
>  void       add_system_version(const char *name, SystemVersionCB cb, VersionType type);
> +extern void register_core_versions(void);
> +
> +const char *core_get_version(bool *available);
> +const char *core_get_arch(bool *available);
> +const char *core_get_compiler(bool *available);
> +
> +const char *icu_get_version(bool *available);
> +const char *glibc_get_version(bool *available);
>  
>  #endif                         /* SYSTEM_VERSION_H */

I think that these functions had better be "static".  Do you expect a need to
call them from other parts of the code?

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

Patch 0003:
-----------

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

A more meaningful function comment would be "Callback for add_system_version()".

> +void
> +jit_register_version(void)
> +{
> +   add_system_version("LLVM", jit_get_version, RunTime);
> +}

I think that should be in utils/misc/system_version.c.
Then you don't need to #include "utils/system_version.h" in the JIT code.

> --- a/src/include/jit/jit.h
> +++ b/src/include/jit/jit.h
> +/*
> + * Get the provider's version string. The flag indicating availability is
> + * passed as an argument, and will be set accordingly if it's not possible to
> + * get the version.
> + */
> +extern const char *jit_get_version(bool *available);

Again, I think "Callback for add_system_version()" would be a more useful
comment.  I have no trouble guessing that the function will return the JIT
version, and if you know add_system_version(), you know what "available" is.

Yours,
Laurenz Albe

Re: System views for versions reporting

Dmitry Dolgov <9erthalion6@gmail.com> — 2025-11-21T14:21:45Z

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?

Re: System views for versions reporting

Laurenz Albe <laurenz.albe@cybertec.at> — 2025-11-21T17:18:06Z

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

Re: System views for versions reporting

Dmitry Dolgov <9erthalion6@gmail.com> — 2025-11-25T15:40:52Z

> On Fri, Nov 21, 2025 at 03:21:45PM +0100, Dmitry Dolgov wrote:
> 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.

Here is the updated patch.

Re: System views for versions reporting

Dmitry Dolgov <9erthalion6@gmail.com> — 2025-11-25T18:54:42Z

> On Tue, Nov 25, 2025 at 04:40:52PM +0100, Dmitry Dolgov wrote:
> > On Fri, Nov 21, 2025 at 03:21:45PM +0100, Dmitry Dolgov wrote:
> > 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.
> 
> Here is the updated patch.

Looks line MinGW and Visual Studio lack unicode/uchar.h, I would need to
add something for them -- will follow up shortly.

Re: System views for versions reporting

Laurenz Albe <laurenz.albe@cybertec.at> — 2025-11-26T13:28:06Z

On Tue, 2025-11-25 at 16:40 +0100, Dmitry Dolgov wrote:
> Here is the updated patch.

Thanks for the updated patch.

Comments:

You didn't address any of my suggestions concerning the documentation,
except that you moved the entry in "System Views" to the correct place.

The second patch contains:

> +void
> +jit_register_version(void)
> +{
> +   add_system_version("LLVM", jit_get_version, RunTime);
> +}

But that belongs into the third patch.

+/*
+ * Callback for add_system_version, returns JIT provider's version string and
+ * reports if it's not available.
+ */
+const char *
+jit_get_version(bool *available)
+{
+   const char *version;
+
+   if (!provider_init())
+   {
+       *available = false;
+       return "";
+   }
+
+   version = provider.get_version();
+
+   if (version == NULL)
+   {
+       *available = false;
+       return "";
+   }
+
+   *available = true;
+   return version;
+}

Perhaps more elegant would be:

    if (provider_init())
    {
        version = provider.get_version();
        if (version)
        {
            *available = true;
            return version;
        }
    }

    *available = false;
    return "";

Other than that, it looks fine.

Yours,
Laurenz Albe

Re: System views for versions reporting

Dmitry Dolgov <9erthalion6@gmail.com> — 2025-11-26T13:53:17Z

> On Wed, Nov 26, 2025 at 02:28:06PM +0100, Laurenz Albe wrote:
> You didn't address any of my suggestions concerning the documentation,
> except that you moved the entry in "System Views" to the correct place.

I did address the build failure and the order of entries, but looks like
I've overlooked the spelling. Was there anything else?

Re: System views for versions reporting

Laurenz Albe <laurenz.albe@cybertec.at> — 2025-11-26T15:39:50Z

On Wed, 2025-11-26 at 14:53 +0100, Dmitry Dolgov wrote:
> > On Wed, Nov 26, 2025 at 02:28:06PM +0100, Laurenz Albe wrote:
> > You didn't address any of my suggestions concerning the documentation,
> > except that you moved the entry in "System Views" to the correct place.
> 
> I did address the build failure and the order of entries, but looks like
> I've overlooked the spelling. Was there anything else?

In the documentation of the "type" column, I suggest using

   Compile Time or Run Time

(capitalization like in the view results), or even better

   <quote>Compile Time</quote> or <quote>Run Time</quote>

Yours,
Laurenz Albe

Re: System views for versions reporting

Dmitry Dolgov <9erthalion6@gmail.com> — 2025-11-26T19:15:27Z

> On Wed, Nov 26, 2025 at 04:39:50PM +0100, Laurenz Albe wrote:
> In the documentation of the "type" column, I suggest using
> 
>    Compile Time or Run Time
> 
> (capitalization like in the view results), or even better
> 
>    <quote>Compile Time</quote> or <quote>Run Time</quote>

Yeah, I've stored them both under "spelling" in my head :) Now
everything should be there.

> But that belongs into the third patch.

Good catch, moved this chunk.

> Perhaps more elegant would be:

I was considering this originally, but my dislike of nested conditions
has prevailed. At the same time I don't mind returning to this version.

I've also realized that MinGW and Visual Studio failures were in fact
due to the uchar.h include not being guarded by USE_ICU.

Re: System views for versions reporting

Laurenz Albe <laurenz.albe@cybertec.at> — 2025-11-27T14:27:21Z

Thanks, the patch looks good now.

There is one tiny thing that I noticed only now, sorry:

> --- a/src/backend/utils/misc/Makefile
> +++ b/src/backend/utils/misc/Makefile
> @@ -32,7 +32,8 @@ OBJS = \
>     stack_depth.o \
>     superuser.o \
>     timeout.o \
> -   tzparser.o
> +   tzparser.o \
> +   system_version.o

That looks like an alphabetically sorted list too, so you should preserve
that ordering.

Anyway, I'll set this patch "ready for committer".

I am not sure if this reduced form (no report on zstd, lz4, openssl,
zlib etc.) is good enough for release, but I'll leave that decision to a committer.

Yours,
Laurenz Albe