Thread: [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 https://www.postgresql.org/message-id/CAMsr+YHokx0rWLV561z3=gAi6CM4YJekgCLkqmDwQSTEjVYhuw@mail.gmail.com . -- Craig Ringer http://www.2ndQuadrant.com/ 2ndQuadrant - PostgreSQL Solutions for the Enterprise
Attachment
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.
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 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. 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 with > > make 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 with
>
> make 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
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 with > > > > > > make 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.
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?
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
> with
>
> make with_llvm=no
>
> or setting with_llvm=no in the environment.
That, that, that's what I suggested upthread?
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.