Обсуждение: Re: System views for versions reporting
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
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
> 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.
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).
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"
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
> 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
> 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.
Вложения
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
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?
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
> 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.
Вложения
> 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.
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
> 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?
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
> 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.
Вложения
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