postgresql: improve passthru.tests #352966

wolfgangwalther · 2024-11-01T20:00:33Z

postgresql and postgresqlPackages's tests currently have the following problems:

some of the nixosTests are missing as passthru tests for the postgresql package
most extension's passthru tests always run with the default postgresql version, e.g. postgresql_12_jit.pkgs.postgis.tests actually tests the extension compiled again postgresql_16.

This PR fixes that - and more. The general approach is to provide a passthru.override function from each of the postgresql related nixosTests - and then use that to override the postgresqlPackage from the derivation/extension.

Furthermore, some tests (postgis and apache_datasketches) were simple enough to easily migrate them to postgresqlTestExtension - which doesn't need any VM, but just runs the tests via postgresqlTestHook. This is much more portable, as it should also work for darwin (do nixosTests work there, normally?) or could also work for pkgsMusl.postgresql... (didn't test, though).

Most importantly.. it would have allowed to run all those tests against the new PostgreSQL version, when we added that in #351253. Instead, they were all still running against the default version in that PR.

Things done

Add a 👍 reaction to pull requests you find important.

Ma27

I like the change, thanks!

Just one question: what's the recommended way of running tests for all versions now?

nixos/tests/postgresql-tls-client-cert.nix

wolfgangwalther · 2024-11-02T08:58:17Z

Just one question: what's the recommended way of running tests for all versions now?

Something like nix-build -A postgresql_12.tests -A postgresql_13.tests ... I guess...

Hm..

The thing about nixosTests.postgresql-XXX (before this change) was that you could test all PG versions, but not run all tests (with one attribute). The thing about passthru tests is, that you can run all tests, but only for a single PG version.

I wonder whether we can somehow have an attribute somewhere, which would make it easy to do both at the same time?

wolfgangwalther · 2024-11-02T10:29:33Z

And while we're considering test coverage: We currently have an option pythonSupport ? false, which I think is never tested either. There are more features for which we could add flags, too (see e.g. #218934). However, we'd need to make sure to actually test those, too.

Is there a specific reason for pythonSupport defaulting to false btw? It seems to me that we should enable features whenever possible in principle - giving us better test coverage - and then have users disable features they don't need. WDYT?

Ma27 · 2024-11-02T13:17:23Z

I wonder whether we can somehow have an attribute somewhere, which would make it easy to do both at the same time?

I mean we have all information available for that.
I'm just not entirely sure where.

The easiest approach would be to add this to the respective files (i.e. nixos/tests/postgresql.nix), but this would be kinda awkward because we'd then have the passthru thing to create tests for a given version and all instantiated versions right next to it.

I don't know. I think I could also live with this approach though.

wolfgangwalther · 2024-11-02T13:20:52Z

The easiest approach would be to add this to the respective files (i.e. nixos/tests/postgresql.nix), but this would be kinda awkward because we'd then have the passthru thing to create tests for a given version and all instantiated versions right next to it.

And that would still only allow to test e.g. nixosTests.postgresql for all versions at once. I was thinking about a way to test nixosTests.postgresql+ nixosTests.postgresql-wal-receiver + ... etc. all in one go, but for all versions.

Ma27 · 2024-11-02T13:27:18Z

Yes.
Maybe we want to have a small script in maintainers/scripts that lets you run tests for all postgresql versions and optionally only a subset of versions or a subset of tests?

wolfgangwalther · 2024-11-02T13:51:07Z

Maybe we want to have a small script in maintainers/scripts that lets you run tests for all postgresql versions and optionally only a subset of versions or a subset of tests?

I don't find myself in the scenario "only a subset of tests" often, I think. But of course, when working on the test itself.. you'd want to run that with all versions. So I think we should add back all the versions to each nixosTests. They could live alongside the passthru.override.

What I do find myself in often is: Run everything for a specific PG version (e.g. when adding a new major version) or run everything for all pg versions (when changing generic.nix etc.). And both includes all the extensions as well.

Both is currently annoying to do.

I wonder whether we'd actually need a top-level/release-postgresql.nix file? I haven't looked too much into that, yet, though. Could I easily build everything in there locally?

Ma27 · 2024-11-02T14:46:43Z

Hmm, it just occurred to me: are passthru.tests things even run by Hydra? IIRC this isn't the case and thus we need some kind of alternative exposure.

Given that we want a nixos/tests/postgresql subtree anyways, how about we add a default.nix in there that adds attributes for each test-case and for each version?

Then we can do nix-build nixos/tests/postgresql to run all tests.
And ecah pkgs.postgresql needs to pick the correct attributes from pkgs.nixosTests in passthru.tests (same for all extensions having test-cases).

I think Nextcloud does it that way already if you'd like to see it in the wild.

Does that sound reasonable? Are you interested in doing that? Otherwise, I'd be willing to take over soonish (at least not today).

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

Given that we want a nixos/tests/postgresql subtree anyways, how about we add a default.nix in there that adds attributes for each test-case and for each version?

That sounds good, yes.

Does that sound reasonable? Are you interested in doing that? Otherwise, I'd be willing to take over soonish (at least not today).

Yeah, I'll do that.

The one thing that will still not be possible with that is to build all non-broken extensions for one (or all) postgresql version at once, possibly even including their tests (which might not always be in nixos/tests/postgresql, because they are defined in the package itself).

Ma27 · 2024-11-02T15:05:07Z

The one thing that will still not be possible with that is to build all non-broken extensions for one (or all) postgresql version at once

I mean, it was the case already that you could only build extensions selectively or all, right?

which might not always be in nixos/tests/postgresql, because they are defined in the package itself

So if we have a default.nix in nixos/tests/postgresql.nix that exposes all tests as attributes (also for each psotgresql version), then this is also exposed via pkgs.nixosTests. This means that we could just do passthru.tests = { inherit (nixosTests.postgresql) postgresql_version postgresql_jit_version postgresql_wal_receiver_version ...; }, right?

wolfgangwalther · 2024-11-02T15:22:44Z

I mean, it was the case already that you could only build extensions selectively or all, right?

Yes. But doing nix-build -A postgresql.pkgs is really annoying, because of broken packages.

So if we have a default.nix in nixos/tests/postgresql.nix that exposes all tests as attributes (also for each psotgresql version), then this is also exposed via pkgs.nixosTests.

Hm, is it? Not sure about that. I would nixosTests.postgresql and nixosTests.postgresql-wal-receiver still to be "top-level nixosTests" attributes.. or are you thinking of nixosTests.postgresql.default and nixosTests.postgresql.wal-receiver etc. instead?

This means that we could just do passthru.tests = { inherit (nixosTests.postgresql) postgresql_version postgresql_jit_version postgresql_wal_receiver_version ...; }, right?

Hm, yeah it seems like you want nixosTests.postgresql to contain all postgresql tests as attributes. OK.

But I think the versioned attributes would still be one level below that.

I don't understand the part where you want to add a _versioned test to passthru.tests. Do you want to run tests for all postgres versions when you build postgresql.tests?

Anyway, I think I have an idea for the nixosTests/postgresql related part. Let's see what I come up with. The one thing that is still missing in my model is the extensions thing - but we can come to that later.

Ma27 · 2024-11-02T15:33:24Z

Yes. But doing nix-build -A postgresql.pkgs is really annoying, because of broken packages.

I fully agree here, I just don't have a really good idea right now.

or are you thinking of nixosTests.postgresql.default and nixosTests.postgresql.wal-receiver etc. instead?

That'd be my suggestion, yes. May I ask what downsides you see with this?

Do you want to run tests for all postgres versions when you build postgresql.tests?

No: my suggestion is to add nixosTests.postgresql.pg_16 to pkgs.postgresql_16.tests basically.

But I think the versioned attributes would still be one level below that.

Would be fine by me.

Let's see what I come up with. The one thing that is still missing in my model is the extensions thing - but we can come to that later.

Yeah sure. I think the fact that the correct postgresql is tested in postgresql.tests and a subtree we can use for ci/OWNERS are two useful improvements already.

wolfgangwalther · 2024-11-02T22:39:40Z

So, in theory this should work quite well now:

To build all tests for a single version do postgresql_15_jit.tests.
To build all versions for a single test do nixosTests.postgresql.postgresql-jit.
To build a single test for a single version, either postgresql_12.tests.postgresql-tls-client-cert or nixosTests.postgresql.postgresql-tls-client-cert.postgresql_12.
To build all tests for all versions do nixosTests.postgresql. *

The same works for each extension individually:

postgresql_13.pkgs.wal2json.tests
nixosTests.postgresql.wal2json

But - there are two problems right now:

nixosTests.postgresql.anonymizer breaks for me, when I test all versions at the same time. Each version alone seems to be no problem, though. Didn't figure out why, yet.
* Evaluating nixosTests.postgresql takes ages and a ton of memory. I wasn't able to make this any faster, yet.

wolfgangwalther · 2024-11-02T22:43:59Z

nixosTests.postgresql.anonymizer breaks for me, when I test all versions at the same time. Each version alone seems to be no problem, though. Didn't figure out why, yet.

Ah, scratch that. It just doesn't work for PG 17. Misread the output.

wolfgangwalther · 2024-11-02T22:53:20Z

It just doesn't work for PG 17.

The failure for the anonymizer test will be solved once #345260 is solved. This will make pg-dump-anon use pg17. The anonymizer test is kind of special, because it uses the same pg-dump-anon package (based on the default postgresql version) for all pg versions - it's just the anonymizer extension that is matching the pg version. pg-dump-anon with pg16 uses pg_dump v16, which doesn't work with pg17 - thus the test failure.

wolfgangwalther · 2024-11-02T22:54:28Z

Evaluating nixosTests.postgresql takes ages and a ton of memory. I wasn't able to make this any faster, yet.

So I guess this is just about evaluating a lot of VMs for all the tests. This seems to take time and I doubt we can make it much faster right now. If nix (lix) was only evaluating in parallel...

Ma27 · 2024-11-03T10:35:29Z

This seems to take time and I doubt we can make it much faster right now. If nix (lix) was only evaluating in parallel...

You may want to use nix-eval-jobs then (or nix-fast-build - that already marries it together with nom), at the expense of more RAM (but I mean, a reason why I upgraded my laptop's RAM to 40GiB was to be able to evaluate all postgresql tests locally 🙈 )

Thanks a lot!
I'll review it soonish, but it'll take a few days. I already did way more nixpkgs stuff this weekend than I intended.

wolfgangwalther · 2024-11-03T10:50:49Z

You may want to use nix-eval-jobs then (or nix-fast-build - that already marries it together with nom), at the expense of more RAM

Even more? It already used something like ~50GB right now... :D

Will have a look.

wolfgangwalther · 2024-11-08T21:45:58Z

THe merge conflicts and the note about the package argument need to be resolved. Other than that, it looks pretty good, thanks!

Merge conflicts resolved and fixed everything that deadnix reported.

wolfgangwalther · 2024-11-09T11:27:55Z

Fixed a debugging left-over in the postgis test which caused it to fail in the latest rebase.

The postgis test is currently broken on master, merging this should fix that, thus:

ZHF: #352882

Ma27 · 2024-11-09T17:19:57Z

I'm afraid we have another merge conflict 🙈

This seems to have broken years ago, because "CREATE EXTENSION pgcrypto;" etc. were added to the upstream file about 6 years ago.

Allows building all PostgreSQL versions at once with: nix-build -A postgresqlVersions Also makes it possible for nixosTests to target all PostgreSQL versions without importing the postgresql folder across the whole repo.

Manually importing postgresql packages from the /pkgs/ folder or manually importing the test from /nixos/tests/ in generic.nix is not only ugly, but also forbidden should we ever move to pkgs/by-name. We can achieve almost the same with a slightly different setup. We allow overriding the postgresql package for the test via passthru.override, to make sure that each postgresql_xx.tests.postgresql-wal-receiver is properly teted with the right version.

Same reasoning as commit before.

Restructuring the nixosTests.postgresql test a little bit to allow calling it with the specific versioned package from generic.nix.

Same reasoning as commit before.

…etches into package There is no need to fire up a whole VM just to run a two line test of creating the extension. We can use postgresqlTestExtension for that. This has the advantage that it runs with postgresqlTestHook, so without a VM, making it more portable.

Same reasoning as commit before.

…package Same reasoning as commit before.

Same reasoning as commit before.

… package Same reasoning as commit before.

…package Same reasoning as commit before.

…e folder This makes it possible to run all those tests at once by building nixosTests.postgresql and allow a simple entry to ci/OWNERS for all tests.

Avoiding "with", using the same names and basic structure in each test. Consistency is key!

Because with as many changes as in here anybody working on those test files will have merge conflicts anyway.

wolfgangwalther · 2024-11-09T17:25:08Z

I'm afraid we have another merge conflict 🙈

Resolved!

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.

github-actions bot added the 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS label Nov 1, 2024

nix-owners bot requested a review from thoughtpolice November 1, 2024 20:01

wolfgangwalther force-pushed the postgresql-passthru-tests branch from 9a6ebf0 to 6e3bf9c Compare November 1, 2024 20:03

wolfgangwalther requested a review from Ma27 November 1, 2024 20:03

Ma27 reviewed Nov 1, 2024

View reviewed changes

nixos/tests/postgresql-tls-client-cert.nix 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: 0 This PR does not cause any packages to rebuild on Linux labels Nov 2, 2024

wolfgangwalther mentioned this pull request Nov 2, 2024

postgresql: use team #352905

Merged

13 tasks

wolfgangwalther force-pushed the postgresql-passthru-tests branch from 6e3bf9c to c8986fe Compare November 2, 2024 22:33

nix-owners bot requested review from infinisil and philiptaron November 2, 2024 22:34

wolfgangwalther requested a review from Ma27 November 2, 2024 22:54

wolfgangwalther mentioned this pull request Nov 5, 2024

nixos/postgresql: change option enableTCPIP to actually mean TCP/IP. #353707

Draft

13 tasks

wolfgangwalther force-pushed the postgresql-passthru-tests branch from 6ec6813 to dadff44 Compare November 9, 2024 11:27

ofborg bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label Nov 9, 2024

wolfgangwalther added 17 commits November 9, 2024 18:23

nixosTests.pgjwt: fix test

d3feaae

This seems to have broken years ago, because "CREATE EXTENSION pgcrypto;" etc. were added to the upstream file about 6 years ago.

postgresqlVersions: init

75d51c5

Allows building all PostgreSQL versions at once with: nix-build -A postgresqlVersions Also makes it possible for nixosTests to target all PostgreSQL versions without importing the postgresql folder across the whole repo.

nixosTests.postgresql-jit: avoid manual imports

65ef738

Same reasoning as commit before.

nixosTests.postgresql-wal2json: avoid manual imports

319d82d

Same reasoning as commit before.

postgresql: add passthru.tests.postgresql

f6c2de9

Restructuring the nixosTests.postgresql test a little bit to allow calling it with the specific versioned package from generic.nix.

postgresql: add passthru.tests.postgresql-tls-client-cert

139c546

Same reasoning as commit before.

postgresqlPackages.postgis: move nixosTests.postgis into package

ecffab1

Same reasoning as commit before.

postgresqlPackages.pgjwt: make passthru.tests work with correct package

0af934a

Same reasoning as commit before.

postgresqlPackages.pgvecto-rs: make passthru.tests work with correct …

a5c41ae

…package Same reasoning as commit before.

postgresqlPackages.tsja: make passthru.tests work with correct package

6d7da20

Same reasoning as commit before.

postgresqlPackages.timescaledb: make passthru.tests work with correct…

23c19a2

… package Same reasoning as commit before.

postgresqlPackages.anonymizer: make passthru.tests work with correct …

db2d6a0

…package Same reasoning as commit before.

nixosTests.postgresql: move all postgresql related nixosTests into on…

9035573

…e folder This makes it possible to run all those tests at once by building nixosTests.postgresql and allow a simple entry to ci/OWNERS for all tests.

nixosTests.postgresql: use a common pattern throughout all tests

128244b

Avoiding "with", using the same names and basic structure in each test. Consistency is key!

nixosTests.postgresql: run nixfmt

45cef36

Because with as many changes as in here anybody working on those test files will have merge conflicts anyway.

wolfgangwalther force-pushed the postgresql-passthru-tests branch from dadff44 to 45cef36 Compare November 9, 2024 17:25

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

Ma27 approved these changes Nov 10, 2024

View reviewed changes

Ma27 merged commit 9a33346 into NixOS:master Nov 10, 2024
27 of 28 checks passed

wolfgangwalther deleted the postgresql-passthru-tests branch November 10, 2024 14:36

wolfgangwalther mentioned this pull request Nov 11, 2024

nixos/postgresql: extension based hardening relaxation #355010

Merged

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

postgresql: improve passthru.tests #352966

postgresql: improve passthru.tests #352966

wolfgangwalther commented Nov 1, 2024

Ma27 left a comment

wolfgangwalther commented Nov 2, 2024

wolfgangwalther commented Nov 2, 2024

Ma27 commented Nov 2, 2024

wolfgangwalther commented Nov 2, 2024

Ma27 commented Nov 2, 2024

wolfgangwalther commented Nov 2, 2024

Ma27 commented Nov 2, 2024

wolfgangwalther commented Nov 2, 2024

Ma27 commented Nov 2, 2024

wolfgangwalther commented Nov 2, 2024

Ma27 commented Nov 2, 2024

wolfgangwalther commented Nov 2, 2024

wolfgangwalther commented Nov 2, 2024

wolfgangwalther commented Nov 2, 2024

wolfgangwalther commented Nov 2, 2024

Ma27 commented Nov 3, 2024

wolfgangwalther commented Nov 3, 2024

wolfgangwalther commented Nov 8, 2024

wolfgangwalther commented Nov 9, 2024

Ma27 commented Nov 9, 2024

wolfgangwalther commented Nov 9, 2024

postgresql: improve passthru.tests #352966

postgresql: improve passthru.tests #352966

Conversation

wolfgangwalther commented Nov 1, 2024

Things done

Ma27 left a comment

Choose a reason for hiding this comment

wolfgangwalther commented Nov 2, 2024

wolfgangwalther commented Nov 2, 2024

Ma27 commented Nov 2, 2024

wolfgangwalther commented Nov 2, 2024

Ma27 commented Nov 2, 2024

wolfgangwalther commented Nov 2, 2024

Ma27 commented Nov 2, 2024

wolfgangwalther commented Nov 2, 2024

Ma27 commented Nov 2, 2024

wolfgangwalther commented Nov 2, 2024

Ma27 commented Nov 2, 2024

wolfgangwalther commented Nov 2, 2024

wolfgangwalther commented Nov 2, 2024

wolfgangwalther commented Nov 2, 2024

wolfgangwalther commented Nov 2, 2024

Ma27 commented Nov 3, 2024

wolfgangwalther commented Nov 3, 2024

wolfgangwalther commented Nov 8, 2024

wolfgangwalther commented Nov 9, 2024

Ma27 commented Nov 9, 2024

wolfgangwalther commented Nov 9, 2024