Обсуждение: [MASSMAIL]Requiring LLVM 14+ in PostgreSQL 18
Hi PostgreSQL 18 will ship after these vacuum horizon systems reach EOL[1]: animal | arch | llvm_version | os | os_release | end_of_support ---------------+---------+--------------+--------+------------+---------------- branta | s390x | 10.0.0 | Ubuntu | 20.04 | 2025-04-01 splitfin | aarch64 | 10.0.0 | Ubuntu | 20.04 | 2025-04-01 urutau | s390x | 10.0.0 | Ubuntu | 20.04 | 2025-04-01 massasauga | aarch64 | 11.1.0 | Amazon | 2 | 2025-06-30 snakefly | aarch64 | 11.1.0 | Amazon | 2 | 2025-06-30 Therefore, some time after the tree re-opens for hacking, we could rip out a bunch of support code for LLVM 10-13, and then rip out support for pre-opaque-pointer mode. Please see attached. [1] https://www.postgresql.org/message-id/CA%2BhUKG%2B-g61yq7Ce4aoZtBDO98b4GXH8Cu3zxVk-Zn1Vh7TKpA%40mail.gmail.com
Вложения
On Wed, Apr 10, 2024 at 1:38 PM Thomas Munro <thomas.munro@gmail.com> wrote: > Therefore, some time after the tree re-opens for hacking, we could rip > out a bunch of support code for LLVM 10-13, and then rip out support > for pre-opaque-pointer mode. Please see attached. ... or of course closer to the end of the cycle if that's what people prefer for some reason, I don't mind too much as long as it happens. I added this to the commitfest app, and it promptly failed for cfbot. That's expected: CI is still using Debian 11 "bullseye", which only has LLVM 11. It became what Debian calls "oldstable" last year, and reaches the end of oldstable in a couple of months from now. Debian 12 "bookworm" is the current stable release, and it has LLVM 14, so we should probably go and update those CI images...
Rebased over ca89db5f. I looked into whether we could drop the "old pass manager" code too[1]. Almost, but nope, even the C++ API lacks a way to set the inline threshold before LLVM 16, so that would cause a regression. Although we just hard-code the threshold to 512 with a comment that sounds like it's pretty arbitrary, a change to the default (225?) would be unjustifiable just for code cleanup. Oh well. [1] https://github.com/macdice/postgres/commit/0d40abdf1feb75210c3a3d2a35e3d6146185974c
Вложения
On 24.04.24 01:43, Thomas Munro wrote: > Rebased over ca89db5f. These patches look fine to me. The new cut-off makes sense, and it does save quite a bit of code. We do need to get the Cirrus CI Debian images updated first, as you had already written. As part of this patch, you also sneak in support for LLVM 18 (llvm-config-18, clang-18 in configure). Should this be a separate patch? And as I'm looking up how this was previously handled, I notice that this list of clang-NN versions was last updated equally sneakily as part of your patch to trim off LLVM <10 (820b5af73dc). I wonder if the original intention of that configure code was that maintaining the versioned list above clang-7/llvm-config-7 was not needed, because the unversioning programs could be used, or maybe because pkg-config could be used. It would be nice if we could get rid of having to update that.
On Mon, May 13, 2024 at 2:33 AM Peter Eisentraut <peter@eisentraut.org> wrote: > These patches look fine to me. The new cut-off makes sense, and it does > save quite a bit of code. We do need to get the Cirrus CI Debian images > updated first, as you had already written. Thanks for looking! > As part of this patch, you also sneak in support for LLVM 18 > (llvm-config-18, clang-18 in configure). Should this be a separate patch? Yeah, right, I didn't really think too hard about why we have that, and now that you question it... > And as I'm looking up how this was previously handled, I notice that > this list of clang-NN versions was last updated equally sneakily as part > of your patch to trim off LLVM <10 (820b5af73dc). I wonder if the > original intention of that configure code was that maintaining the > versioned list above clang-7/llvm-config-7 was not needed, because the > unversioning programs could be used, or maybe because pkg-config could > be used. It would be nice if we could get rid of having to update that. I probably misunderstood why we were doing that, perhaps something to do with the way some distro (Debian?) was doing things with older versions, and yeah I see that we went a long time after 7 without touching it and nobody cared. Yeah, it would be nice to get rid of it. Here's a patch. Meson didn't have that.
Вложения
On 15.05.24 06:21, Thomas Munro wrote: >> And as I'm looking up how this was previously handled, I notice that >> this list of clang-NN versions was last updated equally sneakily as part >> of your patch to trim off LLVM <10 (820b5af73dc). I wonder if the >> original intention of that configure code was that maintaining the >> versioned list above clang-7/llvm-config-7 was not needed, because the >> unversioning programs could be used, or maybe because pkg-config could >> be used. It would be nice if we could get rid of having to update that. > I probably misunderstood why we were doing that, perhaps something to > do with the way some distro (Debian?) was doing things with older > versions, and yeah I see that we went a long time after 7 without > touching it and nobody cared. Yeah, it would be nice to get rid of > it. Here's a patch. Meson didn't have that. Yes, let's get that v3-0001 patch into PG17.
On Wed, May 15, 2024 at 5:20 PM Peter Eisentraut <peter@eisentraut.org> wrote: > Yes, let's get that v3-0001 patch into PG17. Done. Bilal recently created the CI images for Debian Bookworm[1]. You can try them with s/bullseye/bookworm/ in .cirrus.tasks.yml, but it looks like he is still wrestling with a perl installation problem[2] in the 32 bit build, so here is a temporary patch to do that and also delete the 32 bit tests for now. This way cfbot should succeed with the remaining patches. Parked here for v18. [1] https://github.com/anarazel/pg-vm-images/commit/685ca7ccb7b3adecb11d948ac677d54cd9599e6c [2] https://cirrus-ci.com/task/5459439048720384
Вложения
Hi, On Thu, 16 May 2024 at 05:34, Thomas Munro <thomas.munro@gmail.com> wrote: > > On Wed, May 15, 2024 at 5:20 PM Peter Eisentraut <peter@eisentraut.org> wrote: > > Yes, let's get that v3-0001 patch into PG17. > > Done. > > Bilal recently created the CI images for Debian Bookworm[1]. You can > try them with s/bullseye/bookworm/ in .cirrus.tasks.yml, but it looks > like he is still wrestling with a perl installation problem[2] in the > 32 bit build, so here is a temporary patch to do that and also delete > the 32 bit tests for now. This way cfbot should succeed with the > remaining patches. Parked here for v18. Actually, 32 bit builds are working but the Perl version needs to be updated to 'perl5.36-i386-linux-gnu' in .cirrus.tasks.yml. I changed 0001 with the working version of 32 bit builds [1] and the rest is the same. All tests pass now [2]. [1] postgr.es/m/CAN55FZ0fY5EFHXLKCO_=p4pwFmHRoVom_qSE_7B48gpchfAqzw@mail.gmail.com [2] https://cirrus-ci.com/task/4969910856581120 -- Regards, Nazir Bilal Yavuz Microsoft
Вложения
On Fri, May 17, 2024 at 3:17 AM Nazir Bilal Yavuz <byavuz81@gmail.com> wrote: > Actually, 32 bit builds are working but the Perl version needs to be > updated to 'perl5.36-i386-linux-gnu' in .cirrus.tasks.yml. I changed > 0001 with the working version of 32 bit builds [1] and the rest is the > same. All tests pass now [2]. Ahh, right, thanks! I will look at committing your CI/fixup patches.
On Wed, May 15, 2024 at 07:20:09AM +0200, Peter Eisentraut wrote: > Yes, let's get that v3-0001 patch into PG17. Upon seeing this get committed in 4dd29b6833, I noticed that the docs still advertise the llvm-config-$version search dance. That's still correct for Meson-based builds since we use their config-tool machinery, but no longer holds for configure-based builds. The attached patch updates the docs accordingly. -- Ole Peder Brandtzæg In any case, these nights just ain't getting any easier And who could judge us For seeking comfort in the hazy counterfeit land of memory
Вложения
On Sun, May 19, 2024 at 10:46 AM Ole Peder Brandtzæg <olebra@samfundet.no> wrote: > On Wed, May 15, 2024 at 07:20:09AM +0200, Peter Eisentraut wrote: > > Yes, let's get that v3-0001 patch into PG17. > > Upon seeing this get committed in 4dd29b6833, I noticed that the docs > still advertise the llvm-config-$version search dance. That's still > correct for Meson-based builds since we use their config-tool machinery, > but no longer holds for configure-based builds. The attached patch > updates the docs accordingly. Oops, right I didn't know we had that documented. Thanks. Will hold off doing anything until the thaw. Hmm, I also didn't know that Meson had its own list like our just-removed one: https://github.com/mesonbuild/meson/blob/master/mesonbuild/environment.py#L183 Unsurprisingly, it suffers from maintenance lag, priority issues etc (new major versions pop out every 6 months): https://github.com/mesonbuild/meson/issues/10483
On Sun, May 19, 2024 at 11:05:49AM +1200, Thomas Munro wrote: > Oops, right I didn't know we had that documented. Thanks. Will hold > off doing anything until the thaw. No worries, thanks! > Hmm, I also didn't know that Meson had its own list like our just-removed one: > > https://github.com/mesonbuild/meson/blob/master/mesonbuild/environment.py#L183 I didn't either before writing the doc patch, which led me to investigate why it *just works* when doing meson setup and then I saw the 40 odd "Trying a default llvm-config fallback…" lines in meson-log.txt =) -- Ole Peder Brandtzæg It's raining triple sec in Tchula and the radio plays "Crazy Train"
Thomas Munro <thomas.munro@gmail.com> writes:
> Oops, right I didn't know we had that documented. Thanks. Will hold
> off doing anything until the thaw.
FWIW, I don't think the release freeze precludes docs-only fixes.
But if you prefer to sit on this, that's fine too.
regards, tom lane