postgresql: refactor and cleanup by wolfgangwalther · Pull Request #292993 · NixOS/nixpkgs

wolfgangwalther · 2024-03-03T11:38:09Z

Description of changes

TLDR: This is a pure refactor, where postgresql/default.nix is split into multiple files and some passthru attributes are cleaned up. No derivations are changed.

Goal: I am currently working on building PG v16 via meson instead of autotools. This will make it possible to make pkgsStatic.postgresql work - at least for building libpq, which should cover almost all use-cases. This PR does not implement the meson build, yet. But to be able to have autotools-based builds up to v15 and meson-based builds for v16+ separated without repeating all the shared parts, this PR moves things around a little bit and splits them up in multiple files. But even without building with meson, the new structure is still an improvement, I think.

This is best reviewed commit-by-commit. I tried to make it reviewable despite the big chunks of code moved around. The most important fact: The derivations for all 5 versions with and without _jit don't change, so it's a pure refactor.

The files are structured like this:

default.nix contains the logic to create all the top-level attributes, both with and without _jit suffix.
<major>.nix files contain the version-specific stuff: version number, hashes, musl patches. This is recommended here.
generic.nix has all the metadata and most passthru arguments of the derivation, thus dealing with all the packages/extensions, tests and jit-helpers. It also contains all the patches, to make sure that both autotools and meson based builds will stay in sync.
autotools.nix only has those parts which are specific to the autotools builds. This makes it easy to add a meson.nix next to it later.

Along the way, I also cleaned up a few things around arguments and passthru attributes:

There was a readline passthru attribute introduced around 20 years ago - but entirely unused. Removed.
psqlSchema is auto-created instead of passed for each version. Less chance for error when copy&pasting for a new version.
jitSupport / llvm are not passed as passthru attributes to extensions, but via callPackage / the new pkgs scope. Again, reduces the chance for mistakes.
The this argument is removed and replaced with internal recursion. This makes it possible to actually have things like (postgresql.override { jitSupport = true; }).pkgs.postgis work correctly without making sure to pass this all the time.

I also have commits to clean up thisAttr which suffers from the same problem for tests that this did - but those would not be a pure refactor anymore, so I'll put them in a separate PR.

Pinging maintainers: @thoughtpolice @danbst @globin @marsam @ivan @Ma27

Things done

Add a 👍 reaction to pull requests you find important.

nixos/modules/services/databases/postgresql.nix

Ma27 · 2024-03-03T14:33:53Z

May take until next week to get to it. But I'm definitely interested in reviewing that, thanks!

wolfgangwalther · 2024-03-03T14:43:15Z

It's unclear to me why postgresql, postgresql.passthru.tests on x86_64-darwin fails.

It says something about a readline-related failure. Is that related to this?

There was a readline passthru attribute introduced around 20 years ago - but entirely unused. Removed.

I don't see how, though.

Ma27 · 2024-03-03T14:46:09Z

Hmm, try running nix-instantiate -A postgresql --argstr system x86_64-darwin? Or whatever attribute is affected. Then you may be able to debug the problem locally.

You don't need such hardware for the evaluation itself, that only gets relevant when it comes to building stuff, but the ofborg error looks like it didn't even get to that.

wolfgangwalther · 2024-03-03T14:50:02Z

Hmm, try running nix-instantiate -A postgresql --argstr system x86_64-darwin? Or whatever attribute is affected. Then you may be able to debug the problem locally.

You don't need such hardware for the evaluation itself, that only gets relevant when it comes to building stuff, but the ofborg error looks like it didn't even get to that.

Ah right, thanks for the hint. Both postgresql and postgresql.tests evaluate fine for x86_64-darwin for me locally. Thus, I assume this error is random noise and not related to this PR:

error: cannot connect to socket at '/nix/var/nix/gc-socket/socket': No such file or directory

Ma27 · 2024-03-03T14:57:28Z

Oh sorry I only saw the backtrace and didn't scroll down entirely 😅

wolfgangwalther

Note: The latest push was just a rebase on latest master, no changes.

pkgs/servers/sql/postgresql/autotools.nix

pkgs/servers/sql/postgresql/packages.nix

pkgs/servers/sql/postgresql/autotools.nix

pkgs/servers/sql/postgresql/generic.nix

pkgs/servers/sql/postgresql/packages.nix

Ma27 · 2024-03-08T18:21:36Z

Regarding 32badde1f653b338314378e8918b036f538ea8ad: I'm not sure I understand the rationale behind moving things around there.

wolfgangwalther · 2024-03-08T18:52:53Z

Regarding 32badde: I'm not sure I understand the rationale behind moving things around there.

Before this change, I would have to add this for a new version:

    postgresql_17 = import ./17.nix {
      this = self.postgresql_17;
      thisAttr = "postgresql_17";
      inherit self;
    };

In theory, there are 4 chances to get it wrong: Any of the 17s.

After this change, I just need to do this:

    postgresql_17 = ./17.nix;

Granted.. after the later changes to remove this and thisAttr (next PR), there is less chance to go wrong here.

Another thing is, that with the old structure someone could theoretically come up later and add some version specific arguments in default.nix. But that's certainly something we don't want, that's what we have the versioned files for. After the change this would be much harder to do, so I expect nobody to try ;)

pkgs/servers/sql/postgresql/generic.nix

nixos/modules/services/databases/postgresql.md

pkgs/servers/sql/postgresql/generic.nix

Ma27 · 2024-03-10T12:00:44Z

Hmm... would you mind trying out if adding 2da789a409c67c99c7e7b27730256d42f10a5601 to .git-blame-ignore-revs helps to retain a bit of the history of the previous file?

No idea if that works out, but maybe worth a try?

Anyways, I like that change and the fact that we got a few cleanups ready without any rebuilds.

Will let a few other people review as well before merging. cc @marsam @thoughtpolice @ivan

wolfgangwalther · 2024-03-10T13:14:38Z

Hmm... would you mind trying out if adding 2da789a to .git-blame-ignore-revs helps to retain a bit of the history of the previous file?

No idea if that works out, but maybe worth a try?

I think that's a great idea, not sure whether to wait to do this in a second PR (maybe in the follow up cleanup PR), once this is merged, to avoid having to change the commit hash.

Anyways, I like that change and the fact that we got a few cleanups ready without any rebuilds.

Will let a few other people review as well before merging. cc @marsam @thoughtpolice @ivan

👍

Ma27 · 2024-03-10T15:31:17Z

I think that's a great idea, not sure whether to wait to do this in a second PR (maybe in the follow up cleanup PR)

Fair.

This commit is split up into two commits to allow git to detect renames, make rebasing easier and allow a working entry in .git-blame-ignore-revs. To allow bisecting we allow evaluation on every commit by moving the extensions into ext/ext/ first and back to ext/ with the next commit.

This aligns more with the commonly used style in nixpkgs.

No need to reference self here, because llvmPackages / stdenv' are available in that scope anyway. Pure refactor, derivations don't change.

This just renames default.nix to generic.nix, because the biggest chunk of code should move that way in the next commit. This gives us a much better diff for the next commit and makes rebasing **much** easier in case of changes. This commit does not stand on its own and needs to go in with the next commit (2/2).

The recommended [1] structure for a package regarding versioning is to have each version in a separate file. This commit just mechanically copies code around without any changes. Pure refactor, not changing any derivations. [1]: pkgs/README.md

Refactors some low hanging fruit in default.nix to make it easier to add new versions later on. Pure refactor, not changing any derivations. This change makes it easier to add new versions in default.nix without messing up - and also prevents us from adding version-specific arguments in default.nix by accident in the future. Those should be put in the versioned .nix files instead.

This seems to have been introduced 20 years ago in 5863d4f - but seems to have been a copy&paste mistake from the beginning. AFAICT, it's not used anywhere.

The passthru attribute is still set, but automatically created from the major version number. Fewer moving parts decrease the chance for mistakes.

This makes it obvious that the required argument muslPatches must be passed when creating a new version file. Pure refactor, not changing any derivations.

wolfgangwalther · 2024-03-15T17:59:09Z

Rebased after some new extensions caused a merge conflict for ext/default.nix.

I think this is in good shape. @Ma27 do you think there will be any other reviewers? Otherwise, I think we could go ahead with what we have. No functional changes, so very likely no problem.

Ma27 · 2024-03-15T18:49:35Z

I think this is in good shape

Agreed.

Ping @ajs124 @marsam @yu-re-ka you were the last folks who touched the package itself.

I'd consider merging it at some point next week though, I'd rather not stall this.

yu-re-ka · 2024-03-15T19:49:08Z

Eval is failing

otherwise this is probably fine

…sthru This makes it less error-prone to use the llvm package in extensions, because it will always match the package used by the postgresql derivation itself. Previously, you could've accidentally used llvm instead of postgresql.llvm with a different result.

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

This is the default anyway.

We have gssSupport, jitSupport and pythonSupport - but enableSystemd? Across nixpkgs there are currently three styles commonly used, about equally often: withX, xSupport and enableX. Let's at least use one consistent style in this package.

wolfgangwalther · 2024-03-15T20:12:19Z

Eval is failing

Ah, one of the new extensions after rebase was still using the old jitSupport passthru. Fixed.

marsam

LGTM. Good work, thank you!

Ma27

Great work!

This was deprecated in e6bfabf, where we agreed on removing this after one release in [1]. Time has come! [1]: NixOS#292993 (comment)

wolfgangwalther requested review from marsam and thoughtpolice as code owners March 3, 2024 11:38

github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Mar 3, 2024

wolfgangwalther commented Mar 3, 2024

View reviewed changes

nixos/modules/services/databases/postgresql.nix Outdated Show resolved Hide resolved

ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Mar 3, 2024

Ma27 self-requested a review March 3, 2024 14:33

wolfgangwalther force-pushed the postgresql-refactor branch from 82b9cc0 to 12bc04e Compare March 7, 2024 07:52

wolfgangwalther commented Mar 7, 2024

View reviewed changes

pkgs/servers/sql/postgresql/autotools.nix Outdated Show resolved Hide resolved

pkgs/servers/sql/postgresql/packages.nix Outdated Show resolved Hide resolved

wolfgangwalther mentioned this pull request Mar 7, 2024

postgresql: more cleanup #293996

Merged

13 tasks

wolfgangwalther commented Mar 7, 2024

View reviewed changes

pkgs/servers/sql/postgresql/autotools.nix Outdated Show resolved Hide resolved

Ma27 reviewed Mar 8, 2024

View reviewed changes

pkgs/servers/sql/postgresql/generic.nix Outdated Show resolved Hide resolved

pkgs/servers/sql/postgresql/packages.nix Outdated Show resolved Hide resolved

wolfgangwalther force-pushed the postgresql-refactor branch 2 times, most recently from fb18d24 to b76450a Compare March 8, 2024 19:07

Ma27 reviewed Mar 8, 2024

View reviewed changes

pkgs/servers/sql/postgresql/generic.nix Outdated Show resolved Hide resolved

Ma27 reviewed Mar 8, 2024

View reviewed changes

nixos/modules/services/databases/postgresql.md Outdated Show resolved Hide resolved

wolfgangwalther force-pushed the postgresql-refactor branch from b76450a to a5a33e5 Compare March 9, 2024 09:01

wolfgangwalther mentioned this pull request Mar 9, 2024

postgresql: split -lib and -dev outputs cleanly #294504

Merged

13 tasks

Ma27 reviewed Mar 10, 2024

View reviewed changes

pkgs/servers/sql/postgresql/generic.nix Outdated Show resolved Hide resolved

wolfgangwalther added 9 commits March 15, 2024 18:50

postgresql: refactor to move packages.nix to ext/default.nix (2/2)

719034f

This aligns more with the commonly used style in nixpkgs.

postgresql: refactor to remove useless references to "self"

4c8e7af

No need to reference self here, because llvmPackages / stdenv' are available in that scope anyway. Pure refactor, derivations don't change.

postgresql: refactor to remove passthru's readline attribute

a92a323

This seems to have been introduced 20 years ago in 5863d4f - but seems to have been a copy&paste mistake from the beginning. AFAICT, it's not used anywhere.

postgresql: refactor to remove psqlSchema argument

82e6c4a

The passthru attribute is still set, but automatically created from the major version number. Fewer moving parts decrease the chance for mistakes.

postgresql: refactor to move musl patches into versioned .nix files

b301c01

This makes it obvious that the required argument muslPatches must be passed when creating a new version file. Pure refactor, not changing any derivations.

wolfgangwalther force-pushed the postgresql-refactor branch from 833abce to c4d2f56 Compare March 15, 2024 17:53

ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Mar 15, 2024

wolfgangwalther added 4 commits March 15, 2024 21:11

postgresql: refactor to remove doInstallCheck = false

d7ae1c5

This is the default anyway.

wolfgangwalther force-pushed the postgresql-refactor branch from c4d2f56 to e6bfabf Compare March 15, 2024 20:12

postgresql: explain use of fetchurl for alpine patches

0144495

marsam approved these changes Mar 17, 2024

View reviewed changes

Ma27 approved these changes Mar 17, 2024

View reviewed changes

Ma27 merged commit 6af7e81 into NixOS:master Mar 17, 2024

wolfgangwalther deleted the postgresql-refactor branch March 17, 2024 13:26

wolfgangwalther mentioned this pull request Mar 30, 2024

Building static executables directly via nix' pkgsStatic.postgrest PostgREST/postgrest#3363

Open

10 tasks

Uh oh!

Conversation

wolfgangwalther commented Mar 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of changes

Things done

Uh oh!

Uh oh!

Ma27 commented Mar 3, 2024

Uh oh!

wolfgangwalther commented Mar 3, 2024

Uh oh!

Ma27 commented Mar 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wolfgangwalther commented Mar 3, 2024

Uh oh!

Ma27 commented Mar 3, 2024

Uh oh!

wolfgangwalther left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Ma27 commented Mar 8, 2024

Uh oh!

wolfgangwalther commented Mar 8, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Ma27 commented Mar 10, 2024

Uh oh!

wolfgangwalther commented Mar 10, 2024

Uh oh!

Ma27 commented Mar 10, 2024

Uh oh!

wolfgangwalther commented Mar 15, 2024

Uh oh!

Ma27 commented Mar 15, 2024

Uh oh!

yu-re-ka commented Mar 15, 2024

Uh oh!

wolfgangwalther commented Mar 15, 2024

Uh oh!

marsam left a comment

Choose a reason for hiding this comment

Uh oh!

Ma27 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

wolfgangwalther commented Mar 3, 2024 •

edited

Loading

Ma27 commented Mar 3, 2024 •

edited

Loading