fix(nix): use the same postgresql inside switch-ext-version #1990
+12
−3
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
currently, `switch-ext-version` uses `nixpkgs.postgresql`, instead of the `postgresql` chosen from outside.
WalkthroughA new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Repository UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (3)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
nix/packages/postgres.nix (1)
144-150: Good refactoring; note partial duplication remains.The introduction of the
postgres-pkgsbinding eliminates redundant computation withinmakePostgresBin. However,makeOurPostgresPkgsSet(line 117) still independently callsmakeOurPostgresPkgs, so the full extension set is computed twice: once inmakePostgresBinand once inmakeOurPostgresPkgsSet(invoked frommakePostgresat line 173).This partial duplication is not a critical issue but could be optimized in a future refactor.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
nix/packages/postgres.nix
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: pg_net - Postgres 17 (aarch64-darwin)
- GitHub Check: pg_partman - Postgres orioledb-17 (aarch64-darwin)
- GitHub Check: plpgsql_check - Postgres 17 (aarch64-darwin)
- GitHub Check: pg_cron - Postgres 17 (aarch64-darwin)
- GitHub Check: pg_partman - Postgres 17 (aarch64-darwin)
- GitHub Check: no packages to build (aarch64-linux)
🔇 Additional comments (2)
nix/packages/postgres.nix (2)
111-111: LGTM!The change from
pkgs.callPackagetoextCallPackagecorrectly applies the fix to all extensions, ensuring they receive the correctpostgresqlversion in their build environment.
105-109: Fix correctly uses recursive let binding to inject correct postgresql version.The pattern is valid and commonly used in Nix—let bindings are lazy, allowing recursive references.
callPackageWithinjects the correctpostgresqlversion into all extension modules' scope, resolving the original issue where switch-ext-version was using the nixpkgs default instead of the selected version. The mechanism works because when each extension is loaded,extCallPackageis already bound in the scope.
LGUG2Z
approved these changes
Dec 23, 2025
samrose
requested changes
Dec 23, 2025
Collaborator
There was a problem hiding this comment.
Not actually asking for changes yet, but when I trigger a rebuild of wrappers package seems to nearly crash my computer. Just wanting to investigate that aspect before merge, since these changes don't trigger a full rebuild of rust extensions, and we need to make sure our CI can rebuild in a fault tolerant way. Update see comment below #1990 (review)
samrose
approved these changes
Dec 23, 2025
Collaborator
samrose
left a comment
samrose
left a comment
There was a problem hiding this comment.
restoring approval but we'll just document e2e test here prior to merge
|
Will run the smoke tests on this branch and report back |
|
The engine smoke scenario across the 15 17 17-oriole matrix which runs all the SQL tests in nix/tests passed |
|
Waiting on the upgrade smoke scenario results, will report back in about an hour when it's done for 15->17 and 17->17 |
|
All good |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What kind of change does this PR introduce?
Currently,
switch-ext-versionusesnixpkgs.postgresql, instead of thepostgresqlchosen insidenix/packages/postgres.nix.To test for this, we can add
builtins.trace postgresql.drvPathinsidenix/packages/switch-ext-version.nix:What is the current behavior?
postgresqlhas version 15.14, even though we chosepsql_17.What is the new behavior?
Correct one is chosen.
Additional context
There are more stuff that I'd like to propose to change. For instance, there are IFDs for
Cargo.lockthat I believe would be pretty simple to change.Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.