Message-ID: <270874d71328295c571ea760ac06ce747ef11821.camel@cybertec.at>
Subject: Re: System views for versions reporting
From: Laurenz Albe <laurenz.albe@cybertec.at>
To: Dmitry Dolgov <9erthalion6@gmail.com>
Cc: Tom Lane <tgl@sss.pgh.pa.us>, jian he <jian.universality@gmail.com>, Joe
 Conway <mail@joeconway.com>, pgsql-hackers@postgresql.org
Date: Fri, 21 Nov 2025 18:18:06 +0100
In-Reply-To: <jx5ozoohe653lf2f7p4j65b76pikqymdunpopjkeznfdzwgokv@bt43jrh226ee>
References: 
	<vximpev4vsyqvzvjn3qxb2apltuaxw7j4uxc4lggyjh3ejjbzx@66clgfgm6vlk>
	 <e4eb427c-0aea-4821-8940-eb5d949233dd@joeconway.com>
	 <tbbg5xlg46w4us4siwqa7v4y6lbr67hojjg5xgagwihrbsceh5@nfw62fdocrbc>
	 <5jhyl6p3nwcaxczfxuh25q42q6bqlaxyqyfqtw4tldvmm24bw2@rfii5tkwn5rg>
	 <CACJufxFD7ra6-HYmksJPMrY8Q036HA6EFyrUjrOPiq8CrfkGvg@mail.gmail.com>
	 <burlbnmi2a2mxt33migp3m3pqglgtat3axak2prmhiwhwuljxv@7ea4hlniydvk>
	 <866452.1742768493@sss.pgh.pa.us>
	 <yewkpc65y5g6fjd3kge2jetge3q2625hz64mietmyoczhqkmpu@ltotskxk4ndk>
	 <vyejydf4qnamuyz4slawjqzuhl3kgzfzgrtruwiuse2v6xitaa@ejvkxapdvqh5>
	 <a7d6f24f338149a0abfe058d7f5be5bbf4a7cd67.camel@cybertec.at>
	 <jx5ozoohe653lf2f7p4j65b76pikqymdunpopjkeznfdzwgokv@bt43jrh226ee>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.56.2 (3.56.2-2.fc42) 
MIME-Version: 1.0
Archived-At: 
 <https://www.postgresql.org/message-id/270874d71328295c571ea760ac06ce747ef11821.camel%40cybertec.at>
Precedence: bulk

On Fri, 2025-11-21 at 15:21 +0100, Dmitry Dolgov wrote:
> > > +       ctl.hcxt =3D CurrentMemoryContext;
> >=20
> > That hashtable should stick around for the entire life time of the back=
end.
> > Wouldn't it be better to explicitly name the correct memory context, ra=
ther
> > that to rely on being called with "CurrentMemoryContext" set correctly?
>=20
> 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 wind=
ow
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");
> >=20
> > Is that a problem?  Why throw an error?
>=20
> 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 th=
e
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, compi=
ler and
> > > +-- glibc. If built with JIT support, one more record will be display=
ed
> > > +-- containing LLVM version.
> > > +select count(*) >=3D 5 as ok FROM pg_system_versions;
> >=20
> > If you really think that a regression test for selecting from the view
> > is useful, use "SELECT EXISTS (TABLE pg_system_versions)". =20
> > Are you sure that the regression tests will always run with ICU enabled=
?
>=20
> 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 c=
heck
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 =3D false;
> > > +   return "";
> > > +}
> >=20
> > I think it would be better to do do the "available" dance here rather t=
han
> > delegating it to get_version().  get_version() can simply return NULL i=
f there
> > is no version available.
>=20
> 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 fol=
lowing:
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 t=
o the
JIT implementation.

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

Yours,
Laurenz Albe