[PATCH] Skip llvm bytecode generation if LLVM is missing
Short version: Currently if the server is built with --with-llvm the
-devel packages must depend on clang for PGXS to work, even though
llvm is otherwise optional. This is a particular problem on older
platforms where 3rd party LLVM may be required to build the server's
llvmjit support. Work around by skipping the default .bc generation if
no clang is found by PGXS, as if $(with_llvm) was false.
Detail:
If PostgreSQL is configured with --with--lvm it writes with_llvm=yes
into Makefile.global via AC_SUBST, along with the CLANG path and the
path to the LLVM_TOOLSET if supplied. PGXS sets up a %.bc dependency
for OBJS if it detects that the server was compiled with llvm support.
If clang is not found at PGXS extension build-time the extension build
will then fail, despite the user not having installed the
postgresql11-llvmjit (or whatever) package and the extension not
declaring any explicit LLVM dependency or requirement.
Andres and others went to a great deal of effort to make it possible
to separate PostgreSQL's LLVM support, so a server built with LLVM
support doesn't actually have a runtime dependency on llvm unless the
llvmjit module is loaded. This allows packagers to separate it out and
avoid the need to declare an llvm dependency on the main server
package.
I've found that this falls down for -devel packages like those the
PGDG Yum team produces. The problem arises when a container or VM is
used to build the server and its Makefile.global etc. Then the -devel
package containing Makefile.global and other PGXS bits are installed
on a new machine that does not have llvm's clang. PGXS builds will
fail when they attempt to generate bytecode, since they expect clang
to be present at the path baked in to Makefile.global - but it's
absent.
I propose that per the attached patch PGXS should simply skip adding
the automatic dependency for .bc files if clang cannot be found.
Extensions may still choose to explicitly declare the rule in their
own Makefile if they want to force bitcode generation.
If we want to get fancier about it, we could instead split the llvm
support out from Makefile.global into a Makefile.llvm or similar,
which is then conditionally included by Makefile.global if it exists.
Makefile.llvm would be packaged in a new postgresqlXX-llvmjit-devel
package since distros get uppity if you put makefile fragments in
runtime packages. This package would depend on llvm-toolset and clang.
If you install postgresqlXX-devel but not postgresqlXX-llvmjit-devel
you don't get bitcode and don't need clang. If you install
postgresqlXX-llvmjit-devel, the same clang as we built the server with
is declared as a dependency and Makefile.llvm is included, so it all
works.
But I don't think it's worth the hassle and I'd rather just skip
automatic bitcode generation if we don't find clang.
See also yum packagers report at
/messages/by-id/CAMsr+YHokx0rWLV561z3=gAi6CM4YJekgCLkqmDwQSTEjVYhuw@mail.gmail.com
.
--
Craig Ringer http://www.2ndQuadrant.com/
2ndQuadrant - PostgreSQL Solutions for the Enterprise
Attachments:
Makefile-llvm-toolset.patchtext/x-patch; charset=US-ASCII; name=Makefile-llvm-toolset.patchDownload
diff --git a/src/Makefile.global.in b/src/Makefile.global.in
index e3ea7f21a7..df191709b2 100644
--- a/src/Makefile.global.in
+++ b/src/Makefile.global.in
@@ -1020,6 +1020,21 @@ endif # enable_coverage
# LLVM support
#
+# Don't try to build llvm bitcode if the clang used when Pg itself was built
+# can no longer be found. This is possible when this Makefile.global is
+# installed via a -devel package that doesn't have a hard dependency on clang,
+# particularly for enterprise distros that may use a newer clang for builds.
+ifeq ($(with_llvm),yes)
+# Only override setting made by Makefile.global defaults not on cmdline etc
+ifeq ($(origin with_llvm),file)
+ifeq ($(wildcard $(CLANG)),)
+$(warning disabling llvm bitcode generation: $$(CLANG) = $(CLANG) not found)
+with_llvm = no
+endif
+endif
+endif
+
ifndef COMPILE.c.bc
# -Wno-ignored-attributes added so gnu_printf doesn't trigger
# warnings, when the main binary is compiled with C.
On Wed, 2020-03-11 at 11:25 +0800, Craig Ringer wrote:
Short version: Currently if the server is built with --with-llvm the
-devel packages must depend on clang for PGXS to work, even though
llvm is otherwise optional. This is a particular problem on older
platforms where 3rd party LLVM may be required to build the server's
llvmjit support. Work around by skipping the default .bc generation if
no clang is found by PGXS, as if $(with_llvm) was false.
+1
I have struggled with this, as have several users trying to build oracle_fdw.
Yours,
Laurenz Albe
Le mer. 11 mars 2020 à 05:28, Laurenz Albe <laurenz.albe@cybertec.at> a
écrit :
On Wed, 2020-03-11 at 11:25 +0800, Craig Ringer wrote:
Short version: Currently if the server is built with --with-llvm the
-devel packages must depend on clang for PGXS to work, even though
llvm is otherwise optional. This is a particular problem on older
platforms where 3rd party LLVM may be required to build the server's
llvmjit support. Work around by skipping the default .bc generation if
no clang is found by PGXS, as if $(with_llvm) was false.+1
I have struggled with this, as have several users trying to build
oracle_fdw.
+1, I had similar experience with other extensions.
Show quoted text
st 11. 3. 2020 v 6:43 odesílatel Julien Rouhaud <rjuju123@gmail.com> napsal:
Le mer. 11 mars 2020 à 05:28, Laurenz Albe <laurenz.albe@cybertec.at> a
écrit :On Wed, 2020-03-11 at 11:25 +0800, Craig Ringer wrote:
Short version: Currently if the server is built with --with-llvm the
-devel packages must depend on clang for PGXS to work, even though
llvm is otherwise optional. This is a particular problem on older
platforms where 3rd party LLVM may be required to build the server's
llvmjit support. Work around by skipping the default .bc generation if
no clang is found by PGXS, as if $(with_llvm) was false.+1
I have struggled with this, as have several users trying to build
oracle_fdw.+1, I had similar experience with other extensions.
+1
Pavel
On Wed, 11 Mar 2020 at 13:47, Pavel Stehule <pavel.stehule@gmail.com> wrote:
st 11. 3. 2020 v 6:43 odesílatel Julien Rouhaud <rjuju123@gmail.com> napsal:
Le mer. 11 mars 2020 à 05:28, Laurenz Albe <laurenz.albe@cybertec.at> a écrit :
On Wed, 2020-03-11 at 11:25 +0800, Craig Ringer wrote:
Short version: Currently if the server is built with --with-llvm the
-devel packages must depend on clang for PGXS to work, even though
llvm is otherwise optional. This is a particular problem on older
platforms where 3rd party LLVM may be required to build the server's
llvmjit support. Work around by skipping the default .bc generation if
no clang is found by PGXS, as if $(with_llvm) was false.+1
I have struggled with this, as have several users trying to build oracle_fdw.
+1, I had similar experience with other extensions.
+1
BTW, as a workaround in the mean time you can suppress bitcode
generation by building your ext with:
make with_llvm=no all
Similarly, if postgres is not using your ccache for builds because
it's invoking a baked-in compiler path, you can run
make CC=$(type -p gcc) GCC=$(type -p gcc) all
to force fresh path-lookups that should find your ccache-wrappers for
the compiler.
This will also work around problems where ccache was used to build
postgres, so ccache-wrapper paths got baked into Makefile.in, so your
PGXS builds fail with something like:
make: /usr/lib64/ccache/gcc: Command not found
--
Craig Ringer http://www.2ndQuadrant.com/
2ndQuadrant - PostgreSQL Solutions for the Enterprise
Hi,
On 2020-03-11 11:25:28 +0800, Craig Ringer wrote:
I propose that per the attached patch PGXS should simply skip adding
the automatic dependency for .bc files if clang cannot be found.
Extensions may still choose to explicitly declare the rule in their
own Makefile if they want to force bitcode generation.
Hm, that seems like it could also cause silent failures (e.g. after a
package upgrade or such).
How about erroring out, but with an instruction that llvm can be
disabled with make NO_LLVM=1 or such?
Regards,
Andres
At Wed, 11 Mar 2020 12:43:22 -0700, Andres Freund <andres@anarazel.de> wrote in
Hi,
On 2020-03-11 11:25:28 +0800, Craig Ringer wrote:
I propose that per the attached patch PGXS should simply skip adding
the automatic dependency for .bc files if clang cannot be found.
Extensions may still choose to explicitly declare the rule in their
own Makefile if they want to force bitcode generation.Hm, that seems like it could also cause silent failures (e.g. after a
package upgrade or such).How about erroring out, but with an instruction that llvm can be
disabled with make NO_LLVM=1 or such?
+1 for requiring such options for the same reason. The current patch
disables LLVM for the enviroment where clang is installed but ccache
is not, while building an extension based on postgresql-devel package.
(ccache is in EPEL on RHEL/CentOS.)
A bit aside from LLVM itself, I'd like CC/CPP/CLANG to fall back to
them in the PATH on the running environment if they don't exist.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
On Thu, 12 Mar 2020 at 03:43, Andres Freund <andres@anarazel.de> wrote:
On 2020-03-11 11:25:28 +0800, Craig Ringer wrote:
I propose that per the attached patch PGXS should simply skip adding
the automatic dependency for .bc files if clang cannot be found.
Extensions may still choose to explicitly declare the rule in their
own Makefile if they want to force bitcode generation.Hm, that seems like it could also cause silent failures (e.g. after a
package upgrade or such).How about erroring out, but with an instruction that llvm can be
disabled with make NO_LLVM=1 or such?
I thought about that at first, but that'll only benefit people who're
hand-compiling things, and it's already possible with
make with_llvm=no ...
The proportion of people hand-compiling is an ever-shrinking
proportion of the user base. When something's nested inside an rpm
specfile inside a docker container inside a bash script inside another
Docker container on an AWS instance .... not so fun. They might be
able to inject it into the environment. But often not.
Extensions that explicitly must generate bytecode may add their own
dependency rule. Or we could make skipping bytecode generation if llvm
cannot be found at build-time something the extension can turn off
with a PGXS option, as suggested upthread.
I'm reluctant to go with anything that requires each existing
extension to be patched because that introduces a huge lag time for
this change to actually help anyone out.
--
Craig Ringer http://www.2ndQuadrant.com/
2ndQuadrant - PostgreSQL Solutions for the Enterprise
At Thu, 12 Mar 2020 14:08:31 +0800, Craig Ringer <craig@2ndquadrant.com> wrote in
On Thu, 12 Mar 2020 at 03:43, Andres Freund <andres@anarazel.de> wrote:
On 2020-03-11 11:25:28 +0800, Craig Ringer wrote:
I propose that per the attached patch PGXS should simply skip adding
the automatic dependency for .bc files if clang cannot be found.
Extensions may still choose to explicitly declare the rule in their
own Makefile if they want to force bitcode generation.Hm, that seems like it could also cause silent failures (e.g. after a
package upgrade or such).How about erroring out, but with an instruction that llvm can be
disabled with make NO_LLVM=1 or such?I thought about that at first, but that'll only benefit people who're
hand-compiling things, and it's already possible withmake with_llvm=no ...
The proportion of people hand-compiling is an ever-shrinking
proportion of the user base. When something's nested inside an rpm
specfile inside a docker container inside a bash script inside another
Docker container on an AWS instance .... not so fun. They might be
able to inject it into the environment. But often not.Extensions that explicitly must generate bytecode may add their own
dependency rule. Or we could make skipping bytecode generation if llvm
cannot be found at build-time something the extension can turn off
with a PGXS option, as suggested upthread.
FWIW, the patch makes bitcode generation skipped (almost silently)
when clang is installed but ccache is not, with -devel packages for
CentOS8. Counldn't we make the bitcode generation a separate make
target, at least for PGXS build? Rather than turning it on and off by
the condition which doens't seem having a clear relation with the
necessity of bitcode generation?
I'm reluctant to go with anything that requires each existing
extension to be patched because that introduces a huge lag time for
this change to actually help anyone out.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
Hi,
On 2020-03-12 14:08:31 +0800, Craig Ringer wrote:
On Thu, 12 Mar 2020 at 03:43, Andres Freund <andres@anarazel.de> wrote:
On 2020-03-11 11:25:28 +0800, Craig Ringer wrote:
I propose that per the attached patch PGXS should simply skip adding
the automatic dependency for .bc files if clang cannot be found.
Extensions may still choose to explicitly declare the rule in their
own Makefile if they want to force bitcode generation.Hm, that seems like it could also cause silent failures (e.g. after a
package upgrade or such).How about erroring out, but with an instruction that llvm can be
disabled with make NO_LLVM=1 or such?I thought about that at first, but that'll only benefit people who're
hand-compiling things, and it's already possible withmake with_llvm=no ...
Well, the difference is that you'd be told about it, instead of getting
a hard to parse error message.
The proportion of people hand-compiling is an ever-shrinking
proportion of the user base.
Those not building themselves aren't going to care either.
When something's nested inside an rpm specfile inside a docker
container inside a bash script inside another Docker container on an
AWS instance .... not so fun. They might be able to inject it into the
environment. But often not.
Uh, I have very little pity with that argument. If you're script
building stuff, you can also specify the full dependencies.
Extensions that explicitly must generate bytecode may add their own
dependency rule.
You're just moving to per-extension work with that. The likely outcome
is that you're just not going to have bitcode for numerous extensions,
because there's nothing warning you against an incomplete setup. And
that hard dependency then also has to take into account whether PG was
built with llvm enabled or not. That's not a good direction.
Or we could make skipping bytecode generation if llvm cannot be found
at build-time something the extension can turn off with a PGXS option,
as suggested upthread.
See above.
I'm reluctant to go with anything that requires each existing
extension to be patched because that introduces a huge lag time for
this change to actually help anyone out.
That's precisely what you're proposing, though. Just in the inverse.
Greetings,
Andres Freund
On Thu, 12 Mar 2020 at 16:25, Andres Freund <andres@anarazel.de> wrote:
Hi,
On 2020-03-12 14:08:31 +0800, Craig Ringer wrote:
I thought about that at first, but that'll only benefit people who're
hand-compiling things, and it's already possible withmake with_llvm=no ...
Well, the difference is that you'd be told about it, instead of getting
a hard to parse error message.What about adding a WARNING but don't error out if LLVM isn't found? Add
an additional option (if LLVM isn't found) is annoying because it means
adding instruction into README of all extensions. What is the side effects
of not providing .bc files? It seems some extensions won't benefit from
LLVM.
Regards,
--
Euler Taveira http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Hi,
On 2020-03-12 17:22:09 -0300, Euler Taveira wrote:
On Thu, 12 Mar 2020 at 16:25, Andres Freund <andres@anarazel.de> wrote:
Hi,
On 2020-03-12 14:08:31 +0800, Craig Ringer wrote:
I thought about that at first, but that'll only benefit people who're
hand-compiling things, and it's already possible withmake with_llvm=no ...
Well, the difference is that you'd be told about it, instead of getting
a hard to parse error message.What about adding a WARNING but don't error out if LLVM isn't found? Add
an additional option (if LLVM isn't found) is annoying because it means
adding instruction into README of all extensions.
IMO only if the packager screwed up. The dependencies of the package
that includes pgxs, headers should have the dependencies to llvm. Which
e.g. debian's does:
$ apt show postgresql-server-dev-12
Package: postgresql-server-dev-12
Version: 12.2-1+b1
Priority: optional
Section: libdevel
Source: postgresql-12 (12.2-1)
Maintainer: Debian PostgreSQL Maintainers <team+postgresql@tracker.debian.org>
Installed-Size: 5,327 kB
Depends: clang-9, libpq-dev (>= 12~~), llvm-9-dev, postgresql-client-12, postgresql-common (>= 142~)
Breaks: postgresql-server-dev-all (<< 181~)
Homepage: http://www.postgresql.org/
Download-Size: 919 kB
I haven't looked up the dependencies for the rpm packages including the
headers.
It can make sense to split the *binary* packages so that the llvm
dependency is not incurred by default, even when the server was compiled
with LLVM support. But I see very little point in doing so for -dev[el]
packages.
What is the side effects of not providing .bc files?
Prevent JITing the functions in that extension?
It seems some extensions won't benefit from LLVM.
Sure, and those can disable it?
Greetings,
Andres Freund
Sorry, that mail is almost duplicate with another one, which was sent
by accident.
At Thu, 12 Mar 2020 14:59:44 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in
+1 for requiring such options for the same reason. The current patch
disables LLVM for the enviroment where clang is installed but ccache
is not, while building an extension based on postgresql-devel package.
(ccache is in EPEL on RHEL/CentOS.)A bit aside from LLVM itself, I'd like CC/CPP/CLANG to fall back to
them in the PATH on the running environment if they don't exist.
--
Kyotaro Horiguchi
NTT Open Source Software Center
On Fri, 13 Mar 2020 at 04:35, Andres Freund <andres@anarazel.de> wrote:
IMO only if the packager screwed up. The dependencies of the package
that includes pgxs, headers should have the dependencies to llvm. Which
e.g. debian's does:
Yes, I agree that the underlying issue is mainly with packaging.
This proposal came out of chasing down some packaging problems relating to
the EL-7 -devel packages for Pg 11 and 12, per linked mails in initial
post. They don't declare a runtime dependency on llvm toolset or clang, so
they're basically broken given the way we assume the presence of those
tools.
But
(a) do we really want to force everyone to pull in clang and the llvm
toolset when they install the -devel pkg, even if they don't install or
need the postgresqlNN-llvmjit package?
(b) EL-7 doesn't have a usable llvm/clang version even in EPEL, you have to
add a separate SCL LLVM toolset repo. So adding a dependency on
llvm-toolset into EL-7's postgresql11-devel and postgresql12-devel is most
undesirable, especially in a point release, as it'll make lots of stuff
explode messily.
I learned (b) the hard way. Don't do that.
If the consensus is that this is a packaging issue, (a) is fine, and we
should just quit whining and install a suitable clang/llvm, I'll cope with
that. I'll ask yum-packagers to patch Makefile.global for EL-7 with a
workaround like the one proposed here and for newer RH where a suitable
LLVM is available, just declare it as a dependency of the -devel pkg going
forward then make lots of noise about the change.
But the problem is that even for newer RH "enterprise" distros LLVM/clang
live in EPEL, and IIRC we don't presently require any dependencies from
EPEL to install the base postgresqlNN-* packages (except llvmjit). So we'd
be making EPEL a new repo dependency for the -devel pkg and that's not
something I'm too fond of doing in a minor release.
The alternative would be to detect a missing clang and emit a much more
informative error than the current one that explicitly suggests retrying
with
make with_llvm=no
or setting with_llvm=no in the environment.
The whole thing is a mess caused by this "enterprise-y" repository split
between core and "extras" and I'm rather frustrated by the whole thing, but
the current situation isn't much fun for users.
--
Craig Ringer http://www.2ndQuadrant.com/
2ndQuadrant - PostgreSQL Solutions for the Enterprise
On 2020-03-13 14:08:12 +0800, Craig Ringer wrote:
The alternative would be to detect a missing clang and emit a much more
informative error than the current one that explicitly suggests retrying
withmake with_llvm=no
or setting with_llvm=no in the environment.
That, that, that's what I suggested upthread?
On Fri, 13 Mar 2020 at 15:04, Andres Freund <andres@anarazel.de> wrote:
On 2020-03-13 14:08:12 +0800, Craig Ringer wrote:
The alternative would be to detect a missing clang and emit a much more
informative error than the current one that explicitly suggests retrying
withmake with_llvm=no
or setting with_llvm=no in the environment.
That, that, that's what I suggested upthread?
Yes, and I still don't like it. "with_llvm" is the actual already-working
option. I'd rather make this not randomly explode for users, but failing
that we can just hack the Makefile in the rpms for EL-7 (where it's a
particular mess) and rely on an error message for other cases.
--
Craig Ringer http://www.2ndQuadrant.com/
2ndQuadrant - PostgreSQL Solutions for the Enterprise
On 2020-03-15 02:28, Craig Ringer wrote:
On Fri, 13 Mar 2020 at 15:04, Andres Freund <andres@anarazel.de
<mailto:andres@anarazel.de>> wrote:On 2020-03-13 14:08:12 +0800, Craig Ringer wrote:
The alternative would be to detect a missing clang and emit a
much more
informative error than the current one that explicitly suggests
retrying
with
make with_llvm=no
or setting with_llvm=no in the environment.
That, that, that's what I suggested upthread?
Yes, and I still don't like it. "with_llvm" is the actual
already-working option. I'd rather make this not randomly explode for
users, but failing that we can just hack the Makefile in the rpms for
EL-7 (where it's a particular mess) and rely on an error message for
other cases.
I don't really get the problem. with_llvm=no works, so it can be used.
Options that automatically disable things based on what is installed in
the build environment are bad ideas. For instance, we on purpose don't
have configure decide anything based on whether readline is installed.
Either you select it or you don't, there is no "auto" mode.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Thu, 19 Mar 2020 at 18:47, Peter Eisentraut <
peter.eisentraut@2ndquadrant.com> wrote:
On 2020-03-15 02:28, Craig Ringer wrote:
On Fri, 13 Mar 2020 at 15:04, Andres Freund <andres@anarazel.de
<mailto:andres@anarazel.de>> wrote:On 2020-03-13 14:08:12 +0800, Craig Ringer wrote:
The alternative would be to detect a missing clang and emit a
much more
informative error than the current one that explicitly suggests
retrying
with
make with_llvm=no
or setting with_llvm=no in the environment.
That, that, that's what I suggested upthread?
Yes, and I still don't like it. "with_llvm" is the actual
already-working option. I'd rather make this not randomly explode for
users, but failing that we can just hack the Makefile in the rpms for
EL-7 (where it's a particular mess) and rely on an error message for
other cases.I don't really get the problem. with_llvm=no works, so it can be used.
Options that automatically disable things based on what is installed in
the build environment are bad ideas. For instance, we on purpose don't
have configure decide anything based on whether readline is installed.
Either you select it or you don't, there is no "auto" mode.
Fine with me. I wrote it before identifying that with_llvm=no was a viable
workaround.
The whole thing is a bit ugly, but if the fix isn't clearly better than the
problem the fix shouldn't go in. This way it'll be searchable-for at least.
I think we'll be adopting some kind of ugly workaround like this for the
CentOS 7 packages in PGDG yum because they're a bit of a special case,
since the llvm support requires an additional 3rd party that isn't declared
as a build-depend on the devel package. But that can be done at packaging
level + a small patch applied during package builds for CentOS 7 only.
Withdrawn.
--
Craig Ringer http://www.2ndQuadrant.com/
2ndQuadrant - PostgreSQL Solutions for the Enterprise