Skip to content

postgresqlTestHook: add sandbox profile for Darwin#383377

Draft
tie wants to merge 3 commits intoNixOS:masterfrom
tie:postgresql-test-hook-darwin-sandbox
Draft

postgresqlTestHook: add sandbox profile for Darwin#383377
tie wants to merge 3 commits intoNixOS:masterfrom
tie:postgresql-test-hook-darwin-sandbox

Conversation

@tie
Copy link
Member

@tie tie commented Feb 19, 2025

Adds sandbox profile so that PostgreSQL can set up shared memory. Also makes it run faster by skipping sync during initdb and using immediate shutdown mode.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • 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/)
  • 25.05 Release Notes (or backporting 24.11 and 25.05 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.

Add a 👍 reaction to pull requests you find important.

@ofborg ofborg bot added the 6.topic: darwin Running or building packages on Darwin label Feb 19, 2025
@github-actions github-actions bot added 8.has: documentation This PR adds or changes documentation 10.rebuild-darwin: 11-100 This PR causes between 11 and 100 packages to rebuild on Darwin. 10.rebuild-linux: 101-500 This PR causes between 101 and 500 packages to rebuild on Linux. labels Feb 19, 2025
@tie tie marked this pull request as ready for review February 19, 2025 13:27
@tie tie requested a review from wolfgangwalther February 19, 2025 13:27
@wolfgangwalther
Copy link
Contributor

Is this related to #371242? If yes, we need to add this in more places than just the test hook.

Copy link
Member Author

@tie tie Feb 19, 2025

Choose a reason for hiding this comment

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

Is this related to #371242?

I don’t think so. I’m not aware of this issue, but it seems to mention that builds fail on Hydra and IIRC macOS builders do not use sandbox (though things could have changed since the last time I’ve checked).

Forcing mmap implementation instead of POSIX/SysV shared memory might help as discussed here, but I don’t have time to test that.

Copy link
Contributor

Choose a reason for hiding this comment

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

If it's not about that, what is it about then? What kind of error are you getting that is solved by doing this?

Copy link
Member Author

@tie tie Feb 19, 2025

Choose a reason for hiding this comment

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

Well, postgresqlTestHook does not work when sandbox = true in Nix config on macOS because default sandbox profile does not allow shared memory syscalls.

running bootstrap script ... 2025-02-19 14:07:08.362 UTC [98344] FATAL:  could not create shared memory segment: Operation not permitted
2025-02-19 14:07:08.362 UTC [98344] DETAIL:  Failed system call was shmget(key=80109247, size=56, 03600).

(note Operation not permitted at the end of first line)

I’ve just tried running initdb with various combinations of -c shared_memory_type=VALUE and -c dynamic_shared_memory_type=VALUE and none worked, so I’ll remove this TODO comment.

Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately, I can't reproduce FATAL: could not create shared memory segment: Operation not permitted at all.

Can you give me exact instructions, i.e. which branch, which attribute to build?

Copy link
Member Author

Choose a reason for hiding this comment

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

  • aarch64-darwin host
  • sandbox = true set in /etc/nix/nix.conf
  • sudo launchctl kickstart -k system/org.nixos.nix-daemon
    to restart Nix daemon if sandbox = true was not there before
  • Nix 2.18.1
  • I’ve doubled-checked with Nix version 2.24.12 and this seems to be fixed darwin: allow ipc-sysv* in sandbox nix#10878, but older Nix version still require an explicit sandbox profile.
nix build --print-build-logs --no-link --impure --expr 'with import ./. { };
stdenv.mkDerivation {
  name = "postgresql-test-hook-repro";
  dontUnpack = true;
  doCheck = true;
  nativeCheckInputs = [
    postgresqlTestHook
    postgresql
  ];
  installPhase = ''
    runHook preInstall
    mkdir -p "$out"
    runHook postInstall
  '';
}'

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for those details. Unfortunately, I can't test with older nix versions, because I only have non-root access to the nix-community darwin builder. But your explanation sounds sensible.

Not being able to reproduce this makes it harder for me to come up with fixes for other cases where we need the same - and I know there are some.

One major case that we surely need to address in this PR already: postgresql itself. It also runs initdb during the check phase and since we have enabled the check phase on darwin a while ago, you would hit the same. It would be kind of pointless to fix the test hook, but not being able to build postgresql itself before even getting there.

I assume you just didn't rebuild postgresql locally, yet, but have always pulled it from the cache. You should be able to reproduce it with nix-build --check -A postgresql easily.

Could you please also extend the comment to mention the version this has been fixed in and that we need to keep it around for older versions, which we still support?

Copy link
Contributor

Choose a reason for hiding this comment

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

I’ve doubled-checked with Nix version 2.24.12 and this seems to be fixed

Also, lix did not backport that change (yet), so running lix on MacOS will lead to the same problems.

Comment on lines +15 to +16
export PGSSLMODE=${PGSSLMODE:-disable}

Copy link
Contributor

Choose a reason for hiding this comment

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

I am curious: Did you hit a case where that was necessary to fix a build? Or is this another kind of performance improvement?

@wolfgangwalther wolfgangwalther marked this pull request as draft March 24, 2025 19:34
@wolfgangwalther
Copy link
Contributor

Converted to draft, because currently the check phase for PostgreSQL is disabled on master and I'd like to mark the postgresqlTestHook as broken for darwin, too. See #392430. Even though newer nix versions support IPC in the sandbox, they still don't clean up properly, so it still leads to build failures eventually.

Now I wonder: If this bug is fixed in Nix at some point and those segments are cleaned up - should we then change the sandbox profile as suggested here? I don't think so, because that would not give us the proper IPC object cleanup. Thus, I'd rather have this fail before those segments are created on older nix versions.

We could still do the second and third commit here. WDYT @tie?

@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Apr 2, 2025
@emilazy
Copy link
Member

emilazy commented Sep 7, 2025

Completely missed the ping here.

FWIW, I think allowing SysV IPC in the sandbox is not a great idea, unless it can be restricted further, which would require more research. Is there a way to get Postgres to… not do that?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

2.status: merge conflict This PR has merge conflicts with the target branch 6.topic: darwin Running or building packages on Darwin 8.has: documentation This PR adds or changes documentation 10.rebuild-darwin: 11-100 This PR causes between 11 and 100 packages to rebuild on Darwin. 10.rebuild-linux: 101-500 This PR causes between 101 and 500 packages to rebuild on Linux.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants