From 2282fa73a1d654c3c7b7429d8de49947c11bd0e1 Mon Sep 17 00:00:00 2001 From: Maximilian Bosch Date: Sat, 18 Mar 2023 09:54:54 +0100 Subject: postgresql: implement opt-in JIT support MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 #124804!). * For each postgresql version, a new attribute `postgresql__jit` (and a corresponding `postgresqlPackagesJitPackages`) are now exposed for better discoverability and prebuilt artifacts in the binary cache. * In #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 =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 Co-authored-by: Janne Heß Co-authored-by: Nikolay Amiantov [1] https://www.postgresql.org/docs/current/jit-reason.html [2] https://github.com/NixOS/nixpkgs/pull/124804#issuecomment-864616931 & https://github.com/NixOS/nixpkgs/pull/150801#issuecomment-1467868321 [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] https://github.com/NixOS/nixpkgs/pull/181764 [6] https://github.com/postgres/postgres/commit/c45643d618e35ec2fe91438df15abd4f3c0d85ca --- nixos/tests/all-tests.nix | 1 + nixos/tests/postgresql-jit.nix | 50 +++++++++++++++++++++++++++++++++ nixos/tests/postgresql-wal-receiver.nix | 2 +- 3 files changed, 52 insertions(+), 1 deletion(-) create mode 100644 nixos/tests/postgresql-jit.nix (limited to 'nixos/tests') diff --git a/nixos/tests/all-tests.nix b/nixos/tests/all-tests.nix index 2c34a3996d0bc..f1141667ce0fc 100644 --- a/nixos/tests/all-tests.nix +++ b/nixos/tests/all-tests.nix @@ -561,6 +561,7 @@ in { postfixadmin = handleTest ./postfixadmin.nix {}; postgis = handleTest ./postgis.nix {}; postgresql = handleTest ./postgresql.nix {}; + postgresql-jit = handleTest ./postgresql-jit.nix {}; postgresql-wal-receiver = handleTest ./postgresql-wal-receiver.nix {}; powerdns = handleTest ./powerdns.nix {}; powerdns-admin = handleTest ./powerdns-admin.nix {}; diff --git a/nixos/tests/postgresql-jit.nix b/nixos/tests/postgresql-jit.nix new file mode 100644 index 0000000000000..21682aad96da9 --- /dev/null +++ b/nixos/tests/postgresql-jit.nix @@ -0,0 +1,50 @@ +{ system ? builtins.currentSystem +, config ? {} +, pkgs ? import ../.. { inherit system config; } +}: + +with import ../lib/testing-python.nix { inherit system pkgs; }; + +let + inherit (pkgs) lib; + packages = lib.filter + (lib.hasSuffix "_jit") + (builtins.attrNames (import ../../pkgs/servers/sql/postgresql pkgs)); + + mkJitTest = packageName: makeTest { + name = "${packageName}"; + meta.maintainers = with lib.maintainers; [ ma27 ]; + nodes.machine = { pkgs, lib, ... }: { + services.postgresql = { + enable = true; + package = pkgs.${packageName}; + settings.jit = "on"; + initialScript = pkgs.writeText "init.sql" '' + create table demo (id int); + insert into demo (id) select generate_series(1, 5); + ''; + }; + }; + testScript = '' + machine.start() + machine.wait_for_unit("postgresql.service") + + with subtest("JIT is enabled"): + machine.succeed("sudo -u postgres psql <<<'show jit;' | grep 'on'") + + with subtest("Test JIT works fine"): + output = machine.succeed( + "cat ${pkgs.writeText "test.sql" '' + set jit_above_cost = 1; + EXPLAIN ANALYZE SELECT CONCAT('jit result = ', SUM(id)) FROM demo; + SELECT CONCAT('jit result = ', SUM(id)) from demo; + ''} | sudo -u postgres psql" + ) + assert "JIT:" in output + assert "jit result = 15" in output + + machine.shutdown() + ''; + }; +in +lib.genAttrs packages mkJitTest diff --git a/nixos/tests/postgresql-wal-receiver.nix b/nixos/tests/postgresql-wal-receiver.nix index ae2708546f5db..b0bd7711dbcd9 100644 --- a/nixos/tests/postgresql-wal-receiver.nix +++ b/nixos/tests/postgresql-wal-receiver.nix @@ -116,4 +116,4 @@ let }; # Maps the generic function over all attributes of PostgreSQL packages -in builtins.listToAttrs (map makePostgresqlWalReceiverTest (builtins.attrNames (import ../../pkgs/servers/sql/postgresql { }))) +in builtins.listToAttrs (map makePostgresqlWalReceiverTest (builtins.attrNames (import ../../pkgs/servers/sql/postgresql pkgs))) -- cgit 1.4.1 From 608cb37533f25a049d9363f7053890fdab6076eb Mon Sep 17 00:00:00 2001 From: Maximilian Bosch Date: Mon, 20 Mar 2023 13:25:13 +0100 Subject: nixos/tests/postgresql: fix deprecation warning --- nixos/tests/postgresql.nix | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'nixos/tests') diff --git a/nixos/tests/postgresql.nix b/nixos/tests/postgresql.nix index 7e0a82c388288..b44849e0a14e5 100644 --- a/nixos/tests/postgresql.nix +++ b/nixos/tests/postgresql.nix @@ -137,7 +137,7 @@ let maintainers = [ zagy ]; }; - machine = {...}: + nodes.machine = {...}: { services.postgresql = { enable = true; -- cgit 1.4.1 From e2fb65175228a992f196f3b1700a53e18602e7f6 Mon Sep 17 00:00:00 2001 From: Maximilian Bosch Date: Sat, 25 Mar 2023 11:34:24 +0100 Subject: nixos/postgresql: fix enableJIT Make sure that JIT is actually available when using services.postgresql = { enable = true; enableJIT = true; package = pkgs.postgresql_15; }; The current behavior is counter-intuitive because the docs state that `enableJIT = true;` is sufficient even though it wasn't in that case because the declared package doesn't have the LLVM dependency. Fixed by using `package.withJIT` if `enableJIT = true;` and `package.jitSupport` is `false`. Also updated the postgresql-jit test to test for that case. --- nixos/modules/services/databases/postgresql.nix | 13 +++++++++++-- nixos/tests/postgresql-jit.nix | 6 ++---- 2 files changed, 13 insertions(+), 6 deletions(-) (limited to 'nixos/tests') diff --git a/nixos/modules/services/databases/postgresql.nix b/nixos/modules/services/databases/postgresql.nix index 16f76248b3a0d..3d55995aba055 100644 --- a/nixos/modules/services/databases/postgresql.nix +++ b/nixos/modules/services/databases/postgresql.nix @@ -7,9 +7,18 @@ let cfg = config.services.postgresql; postgresql = + let + # ensure that + # services.postgresql = { + # enableJIT = true; + # package = pkgs.postgresql_; + # }; + # works. + base = if cfg.enableJIT && !cfg.package.jitSupport then cfg.package.withJIT else cfg.package; + in if cfg.extraPlugins == [] - then cfg.package - else cfg.package.withPackages (_: cfg.extraPlugins); + then base + else base.withPackages (_: cfg.extraPlugins); toStr = value: if true == value then "yes" diff --git a/nixos/tests/postgresql-jit.nix b/nixos/tests/postgresql-jit.nix index 21682aad96da9..baf26b8da2b39 100644 --- a/nixos/tests/postgresql-jit.nix +++ b/nixos/tests/postgresql-jit.nix @@ -7,9 +7,7 @@ with import ../lib/testing-python.nix { inherit system pkgs; }; let inherit (pkgs) lib; - packages = lib.filter - (lib.hasSuffix "_jit") - (builtins.attrNames (import ../../pkgs/servers/sql/postgresql pkgs)); + packages = builtins.attrNames (import ../../pkgs/servers/sql/postgresql pkgs); mkJitTest = packageName: makeTest { name = "${packageName}"; @@ -17,8 +15,8 @@ let nodes.machine = { pkgs, lib, ... }: { services.postgresql = { enable = true; + enableJIT = true; package = pkgs.${packageName}; - settings.jit = "on"; initialScript = pkgs.writeText "init.sql" '' create table demo (id int); insert into demo (id) select generate_series(1, 5); -- cgit 1.4.1