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: 16.4 -> 17.0 #345260

Merged
merged 2 commits into from
Nov 14, 2024
Merged

postgresql: 16.4 -> 17.0 #345260

merged 2 commits into from
Nov 14, 2024

Conversation

wolfgangwalther
Copy link
Contributor

@wolfgangwalther wolfgangwalther commented Sep 29, 2024

Description of changes

PostgreSQL 17 has been released.

This PR adds the new major version and then updates the postgresql package to it. The module's default version is unchanged as discussed in #329611 (comment).

What I did so far:

  • Built postgresql 17 with and without JIT on x86_64-linux, aarch64-darwin and with pkgsMusl.
  • Built related nixos tests on linux.
  • Updated some extensions breaking with the new version, where an update was available. There are more which currently still break, but no new version / fix is available, yet.
  • Started a nixpkgs-review run and found one related build failure, so far: Kept python3Packages.asyncpg at v16. (still running)

I did all this by cherry-picking the commits here on master.

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/)
  • 24.11 Release Notes (or backporting 23.11 and 24.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.

@wolfgangwalther
Copy link
Contributor Author

wolfgangwalther commented Sep 29, 2024

Started a nixpkgs-review run [...]

I can't nearly complete a run of it, though, without crashing my machine.

I would love to have a way to only test direct reverse dependencies of postgresql - but I don't know how.

pkgs/servers/sql/postgresql/ext/pg_similarity.nix Outdated Show resolved Hide resolved
pkgs/servers/sql/postgresql/ext/pg_similarity.nix Outdated Show resolved Hide resolved
pkgs/servers/sql/postgresql/generic.nix Outdated Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this overwrite be in python-packages.nix?

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, is there some consensus on what's preferred?
I'm somewhat indifferent on that since I can see reasons for both styles (explicitness for what's required and less error-prone backports[1] vs more unneeded changes to a file and weird-looking code if you ever have a reason to use an older version (postgresql_16 = postgresql_15; or whatever)).

[1] If the default postgresql is older on stable, a naive backport will leave a broken package even though the attribute of the newer package is available just not the default. Running into this regularly with Go stuff.

Copy link
Contributor Author

@wolfgangwalther wolfgangwalther Nov 1, 2024

Choose a reason for hiding this comment

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

Found a note for by-name and overrides, that suggests the style in xxx-packages.nix would be preferred, especially because of (breaking) overriding: https://github.com/NixOS/nixpkgs/blob/master/pkgs/by-name/README.md#changing-implicit-attribute-defaults

@SuperSandro2000
Copy link
Member

I can't nearly complete a run of it, though, without crashing my machine.

That is not expected.

I would love to have a way to only test direct reverse dependencies of postgresql - but I don't know how.

We could do a hydra jobset but how do you assess the situation outside of postgres extensions: Are there expected to be many breakages?

@Ma27
Copy link
Member

Ma27 commented Oct 8, 2024

fwiw it's on my radar and I'll try to give a review soonish!

@wolfgangwalther wolfgangwalther mentioned this pull request Oct 9, 2024
4 tasks
@takeda
Copy link
Contributor

takeda commented Oct 15, 2024

Is this being held because some packages are being broken? Would it make sense to release 17.0 but still keep 16.x as the default, and resolving the issues when promoting the default from 16 to 17?

This would allow others to start using 17.0 and possibly also find any issues with it and other packages.

Copy link
Member

@Ma27 Ma27 left a comment

Choose a reason for hiding this comment

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

Currently fails for me with

postgresql> applying patch /nix/store/4gn6alxx0i0h61bdx1cfq217gqmc4q6x-b27622c90869aab63cfe22159a459c57768b0fa4.patch
postgresql> patching file src/test/regress/expected/date.out
postgresql> Reversed (or previously applied) patch detected!  Assume -R? [n] 
postgresql> Apply anyway? [n] 
postgresql> Skipping patch.
postgresql> 3 out of 3 hunks ignored -- saving rejects to file src/test/regress/expected/date.out.rej
postgresql> patching file src/test/regress/expected/horology.out
postgresql> Reversed (or previously applied) patch detected!  Assume -R? [n] 
postgresql> Apply anyway? [n] 
postgresql> Skipping patch.
postgresql> 9 out of 9 hunks ignored -- saving rejects to file src/test/regress/expected/horology.out.rej
postgresql> patching file src/test/regress/expected/timestamptz.out
postgresql> Reversed (or previously applied) patch detected!  Assume -R? [n] 
postgresql> Apply anyway? [n] 
postgresql> Skipping patch.
postgresql> 13 out of 13 hunks ignored -- saving rejects to file src/test/regress/expected/timestamptz.out.rej
postgresql> patching file src/test/regress/pg_regress.c
postgresql> Reversed (or previously applied) patch detected!  Assume -R? [n] 
postgresql> Apply anyway? [n] 
postgresql> Skipping patch.
postgresql> 1 out of 1 hunk ignored -- saving rejects to file src/test/regress/pg_regress.c.rej
postgresql> patching file src/test/regress/sql/horology.sql
postgresql> Reversed (or previously applied) patch detected!  Assume -R? [n] 
postgresql> Apply anyway? [n] 
postgresql> Skipping patch.
postgresql> 1 out of 1 hunk ignored -- saving rejects to file src/test/regress/sql/horology.sql.rej
postgresql> patching file src/test/regress/sql/timestamptz.sql
postgresql> Reversed (or previously applied) patch detected!  Assume -R? [n] 
postgresql> Apply anyway? [n] 
postgresql> Skipping patch.
postgresql> 1 out of 1 hunk ignored -- saving rejects to file src/test/regress/sql/timestamptz.sql.rej

@SuperSandro2000
Copy link
Member

SuperSandro2000 commented Oct 21, 2024

I excluded the patch with the below diff but now disallowed reference complains about some -dev packages leaking into bin/postgres

diff --git a/pkgs/servers/sql/postgresql/generic.nix b/pkgs/servers/sql/postgresql/generic.nix
index 90306fa3f5e8..d709cab1243a 100644
--- a/pkgs/servers/sql/postgresql/generic.nix
+++ b/pkgs/servers/sql/postgresql/generic.nix
@@ -79,7 +79,8 @@
       disallowedRequisites = [
         stdenv'.cc
       ] ++ (
-        map lib.getDev (builtins.filter (drv: drv ? "dev") finalAttrs.buildInputs)
+        # map lib.getDev (builtins.filter (drv: drv ? "dev") finalAttrs.buildInputs)
+        []
       ) ++ lib.optionals jitSupport [
         llvmPackages.llvm.out
       ];
@@ -170,7 +171,7 @@
       })

       # TODO: Remove this with the next set of minor releases
-      (fetchpatch (
+      (if olderThan "17" then fetchpatch (
         if atLeast "14" then {
           url = "https://github.com/postgres/postgres/commit/b27622c90869aab63cfe22159a459c57768b0fa4.patch";
           hash = "sha256-7G+BkJULhyx6nlMEjClcr2PJg6awgymZHr2JgGhXanA=";
@@ -183,7 +184,7 @@
           url = "https://github.com/postgres/postgres/commit/205813da4c264d80db3c3215db199cc119e18369.patch";
           hash = "sha256-L8/ns/fxTh2ayfDQXtBIKaArFhMd+v86UxVFWQdmzUw=";
           excludes = [ "doc/*" ];
-        })
+        }) else {}
       )
     ] ++ lib.optionals stdenv'.hostPlatform.isMusl (
       # Using fetchurl instead of fetchpatch on purpose: https://github.com/NixOS/nixpkgs/issues/240141

@wolfgangwalther wolfgangwalther mentioned this pull request Oct 25, 2024
13 tasks
@wolfgangwalther
Copy link
Contributor Author

Would it make sense to release 17.0 but still keep 16.x as the default

Did that in #351253 targeting the master branch. Once that is merged, I'll rebase this branch to only contain updating postgresql: 16.4 -> 17.0 targeting staging.

Currently fails for me with

Ah, yeah, that happened when testing against an older master branch and then opening the PR against staging... fixed in #351253.

I excluded the patch with the below diff but now disallowed reference complains about some -dev packages leaking into bin/postgres

My diff in #351253 looks slightly different and I never got those reference errors.

@wolfgangwalther wolfgangwalther marked this pull request as draft October 27, 2024 08:44
@wolfgangwalther
Copy link
Contributor Author

Converted to draft until pg17 is merged into master and master into staging.

Copy link
Member

@Ma27 Ma27 left a comment

Choose a reason for hiding this comment

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

Most of the stuff was reviewed already and the rest is also looking good.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, is there some consensus on what's preferred?
I'm somewhat indifferent on that since I can see reasons for both styles (explicitness for what's required and less error-prone backports[1] vs more unneeded changes to a file and weird-looking code if you ever have a reason to use an older version (postgresql_16 = postgresql_15; or whatever)).

[1] If the default postgresql is older on stable, a naive backport will leave a broken package even though the attribute of the newer package is available just not the default. Running into this regularly with Go stuff.

@wolfgangwalther
Copy link
Contributor Author

Rebased, this now only contains the changes to move postgresql from 16 to 17.

I can't nearly complete a run of it, though, without crashing my machine.

That is not expected.

Still no change. About 5000 packages need to be rebuilt and eventually memory usage is so high, that sway becomes entirely unresponsive. I tried disabling swap, no difference. I also tried disabling memory overcommit - but that will just prevent nix from building much earlier, because it can't allocate memory. Not sure what's going on.

I am now trying again by moving /tmp off of tmpfs, maybe that helps...

I would love to have a way to only test direct reverse dependencies of postgresql - but I don't know how.

We could do a hydra jobset but how do you assess the situation outside of postgres extensions: Are there expected to be many breakages?

There shouldn't be many, I guess. The majority of libpq users should just keep working. The blast radius might be higher, because of reverse dependencies, though. Most packages using libpq directly would be some kind of drivers, so they would affect a lot of packages. Once those build successfully... everything behind that should not be a problem anymore.

It would be great if nixpkgs-review had a flag to only test direct reverse dependencies and not "everything that changed"...


I am currently testing whether python3Packges.psycopg works now - it should, it has been updated in the meantime. python3Packages.asyncpg now has an update upstream, which supports pg17, I will try to update to that as well.

@wolfgangwalther wolfgangwalther marked this pull request as ready for review October 28, 2024 20:40
@wolfgangwalther
Copy link
Contributor Author

wolfgangwalther commented Oct 29, 2024

Started a nixpkgs-review run [...]

All the three broken packages I knew about work now.

I am still running tests to find others.

@wolfgangwalther
Copy link
Contributor Author

ofborg eval fails for unrelated reasons, but I won't rebase right now, because I don't want to rebuild the world again during my tests...

@kirillrdy
Copy link
Member

We are in feature freeze, so this will not be merged until after 24.11

@wolfgangwalther
Copy link
Contributor Author

We are in feature freeze, so this will not be merged until after 24.11

I know and agree.

@kirillrdy
Copy link
Member

sorry, i just realised that staging will not make it to 24.11 anyways, ignore my comment

@wolfgangwalther
Copy link
Contributor Author

I have now built all direct reverse dependencies (by grepping for postgresql and building those packages) and found only one breaking package which I fixed (kore) by pinning to postgresql v16.

This should be good from my side.

@wolfgangwalther wolfgangwalther requested a review from Ma27 October 29, 2024 19:30
@ofborg ofborg bot requested a review from eadwu October 30, 2024 02:28
Copy link
Member

@Ma27 Ma27 left a comment

Choose a reason for hiding this comment

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

According to #339153 there's a final staging-next cycle starting on Nov 6. This means that staging will be merged into staging-next then and we'd end up with the bump in 24.11. Hence blocking.

Code-wise it looks good and the touched packages build for me, so I'd merge right after that.

@wolfgangwalther
Copy link
Contributor Author

Let's make sure we revert #354526 before merge.

@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Nov 10, 2024
As discussed in [1] this change is decoupled from the major version
update for the NixOS postgresql module.

[1]: https://github.com/NixOS/nixpkgs/pull/329611/files#r1701393693
@wolfgangwalther
Copy link
Contributor Author

Let's make sure we revert #354526 before merge.

Put that in and rebased.

Since current staging won't make it to 24.11 anymore anyway, this should be good to go @Ma27.

@ofborg ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Nov 14, 2024
@Ma27 Ma27 merged commit 2e21fa2 into NixOS:staging Nov 14, 2024
27 of 28 checks passed
@wolfgangwalther wolfgangwalther deleted the postgresql-17 branch November 15, 2024 18:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants