postgresql: split -lib and -dev outputs cleanly #294504
Conversation
wolfgangwalther
requested review from
jtojnar,
aanderse,
drupol,
globin,
Ma27,
talyz,
mweinelt,
thoughtpolice and
marsam
as code owners
github-actions
bot
added
6.topic: python
6.topic: nixos
|
Converting to a draft until the base PRs are merged. |
Yeah, from a first glance I prefer this PR over the alternatives. |
I don't think this Of course the fix is simple - just make |
I wonder how this would work before this PR anyway. Currently, those libraries are in the default output, but neither When I remove those libraries entirely, then No mention of the -dev output anywhere, but it still works fine. Without knowing exactly what the magic behind the scenes does here, I conclude that this will work just fine without pg_config / pkg-config returning another |
wolfgangwalther
force-pushed
the
postgresql-outputs
branch
from
912d957 to
080e95d
Compare
ofborg
bot
added
8.has: package (new)
wolfgangwalther
force-pushed
the
postgresql-outputs
branch
from
080e95d to
9900726
Compare
Yep, I came to the same conclusion a while back.
I don't think this is a great solution either. I would find it pretty surprising if installing Alternatively I could see
This is one of the reason I still think a separate |
|
Just realized I lost the commit to make other packages use the -dev output when manually looking for pg_config along the way, when splitting of the "make libpq a package" related changes. I'm working on a PR to fix that. |
This was supposed to happen in NixOS#294504, but the commit was accidentally left out when splitting off some libpq-related changes. Originated in NixOS#179962, by Sandro. Co-authored-by: Sandro Jäckel <sandro.jaeckel@gmail.com> Co-authored-by: Wolfgang Walther <walther@technowledgy.de>
|
Bisect says 435f51c |
|
Looks like the header needs an anchor. This seems to be enough to workaround build failure: --- a/nixos/modules/services/databases/postgresql.md
+++ b/nixos/modules/services/databases/postgresql.md
@@ -366 +366 @@ evaluates to `"foobar"`.
-## Notable differences to upstream
+## Notable differences to upstream {#foo} |
|
Proposed the fix as: |
We introduced LTO in NixOS#294504. At that time, we still needed to use LLVM / lld to make this work on darwin. For this to work for extensions, they would need to set CFLAGS=-fuse-ld=lld, too. However, since NixOS#307880 landed, we don't need to do this anymore in the first place, LTO just works out of the box on darwin. Resolves NixOS#342362
|
hi, I have a very strange performance regression after this PR, sorry cant provide sql file, but it's just a restore dump of somewhat small db ( 100Mb ) before this PR after this PR so far I've bisected it to this lines commenting those lines restores performance, no idea why. this is for non jit version of thoughts ? it would be good if someone else can verify this regression |
|
@kirillrdy Are you on darwin or linux?
Once you take this away, some other settings in the derivation will be affected (I can't pinpoint which exactly, will have to test). So this doesn't need to be about structuredAttrs directly, but possibly about something else that is disabled with it. |
thanks for quick response, i am on but no difference. |
|
Just to be clear, the absence of I must say, I guess I'll look into it because I really want to know how this could happen 🙃 |
|
@Ma27 thank you for showing interest in this base commit is 7797728, and commit without structured attrs is kirillrdy@5afb38a I had to comment out outputChecks.lib as well because it doesn't eval, but if you comment out just outputChecks.lib the regression is still there |
|
Thanks for the reproducer, that will save me quite a bit of time trying to set one up myself. Will try to look into that tomorrow as well. |
|
Congratulations, you've found a nasty stdenv bug. |
A while ago `postgresql` switched to using structured attrs[1]. In the
PR it was reported that this made postgresql notably slower when
importing SQL dumps[2].
After a bit of debugging it turned out that the hardening was entirely
missing and the following combination of settings was the culprit:
hardeningEnable = [ "pie" ];
__structuredAttrs = true;
I.e. the combination of custom hardening settings and structured attrs.
What happened here is that internally, the default and enabled hardening
flags get written into NIX_HARDENING_ENABLE. However, the value is a list
and the setting is not in the `env` section. This means that in the
structured-attrs case we get something like
declare -ax NIX_HARDENING_ENABLE=([0]="bindnow" [1]="format" [2]="fortify" [3]="fortify3" [4]="pic" [5]="relro" [6]="stackprotector" [7]="strictoverflow" [8]="zerocallusedregs" [9]="pie")
i.e. an actual array rather than a string with all hardening flags being
space-separated which is what the hardening code of the cc-wrapper
expects[3].
This only happens if `hardeningEnable` or `hardeningDisable` are
explicitly set by a derivation: if none of those are set,
`NIX_HARDENING_ENABLE` won't be set by `stdenv.mkDerivation` and the
default hardening flags are configured by the setup hook of the
cc-wrapper[4].
In other words, this _only_ applies to derivations that have both custom
hardening settings _and_ `__structuredAttrs = true;`.
I figured that it's far easier to just write a space-separated string
for NIX_HARDENING_ENABLE into `env` if needed rather than changing the
code in `add-hardening.sh` which would've caused a full rebuild. The
flags are well-known identifiers anyways, so there's no risk of issues
coming from missing escapes.
[1] NixOS#294504
[2] NixOS#294504 (comment)
[3] https://github.com/NixOS/nixpkgs/blob/cf3e5d3744dc26c3498aa5dadfa0e078c632cede/pkgs/build-support/cc-wrapper/add-hardening.sh#L9
[4] https://github.com/NixOS/nixpkgs/blob/cf3e5d3744dc26c3498aa5dadfa0e078c632cede/pkgs/build-support/cc-wrapper/setup-hook.sh#L114
|
See #353131. |
… true;` Replaces / Closes NixOS#353131 A while ago `postgresql` switched to using structured attrs[1]. In the PR it was reported that this made postgresql notably slower when importing SQL dumps[2]. After a bit of debugging it turned out that the hardening was entirely missing and the following combination of settings was the culprit: hardeningEnable = [ "pie" ]; __structuredAttrs = true; I.e. the combination of custom hardening settings and structured attrs. What happened here is that internally the default and enabled hardening flags get written into `NIX_HARDENING_ENABLE`. However, the value is a list and the setting is not in the `env` section. This means that in the structured-attrs case we get something like declare -ax NIX_HARDENING_ENABLE=([0]="bindnow" [1]="format" [2]="fortify" [3]="fortify3" [4]="pic" [5]="relro" [6]="stackprotector" [7]="strictoverflow" [8]="zerocallusedregs" [9]="pie") i.e. an actual array rather than a string with all hardening flags being space-separated which is what the hardening code of the cc-wrapper expects[3]. This only happens if `hardeningEnable` or `hardeningDisable` are explicitly set by a derivation: if none of those are set, `NIX_HARDENING_ENABLE` won't be set by `stdenv.mkDerivation` and the default hardening flags are configured by the setup hook of the cc-wrapper[4]. In other words, this _only_ applies to derivations that have both custom hardening settings _and_ `__structuredAttrs = true;`. All values of `NIX_HARDENING_ENABLE` are well-known, so we don't have to worry about escaping issues. Just forcing it to a string by concatenating the list everytime solves the issue without additional issues like eval errors when inheriting `env` from a structuredAttrs derivation[5]. The price we're paying is a full rebuild. [1] NixOS#294504 [2] NixOS#294504 (comment) [3] https://github.com/NixOS/nixpkgs/blob/cf3e5d3744dc26c3498aa5dadfa0e078c632cede/pkgs/build-support/cc-wrapper/add-hardening.sh#L9 [4] https://github.com/NixOS/nixpkgs/blob/cf3e5d3744dc26c3498aa5dadfa0e078c632cede/pkgs/build-support/cc-wrapper/setup-hook.sh#L114 [5] NixOS@1e84a7f
@wolfgangwalther did you manage to find a way to build |
Yes, see #345289. |
Thanks very much :) |
Description of changes
TLDR: This splits outputs for the postgresql package cleanly, using a minimum of patching and/or nuking of references. Instead, dead references to other store paths are removed at compile and link time. The minimal -lib output is then added as a top-level
libpqpackage.This builds on my previous work in #292993 and #293996 and contains commits from those two branches. Please ignore the first ~20 commits here and provide feedback for those in the two other PRs. This PR starts with
treewide: prepare pg_config moving to dev. This commit (by @jtojnar) and the 4 commits after (by @SuperSandro2000) were cherry-picked from #179962. Compared to #179962, the approach taken in this PR is simpler and requires far less patching of postgresql source code. In fact, the overall size of patches is reduced compared to before and fewer references needed to be removed afterwards.While the most work here involved splitting the -dev output, a clean -lib output comes with that naturally. Thus, this PR replaces #179962 and #273175. The cleaned up lib output was discussed in #61580 (comment) (@thoughtpolice). With this output available, it's easy to create a top-level attribute
libpqpointing to the latest version's lib output automatically as suggested by @danbst in #61580 (comment), which should resolve #61580 (@nh2). A true separate libpq package as proposed in #234470 (@szlend) would then not be necessary. This PR will be followed-up by another PR to build postgresql v16+ based on meson instead of autotools as mentioned in #292993 (comment). With a bit of patching (of which I have a working PoC already), this will allow us to build postgresql (at least libpq, but possibly even more) inpkgsStatic- which is probably the biggest thing that didn't work so far and why a separate libpq package was asked for in the first place.After the split, the outputs contain the following:
out:pg_configandecpgwhich are in the dev output. The default output thus includes both server-side and client-side binaries.lib:/share/localefolders, if there were any built, which is not the case right nowdocandmanas beforedev:/include(both server and client)pgxsinfrastructure to build extensionslibpgport*.a,libpgcommon*.aandlibpgfeutils.aecpgandpg_configbinaries as mentioned abovepg_configoutputs all paths correctly. Example:One thing that is still missing right now, is that theThis should work, see comment below. I think this question was also raised by @szlend in #273175 (comment).-dev/libfolder must be added to the pkg-config file, so thatlibpgcommon,libpgportetc. can be found. It's still unclear to me whether we need to add this to some other parts of thepg_configoutput, too, to make it possible for pg_config-based builds to actually find those static libraries.Maintainer ping, if not mentioned above already: @globin @marsam @ivan @Ma27
Things done
I built all 5 versions (12-16) with and without JIT support, with glibc (default) and via
pkgsMusl. I also built.testsfor the glibc variants (musl didn't work for me for unrelated reasons). I built all extensions with various versions (which resulted in #294457). This works very well, so far.What I did not build, yet, is any downstream packages actually depending on
postgresqland the new -dev output. This will be the next step, but I would like to put this PR out for feedback already.nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.