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: Make systemd support configurable #61581

Merged

Conversation

nh2
Copy link
Contributor

@nh2 nh2 commented May 16, 2019

Motivation for this change

See #61580.

On by default, as was before. No evaluations should change.

The systemd dependency means that all libraries or applications using only libpq
will also pull systemd into their closures.

Further, no application or library using libpq can be built against musl, as
systemd relies on glibc-only features and thus does not build with musl.

With it being configurable, packages that need only the library can at
least turn it off to reduce their closure size.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@nh2 nh2 requested review from matthewbauer and domenkozar May 16, 2019 03:20
@nh2 nh2 requested a review from thoughtpolice as a code owner May 16, 2019 03:20
@nh2 nh2 mentioned this pull request May 16, 2019
4 tasks
@thoughtpolice
Copy link
Member

thoughtpolice commented May 16, 2019

Based on the discussion in #61580, I'd restructure this all to look something like this:

{ version
, useSystemd ? true
...
}:

let
  # systemd doesn't build with musl, and can't work on darwin
  # see #61580 for information & discussion about musl support
  systemdCompatibleEnv = !stdenv.isDarwin && !stdenv.hostPlatform.isMusl;

  # only postgresql 9.6 and later support systemd notifications
  systemdServerSupport = stdenv.lib.versionAtLeast "9.6" version;

  # enable if asked, iff the prior conditions are met
  shouldEnableSystemd = useSystemd && systemdCompatibleEnv && systemdServerSupport;
in
  # ...

  buildInputs = ... lib.optional shouldEnableSystemd [ systemd ] ...

Copy link
Member

@thoughtpolice thoughtpolice left a comment

Choose a reason for hiding this comment

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

(See prior notes)

@thoughtpolice
Copy link
Member

thoughtpolice commented May 16, 2019

And, actually: in this case, of adopting my suggested change from the last comment -- I probably would just not even include useSystemd ? false as a parameter, since there is, AFAICS, little reason to avoid it otherwise.

I wouldn't block this change if it was left in -- it's just that I don't think it's very useful at all, given the proposed change, and if we add it, we can't easily remove it later since someone might use it (even for odd or strange reasons), effectively locking us into supporting it.

On by default, as was before.

The systemd dependency means that all libraries or applications using only libpq
will also pull systemd into their closures.

Further, no application or library using libpq can be built against musl, as
systemd relies on glibc-only features and thus does not build with musl.

With it being configurable, packages that need only the library can at
least turn it off to reduce their closure size.
@nh2 nh2 force-pushed the issue-61580-postgresql-systemd-configurable branch from c234313 to 549ad30 Compare May 17, 2019 03:58
@ofborg ofborg bot requested review from danbst, ocharles and thoughtpolice May 17, 2019 04:42
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 501+ 10.rebuild-linux: 1001-2500 labels May 17, 2019
Copy link
Contributor

@danbst danbst left a comment

Choose a reason for hiding this comment

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

This shouldn't harm IMO.

@@ -6,6 +6,9 @@ let
, glibc, zlib, readline, openssl, icu, systemd, libossp_uuid
, pkgconfig, libxml2, tzdata

# This is important to obtain a version of `libpq` that does not depend on systemd.
, enableSystemd ? (lib.versionAtLeast "9.6" version && !stdenv.isDarwin)
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe also add an assert that systemd can't be null when enableSystemd = true. Though this is minor

@danbst danbst merged commit 943baed into NixOS:master Jul 21, 2019
@fpletz
Copy link
Member

fpletz commented Jul 23, 2019

This breaks systemd support for postgresql on master resulting in the systemd service being killed due to timeout because postgresql won't notify systemd, see this journal log:

1563849511.732376 trolovo systemd[1]: Starting PostgreSQL Server...
1563849511.786683 trolovo g8fqi5ssqms72x760xg3njcgncp1b27w-unit-script-postgresql-start[19562]: LOG:  database system was shut down at 2019-07-23 02:34:59 GMT
1563849511.791724 trolovo g8fqi5ssqms72x760xg3njcgncp1b27w-unit-script-postgresql-start[19562]: LOG:  MultiXact member wraparound protections are now enabled
1563849511.792795 trolovo g8fqi5ssqms72x760xg3njcgncp1b27w-unit-script-postgresql-start[19562]: LOG:  database system is ready to accept connections
1563849511.793021 trolovo g8fqi5ssqms72x760xg3njcgncp1b27w-unit-script-postgresql-start[19562]: LOG:  autovacuum launcher started
1563849631.761013 trolovo systemd[1]: postgresql.service: Start operation timed out. Terminating.
1563849631.763162 trolovo g8fqi5ssqms72x760xg3njcgncp1b27w-unit-script-postgresql-start[19562]: LOG:  received fast shutdown request
1563849631.763162 trolovo g8fqi5ssqms72x760xg3njcgncp1b27w-unit-script-postgresql-start[19562]: LOG:  aborting any active transactions
1563849631.763837 trolovo g8fqi5ssqms72x760xg3njcgncp1b27w-unit-script-postgresql-start[19562]: LOG:  autovacuum launcher shutting down
1563849631.763948 trolovo g8fqi5ssqms72x760xg3njcgncp1b27w-unit-script-postgresql-start[19562]: LOG:  shutting down
1563849631.809723 trolovo g8fqi5ssqms72x760xg3njcgncp1b27w-unit-script-postgresql-start[19562]: LOG:  database system is shut down
1563849631.810316 trolovo systemd[1]: postgresql.service: Failed with result 'timeout'.
1563849631.810877 trolovo systemd[1]: Failed to start PostgreSQL Server.

@fpletz
Copy link
Member

fpletz commented Jul 23, 2019

Fixed on master (290cfc7). versionAtLeast was called with arguments in the wrong order.

Note that even the NixOS tests for postgresql fail with the original change that was merged. Please run NixOS tests as part of PR reviews.

pull bot pushed a commit to avitex/nixpkgs that referenced this pull request Jul 23, 2019
See comments in NixOS#61581. versionAtLeast was called with arguments in the
wrong order.
@danbst
Copy link
Contributor

danbst commented Jul 23, 2019

sorry, will do

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 501+ 10.rebuild-linux: 1001-2500
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants