Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

postgresql: implement opt-in JIT support #221851

Merged
merged 6 commits into from
Mar 29, 2023

Conversation

Ma27
Copy link
Member

@Ma27 Ma27 commented Mar 18, 2023

Description of changes

Closes #150801

Note: I decided against resuming directly on #150801 because the conflict was too big (and resolving it seemed too error-prone to me). Also the this-refactoring could be done in an easier manner, i.e. by exposing JIT attributes with the correct configuration. More on that below.

This patch creates variants of the postgresql*-packages with JIT[1] support. Please note that a lot of the work was derived from previous patches filed by other contributors, namely dasJ, andir and abbradar, hence the co-authored-by tags below.

Effectively, the following things have changed:

  • For JIT variants an LLVM-backed stdenv with clang is now used as
    suggested by dasJ[2]. We need LLVM and CLang[3] anyways to build the
    JIT-part, so no need to mix this up with GCC's stdenv. Also, using the
    dev-output of LLVM and clang's stdenv for building (and adding llvm
    libs as build-inputs) seems more cross friendly to me (which will
    become useful when cross-building for JIT-variants will actually be
    supported).

  • Plugins inherit the build flags from the Makefiles in $out/lib/pgxs/src (e.g. -Werror=unguarded-availability-new). Since some of the flags are clang-specific (and stem from the use of the CLang stdenv) and don't work on gcc, the stdenv of pkgs.postgresql is passed to the plugins. I.e., plugins for non-JIT variants are built with a gcc stdenv on Linux and plugins for JIT variants with a clang stdenv.

    Since plv8 hard-codes gcc as $CC in its Makefile[4], I marked it as broken for JIT-variants of postgresql only.

  • Added a test-matrix to confirm that JIT works fine on each pkgs.postgresql_*_jit (thanks Andi for the original test in postgresql: support JIT compilation for version >= 11 #124804!).

  • For each postgresql version, a new attribute postgresql_<version>_jit (and a corresponding postgresqlPackages<version>JitPackages) are now exposed for better discoverability and prebuilt artifacts in the binary cache.

  • In postgresql: support JIT #150801 the this-argument was replaced by an internal recursion. I decided against this approach because it'd blow up the diff even more which makes the readability way harder and also harder to revert this if necessary.

    Instead, it is made sure that this always points to the correct variant of postgresql and re-using that in an additional .override {}-expression is trivial because the JIT-variant is exposed in all-packages.nix.

  • I think the changes are sufficiently big to actually add myself as maintainer here.

  • Added libxcrypt to buildInputs for versions <v13. While building things with an LLVM stdenv, these versions complained that the extern crypt() symbol can't be found. Not sure what this is exactly about, but since we want to switch to libxcrypt for crypt() usage anyways[5] I decided to add it. For >=13 it's not relevant anymore anyways[6].

  • JIT support doesn't work with cross-compilation. It is attempted to build LLVM-bytecode
    (%.bc is the corresponding make(1)-rule) for each sub-directory in backend/ for
    the JIT apparently, but with a $(CLANG) that can produce binaries for the build, not the
    host-platform.

    I managed to get a cross-build with JIT support working with
    depsBuildBuild = [ llvmPackages.clang ] ++ buildInputs, but considering that the
    resulting LLVM IR isn't platform-independent this doesn't give you much.
    In fact, I tried to test the result in a VM-test, but as soon as JIT was used to optimize
    a query, postgres would coredump with Illegal instruction.

A common concern of the original approach - with llvm as build input - was the massive increase of closure size. With the new approach of using the LLVM stdenv directly and patching out references to the clang drv in $out the effective closure size changes are:

$ nix path-info -Sh $(nix-build -A postgresql_14)
/nix/store/kssxxqycwa3c7kmwmykwxqvspxxa6r1w-postgresql-14.7	306.4M
$ nix path-info -Sh $(nix-build -A postgresql_14_jit)
/nix/store/xc7qmgqrn4h5yr4vmdwy56gs4bmja9ym-postgresql-14.7	689.2M

Most of the increase in closure-size stems from the lib-output of LLVM

$ nix path-info -Sh /nix/store/5r97sbs5j6mw7qnbg8nhnq1gad9973ap-llvm-11.1.0-lib
/nix/store/5r97sbs5j6mw7qnbg8nhnq1gad9973ap-llvm-11.1.0-lib	349.8M

which is why this shouldn't be enabled by default.

While this is quite much because of LLVM, it's still a massive improvement over the simple approach of adding llvm/clang as build-inputs and building with --with-llvm:

$ nix path-info -Sh $(nix-build -E '
with import ./. {};
postgresql.overrideAttrs ({ configureFlags ? [], buildInputs ? [], ... }: {
  configureFlags = configureFlags ++ [ "--with-llvm" ];
  buildInputs = buildInputs ++ [ llvm clang ];
})' -j0)
/nix/store/i3bd2r21c6c3428xb4gavjnplfqxn27p-postgresql-14.7	  1.6G

Co-authored-by: Andreas Rammhold andreas@rammhold.de
Co-authored-by: Janne Heß janne@hess.ooo
Co-authored-by: Nikolay Amiantov ab@fmap.me

[1] https://www.postgresql.org/docs/current/jit-reason.html [2] #124804 (comment)
& #150801 (comment)
[3] This fails with the following error otherwise:
configure: error: clang not found, but required when compiling --with-llvm, specify with CLANG=
[4] https://github.com/plv8/plv8/blob/v3.1.5/Makefile#L14 [5] #181764
[6] postgres/postgres@c45643d

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.05 Release Notes (or backporting 22.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

@Ma27
Copy link
Member Author

Ma27 commented Mar 18, 2023

@ofborg build postgresql_jit

@Ma27
Copy link
Member Author

Ma27 commented Mar 19, 2023

So, darwin is failing for the JIT variant, but appears to be working fine for the non-JIT version, so existing users won't be affected.

I guess the issue is fixable rather trivially, but I don't own a mac and I don't want to pay cloud-providers for that (this is something I implemented in my spare-time!), so I'm inclined to mark it as broken and wait for someone who's interested and has the resources to fix it unless somebody can give me a darwin environment.

@mweinelt
Copy link
Member

There exists a community-maintained darwin builder at https://github.com/winterqt/darwin-build-box.

Ma27 added a commit to Ma27/darwin-build-box that referenced this pull request Mar 19, 2023
Even though I don't do much darwin-related stuff, it's sometimes useful
to have such a box, most recent case is the JIT support for
postgresql[1].

Note: I purposely added uid 515 because nix-community#12 and nix-community#14 are in front of me
in the queue ;-)

[1] NixOS/nixpkgs#221851 (comment)
Ma27 added a commit to Ma27/darwin-build-box that referenced this pull request Mar 19, 2023
Even though I don't do much darwin-related stuff, it's sometimes useful
to have such a box, most recent case is the JIT support for
postgresql[1].

[1] NixOS/nixpkgs#221851 (comment)
@RaitoBezarius
Copy link
Member

RaitoBezarius commented Mar 19, 2023

Testing this on a Linux PostgreSQL-based Nextcloud personal production.

Copy link
Member

@RaitoBezarius RaitoBezarius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be interesting if there was a way to enable JIT in the NixOS module without having to find out the existing PostgreSQL version wrt to stateVersion.

@Ma27
Copy link
Member Author

Ma27 commented Mar 29, 2023

I think this is in a pretty usable state now, so I'll ask the other way round now, any objections to merge it as-is?

cc @SuperSandro2000 @mweinelt @RaitoBezarius @fpletz

@ofborg ofborg bot requested a review from marsam March 29, 2023 06:57
Copy link
Member

@ajs124 ajs124 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

didn't test, but package changes look similar enough to what we're running and the rest looks reasonable as well

@SuperSandro2000
Copy link
Member

SuperSandro2000 commented Mar 29, 2023

Since this is not triggering rebuilds in libpq, we had a fair amount of reviews and jit is off by default, I am going ahead and merge it.

@SuperSandro2000 SuperSandro2000 merged commit c2ae278 into NixOS:master Mar 29, 2023
@Ma27 Ma27 deleted the postgresql-jit-support branch March 29, 2023 11:41
wolfgangwalther added a commit to wolfgangwalther/nixpkgs that referenced this pull request Mar 15, 2024
This was proposed by abbradar in NixOS#150801, but left out of the follow up PR
NixOS#221851 by Ma27 to reduce the size of the diff. Compared to the initial
proposal this includes the callPackage call in the recursion, which avoids
breaking the withJIT/withoutJIT helpers.

In terms of nixpkgs, this is a pure refactor, no derivations change. However,
this makes downstream expressions like the following possible:

  (postgresql.override { jitSupport = true; }).pkgs.postgis

This would have not worked before without passing another "this" argument,
which is error prone as can be seen in this example:

  https://github.com/PostgREST/postgrest/pull/3222/files
wolfgangwalther added a commit to wolfgangwalther/nixpkgs that referenced this pull request Apr 4, 2024
The enableJIT = true case was fixed in NixOS#221851 or
e2fb651 respectively.

However this did not take the case into consideration, when doing this:

    services.postgresql = {
      enable = true;
      enableJIT = false;
      package = pkgs.postgresql_15_jit;
    };

If enableJIT is treated as the source of truth, then this should indeed
cause JIT to be disabled, which this commit does.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants