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

pg-dump-anon: use latest postgresql available #354526

Merged
merged 1 commit into from
Nov 9, 2024
Merged

Conversation

Ma27
Copy link
Member

@Ma27 Ma27 commented Nov 8, 2024

While reviewing #352966 I noticed that the pg_anonymizer test fails for postgresql 17. The reason for that is that pkgs.postgresql is v16 and using its psql to connect against a v17 database doesn't work.

I decided that we'll just use the latest available package in here. I don't want to introduce another attribute (postgresql_latest), if there are too many instances of that we're blocked on adding new postgresql majors directly to master again which is the current status quo. With the test rework in #352966 it's also way easier to catch this.

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.

While reviewing NixOS#352966 I noticed that the pg_anonymizer test fails for
postgresql 17. The reason for that is that `pkgs.postgresql` is v16 and
using its psql to connect against a v17 database doesn't work.

I decided that we'll just use the latest available package in here. I
don't want to introduce another attribute (`postgresql_latest`), if
there are too many instances of that we're blocked on adding new
postgresql majors directly to master again which is the current status
quo. With the test rework in NixOS#352966 it's also way easier to catch this.
@wolfgangwalther
Copy link
Contributor

Hm, the "proper" fix would be to merge #345260, right?

I'll add a note over there to turn this back to postgresql once it switches to v17.

@wolfgangwalther wolfgangwalther mentioned this pull request Nov 8, 2024
13 tasks
@Ma27
Copy link
Member Author

Ma27 commented Nov 9, 2024

Hm, the "proper" fix would be to merge #345260, right?

Sort of, yes.
But this won't land in 24.11, so umless we merge this thing first, we'll ship a partly broken package on 24.11.

@wolfgangwalther
Copy link
Contributor

But this won't land in 24.11, so umless we merge this thing first, we'll ship a partly broken package on 24.11.

Yeah, I know, so agree on doing what is proposed here temporarily.

I was just thinking about how to prevent this in the future.. but the only way to prevent a temporarily broken test in this case, is to make sure that postgresql is always the latest version, bumping at the same time that the new major is added.

Which is kind of an argument against the "let's split the init of new major and bump of postgresql" that we discussed earlier.

@Ma27
Copy link
Member Author

Ma27 commented Nov 9, 2024

Which is kind of an argument against the "let's split the init of new major and bump of postgresql" that we discussed earlier.

Now that I'm thinking about it, the approach actually seems like a rather bad idea: you can deploy postgresql 17 instances on 24.11 now, but a lot of software will fail to talk to it (given that the libpq most stuff is using is still postgresql 16), right?

So, first of all, I'd say that yes, we should couple adding new majors with making it the default attribute[1].

The question is now, what do we do with 24.11?
I think adding something to the release notes is the least (and perhaps even an eval-warning).

[1] FTR and as discussed, this doesn't imply that this will be the default postgresql version installed by NixOS.

@Ma27
Copy link
Member Author

Ma27 commented Nov 9, 2024

Anyways, I think we agree that this change is valid on its own, so I'll go ahead and merge.

@Ma27 Ma27 merged commit f11b5ff into NixOS:master Nov 9, 2024
32 of 33 checks passed
@Ma27 Ma27 deleted the pg-dump-anon-v17 branch November 9, 2024 16:57
@wolfgangwalther
Copy link
Contributor

Now that I'm thinking about it, the approach actually seems like a rather bad idea: you can deploy postgresql 17 instances on 24.11 now, but a lot of software will fail to talk to it (given that the libpq most stuff is using is still postgresql 16), right?

I don't think everything will be broken. Most applications using libpq 16 will just keep working with a v17 server.

The question is now, what do we do with 24.11?
I think adding something to the release notes is the least (and perhaps even an eval-warning).

I don't think we need an eval warning, probably not even something in the release notes.

Imho, we had the same situation with postgresql = 15, but 16 in the tree already, for a long time, right?

I expect very packages to break like pg-dump-anon here.

@wolfgangwalther
Copy link
Contributor

Most applications using libpq 16 will just keep working with a v17 server.

Practical example: I just updated PostgREST's CI to test with v17 server as well, but still linking the app against v16. All works fine.

@Ma27
Copy link
Member Author

Ma27 commented Nov 9, 2024

Ah no, my mistake, the culprit wasn't the psql invocation, but the pg_dump which makes a lot more sense given that we indeed had postgresql_16 packaged for a while with 15 being the default.

Apologies!

But in that case, we can keep it that way and add new majors early (as we've done already). Sorry for the noise 🙈

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.

2 participants