Skip to content

Conversation

@jfroche
Copy link
Collaborator

@jfroche jfroche commented Jul 21, 2025

Use one recent version of nixpkgs in the flake inputs.

a76c4553d7e741e17f289224eda135423de0491d -> f61125a668a320878494449750330ca58b78c557

Summary by CodeRabbit

  • New Features

    • Tests/exension packaging now expose installed-extension metadata and provide JIT on/off toggles; host-platform resolution improved.
  • Bug Fixes

    • PostgreSQL 17 compatibility fixes and build robustness improvements for older toolchains and tool versions.
  • Updates

    • PostgreSQL runtime artifacts updated to new packaged releases; dependency inputs reorganized and many build hashes refreshed; several Darwin-specific conditionals simplified/removed.
  • Database

    • Migration version column length constraint removed for greater flexibility.

✏️ Tip: You can customize this high-level summary in your review settings.

@jfroche jfroche force-pushed the update-nixpkgs branch 2 times, most recently from a3275e8 to 98633c4 Compare July 21, 2025 22:29
@jfroche jfroche marked this pull request as ready for review July 22, 2025 13:39
@jfroche jfroche requested review from a team as code owners July 22, 2025 13:39
@samrose
Copy link
Collaborator

samrose commented Aug 8, 2025

rebased on develop, and will e2e test ami for each major version

@samrose samrose force-pushed the update-nixpkgs branch 2 times, most recently from c061085 to 07fb5e2 Compare September 2, 2025 13:28
@samrose samrose force-pushed the update-nixpkgs branch 2 times, most recently from 6b51806 to a33cf5d Compare September 10, 2025 17:58
@samrose samrose force-pushed the update-nixpkgs branch 2 times, most recently from 37962e0 to 52722b1 Compare November 18, 2025 15:45
@yvan-sraka yvan-sraka self-assigned this Nov 19, 2025
@yvan-sraka yvan-sraka marked this pull request as draft November 19, 2025 21:13
Copy link
Collaborator

@samrose samrose left a comment

Choose a reason for hiding this comment

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

adding request changes just to stop merge until fully tested

@yvan-sraka yvan-sraka force-pushed the update-nixpkgs branch 3 times, most recently from f470b85 to dc596ec Compare November 25, 2025 15:57
yvan-sraka and others added 21 commits January 5, 2026 18:21
…64-darwin

Version 0.19.5 has a dangling pointer issue in src/core.c:177 that causes
compilation to fail on aarch64-darwin with newer clang versions. This adds
-Wno-error=dangling-assignment to allow compilation to proceed.

The flag is restricted to aarch64-darwin only since GCC on Linux doesn't
recognize this warning option.
Add -headerpad_max_install_names linker flag to prevent 'install_name_tool:
changing install names or rpaths can't be redone' errors on macOS. This
allocates sufficient header space for install_name_tool to modify the
shared library during post-install processing.
… extensions

Test assertions were inconsistent: expecting {version}.so and {extension_name}-{version}.so
patterns while actual libraries use {lib_name}-{version}.so (e.g. pg_partman_bgw-5.3.1.so,
timescaledb-loader-2.16.1.so). Updated PostgresExtensionTest to accept Optional[str] lib_name
parameter and use consistent {lib_name}-{version}.so assertions throughout.

Added missing timescaledb-loader.so symlink creation and updated the switch script to
maintain both symlinks when switching versions.

Fixes 'Expected timescaledb version 2.16.1, but found timescaledb-loader-2.16.1.so' and
'Expected pg_partman version 5.3.1, but found pg_partman_bgw-5.3.1.so' test failures.
Use overlay instead of passing full nixpkgs-oldstable to avoid exposing
entire old nixpkgs attribute set. Now provides specific packages
(curl_8_4, v8_oldstable) through overlay.
Keep revision support that was accidentally simplified during nixpkgs update.
Restores conditional logic to support building orioledb from specific git
revision hashes instead of only tags.
These files were moved to subdirectories and the old .nix files should be
removed since they were moved to new directory structure with default.nix files.
Remove nixpkgs-go124 dependency since current nixpkgs has Go 1.25.4
which is sufficient for packer. Remove nix-fast-build input as unused
and add follows directives to resolve duplicate dependencies.
Run flake-linter to eliminate duplicate dependencies, and remove unused
cargo-pgrx / rustc versions hashes.
Move pgrouting.nix to pgrouting/default.nix and groonga package to
pgroonga/groonga.nix. Update imports and fix relative paths for versions.json
and patch files.
Replace inline bash script generation with writeShellScriptBin for rustc
wrapper that filters empty postmaster_stub.rs arguments. Apply wrapper only
for pgrx < 0.12 since issue was fixed upstream in pgrx#1435 and pgrx#1441.
Move nixpkgs-oldstable import to a let binding to avoid importing it
three times...
Remove supabase-groonga from global flake packages and import it locally
in pgroonga extension instead. Other components access groonga via
pgroonga.passthru.groonga since groonga is only used by pgroonga.
The supabase-groonga package was moved to pgroonga extension in commit 1f0ed1f,
but Ansible was still trying to install it separately causing build failures.
This reverts commit 55474686405394826ad07a032cbcf0e36a924554.
This reverts commit cfa9bcb92219f391469ae743f95c8856b0a99561.
Remove references to darwin.apple_sdk.frameworks.{IOKit,CoreFoundation}
which have been deprecated in nixpkgs. Disable CGO to avoid Darwin
framework dependencies entirely.
With this new nixpkgs update come gcc 15, which introduced stricter C standard compliance. plpgsql-check fails to build due to this.
To fix the build, we explicitly set the C standard to gnu17.
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 5

Fix all issues with AI Agents 🤖
In @nix/cargo-pgrx/buildPgrxExtension.nix:
- Line 151: The Darwin-specific Security framework was removed from buildInputs
(buildInputs and darwin.apple_sdk.frameworks.Security) without explanation;
either restore it for macOS builds or add a clear comment/commit note proving
it's unnecessary and that RUSTFLAGS-based handling (RUSTFLAGS / stdenv.isDarwin)
still satisfies all macOS link-time requirements. Fix by making buildInputs
conditional on stdenv.isDarwin to include darwin.apple_sdk.frameworks.Security
when building on Darwin, or if you intentionally removed it, add a comment and
update the commit message explaining why macOS linking remains correct without
the framework and reference the RUSTFLAGS changes that replace it.

In @nix/ext/pg_jsonschema/default.nix:
- Around line 83-93: The doCheck conditional currently disables tests for
versions ["0.2.0","0.3.1","0.3.3"], which reduces coverage; inspect the test
harness in those specific releases (look at version tag checks and pgrx test
invocation in the repo) to confirm why they fail, and either (A) add a targeted
alternative test path for those versions (for example implement a custom
installcheck script similar to pg_graphql and wire it into the build for the
affected versions), or (B) modify the Nix build to run pgrx tests outside the
sandbox/with sandbox disabled in CI for those versions, and if neither is
feasible add an explicit NOTE in the commit/PR describing the untested versions
and the mitigation chosen; update the doCheck clause only after implementing one
of these fixes and reference the version list and doCheck symbol in the commit
message.

In @nix/ext/pgrouting/default.nix:
- Around line 61-69: The cmakeFlags array includes an unexplained
"-DCMAKE_POLICY_VERSION_MINIMUM=3.5" which conflicts with pgrouting's own
cmake_minimum_required (3.2 for older releases or 3.12 currently); either remove
that flag from cmakeFlags or replace it with a brief comment explaining why 3.5
is required for this package build and which pgrouting versions need it; locate
and edit the cmakeFlags definition (the cmakeFlags variable and the
"-DCMAKE_POLICY_VERSION_MINIMUM=3.5" entry) or add an adjacent comment
clarifying the rationale and affected pgrouting versions.

In @nix/ext/plv8/default.nix:
- Around line 38-48: The inner per-derivation v8 attribute and duplicate
conditional in buildInputs are dead/redundant because all versions <= 3.1.10, so
remove the conditional v8 attribute inside the build (the v8 = (if
(builtins.compareVersions "3.1.10" version >= 0) then v8 else nodejs_20.libv8)
line and any duplicate conditional in buildInputs) and rely on the outer v8 =
v8_oldstable binding; if you want future-proofing instead of removing logic,
centralize version-specific selection into a single helper (e.g.,
selectV8(version)) used by the build function rather than duplicating
conditionals in stdenv.mkDerivation and buildInputs.

In @testinfra/test_ami_nix.py:
- Around line 790-792: The test currently uses "assert False, (\"PostgREST logs
contain 'session is not read-only' errors even though PostgreSQL is configured
for read-only mode\")" which can be stripped out with Python's -O optimization;
replace that assert with an explicit exception by raising AssertionError with
the same message (i.e., use raise AssertionError("PostgREST logs contain
'session is not read-only' errors even though PostgreSQL is configured for
read-only mode")) so the failure cannot be bypassed by optimizations and still
provides the same diagnostic text.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
nix/ext/pg_jsonschema/default.nix (1)

48-57: Fix non-existent versions in previousVersions array.

The previousVersions array includes versions that don't exist in nix/ext/versions.json: 0.3.0, 0.1.4, 0.1.3, and 0.1.2. These missing versions will cause build failures when the postBuild script attempts to generate migration files.

Only these versions are available: 0.1.0, 0.1.1, 0.2.0, 0.3.1 (and 0.3.3, which appears to be the current version).

Update the previousVersions array to include only versions that exist in versions.json.

♻️ Duplicate comments (4)
migrations/schema-15.sql (1)

640-640: LGTM - Already discussed and accepted.

The change from character varying(128) to character varying is fully explained in the past review comments. This results from updating dbmate (2.14.0 → 2.28.0) as part of the nixpkgs upgrade, and the team has accepted this change. The modification is consistent across all three schema migration files (schema-15.sql, schema-17.sql, and schema-orioledb-17.sql).

flake.nix (1)

10-29: Verify no duplicate dependencies after input restructuring.

Per previous review discussion, consider running nix run github:Mic92/flake-linter on this branch to ensure the restructured inputs don't introduce duplicate dependencies in the flake lock.

nix/ext/tests/plpgsql_check.nix (1)

22-26: Same withJIT/withoutJIT pattern as other test files.

This file exhibits the same pattern where both withJIT and withoutJIT point to the same pkg. See the review comment on nix/ext/tests/pgjwt.nix for the detailed concern about JIT differentiation.

nix/ext/tests/postgis.nix (1)

22-26: Same withJIT/withoutJIT pattern as other test files.

This file exhibits the same pattern where both withJIT and withoutJIT point to the same pkg. See the review comment on nix/ext/tests/pgjwt.nix for the detailed concern about JIT differentiation.

🧹 Nitpick comments (5)
nix/packages/lib.nix (1)

118-126: Defensive error handling is a solid improvement.

The explicit copy operations with error messages and exit-on-failure make build failures easier to diagnose. The success message on line 126 confirms the copy phase completed.

One minor observation: pg_ident.conf (line 123) and the extension scripts (line 124) don't receive explicit chmod operations like the other config files (lines 127-130). If default permissions are sufficient, this is fine; otherwise, consider adding chmod 644 for consistency.

nix/ext/tests/orioledb.nix (1)

18-22: Clarify JIT toggle implementation.

The installedExtensions attribute correctly exposes the extension metadata. However, both withJIT and withoutJIT point to the same pkg variable, which means no actual JIT toggling occurs. If this is a placeholder for future JIT support or if JIT toggling isn't relevant for orioledb tests, please confirm this is the intended behavior.

nix/ext/tests/pgmq.nix (1)

22-26: Clarify JIT toggle implementation.

The installedExtensions attribute correctly exposes the extension metadata. However, both withJIT and withoutJIT point to the same pkg variable, which means no actual JIT toggling occurs. If this is a placeholder for future JIT support or if JIT toggling isn't relevant for pgmq tests, please confirm this is the intended behavior.

nix/postgresql/generic.nix (1)

345-345: Clarify intent of empty teams array.

Setting teams = [ ] appears intentional but unconventional. If this is meant to satisfy a required schema field or explicitly indicate no team ownership, a brief comment would help future maintainers understand the intent.

nix/ext/plv8/default.nix (1)

136-158: Consider consolidating duplicate patchelf logic.

The patchelf commands in both the PostgreSQL 15 branch (lines 144-146) and the else branch (lines 155-157) are identical. If no differentiation is planned, this could be simplified.

🔎 Suggested simplification
-        if [ "${lib.versions.major postgresql.version}" = "15" ]; then
-          ${lib.optionalString stdenv.isDarwin ''
-            install_name_tool -add_rpath "${v8}/lib" $out/lib/$LIB_NAME
-            install_name_tool -add_rpath "${postgresql}/lib" $out/lib/$LIB_NAME
-            install_name_tool -add_rpath "${libcxx}/lib" $out/lib/$LIB_NAME
-            install_name_tool -change @rpath/libv8_monolith.dylib ${v8}/lib/libv8_monolith.dylib $out/lib/$LIB_NAME
-          ''}
-
-          ${lib.optionalString (!stdenv.isDarwin) ''
-            ${patchelf}/bin/patchelf --set-rpath "${v8}/lib:${postgresql}/lib:${libcxx}/lib" $out/lib/$LIB_NAME
-          ''}
-        else
-          ${lib.optionalString stdenv.isDarwin ''
-            install_name_tool -add_rpath "${v8}/lib" $out/lib/$LIB_NAME
-            install_name_tool -add_rpath "${postgresql}/lib" $out/lib/$LIB_NAME
-            install_name_tool -add_rpath "${libcxx}/lib" $out/lib/$LIB_NAME
-            install_name_tool -change @rpath/libv8_monolith.dylib ${v8}/lib/libv8_monolith.dylib $out/lib/$LIB_NAME
-          ''}
-
-          ${lib.optionalString (!stdenv.isDarwin) ''
-            ${patchelf}/bin/patchelf --set-rpath "${v8}/lib:${postgresql}/lib:${libcxx}/lib" $out/lib/$LIB_NAME
-          ''}
-        fi
+        ${lib.optionalString stdenv.isDarwin ''
+          install_name_tool -add_rpath "${v8}/lib" $out/lib/$LIB_NAME
+          install_name_tool -add_rpath "${postgresql}/lib" $out/lib/$LIB_NAME
+          install_name_tool -add_rpath "${libcxx}/lib" $out/lib/$LIB_NAME
+          install_name_tool -change @rpath/libv8_monolith.dylib ${v8}/lib/libv8_monolith.dylib $out/lib/$LIB_NAME
+        ''}
+
+        ${lib.optionalString (!stdenv.isDarwin) ''
+          ${patchelf}/bin/patchelf --set-rpath "${v8}/lib:${postgresql}/lib:${libcxx}/lib" $out/lib/$LIB_NAME
+        ''}
📜 Review details

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between dfbdf86 and acf3ba4.

⛔ Files ignored due to path filters (1)
  • flake.lock is excluded by !**/*.lock
📒 Files selected for processing (54)
  • ansible/vars.yml
  • flake.nix
  • migrations/schema-15.sql
  • migrations/schema-17.sql
  • migrations/schema-orioledb-17.sql
  • nix/cargo-pgrx/buildPgrxExtension.nix
  • nix/cargo-pgrx/default.nix
  • nix/cargo-pgrx/versions.json
  • nix/checks.nix
  • nix/ext/gdal.nix
  • nix/ext/pg_jsonschema/default.nix
  • nix/ext/pg_net.nix
  • nix/ext/pgaudit.nix
  • nix/ext/pgroonga/default.nix
  • nix/ext/pgroonga/groonga.nix
  • nix/ext/pgrouting/default.nix
  • nix/ext/pgrouting/pgrouting-3.4.1-pg17.patch
  • nix/ext/plpgsql-check.nix
  • nix/ext/plv8/default.nix
  • nix/ext/postgis.nix
  • nix/ext/tests/default.nix
  • nix/ext/tests/http.nix
  • nix/ext/tests/lib.py
  • nix/ext/tests/orioledb.nix
  • nix/ext/tests/pg_plan_filter.nix
  • nix/ext/tests/pg_repack.nix
  • nix/ext/tests/pg_safeupdate.nix
  • nix/ext/tests/pgjwt.nix
  • nix/ext/tests/pgmq.nix
  • nix/ext/tests/pgroonga.nix
  • nix/ext/tests/pgrouting.nix
  • nix/ext/tests/pgsodium.nix
  • nix/ext/tests/plpgsql_check.nix
  • nix/ext/tests/plv8.nix
  • nix/ext/tests/postgis.nix
  • nix/ext/tests/timescaledb.nix
  • nix/ext/tests/vault.nix
  • nix/ext/timescaledb.nix
  • nix/ext/vault.nix
  • nix/ext/wal2json.nix
  • nix/ext/wrappers/default.nix
  • nix/nixpkgs.nix
  • nix/overlays/default.nix
  • nix/packages/default.nix
  • nix/packages/groonga/do-not-use-vendored-libraries.patch
  • nix/packages/groonga/fix-cmake-install-path.patch
  • nix/packages/lib.nix
  • nix/packages/packer.nix
  • nix/packages/postgres.nix
  • nix/packages/sfcgal.nix
  • nix/packages/supascan.nix
  • nix/packages/sync-exts-versions.nix
  • nix/postgresql/generic.nix
  • testinfra/test_ami_nix.py
💤 Files with no reviewable changes (4)
  • nix/packages/sfcgal.nix
  • nix/overlays/default.nix
  • nix/packages/groonga/do-not-use-vendored-libraries.patch
  • nix/packages/groonga/fix-cmake-install-path.patch
🧰 Additional context used
🪛 Ruff (0.14.10)
testinfra/test_ami_nix.py

790-790: Do not assert False (python -O removes these calls), raise AssertionError()

Replace assert False

(B011)

🔇 Additional comments (59)
nix/ext/pgroonga/default.nix (1)

60-61: LGTM! Formatting improvement.

The reformatting of the buildInputs list improves readability while maintaining identical semantics.

nix/ext/gdal.nix (1)

50-57: LGTM! Formatting improvement.

The reformatting of cmakeFlags into a flat list with clearly separated platform-specific options improves readability without altering behavior.

nix/ext/pgaudit.nix (1)

187-200: LGTM! Formatting improvement.

The reflowed indentation improves readability while preserving the identical logic for conditional migration symlink creation.

nix/ext/vault.nix (1)

47-70: LGTM! Formatting improvement.

The reformatted installPhase improves readability through clearer indentation and line breaks while maintaining identical build logic.

migrations/schema-orioledb-17.sql (1)

655-655: LGTM - Coordinated schema change across all migration files.

The removal of the length constraint on the version column (from character varying(128) to character varying) is consistent across all three schema migration files in this PR. As explained in the past review comments on schema-15.sql, this change results from updating dbmate from version 2.14.0 to 2.28.0, and the team has accepted this change to keep the tool up to date.

migrations/schema-17.sql (1)

641-641: LGTM - Coordinated schema change.

This change mirrors the identical modification in schema-15.sql and schema-orioledb-17.sql, resulting from the dbmate version update (2.14.0 → 2.28.0) included in the nixpkgs upgrade.

nix/packages/lib.nix (2)

103-104: LGTM!

The formatting adjustment and inline comment improve readability without changing behavior.


133-137: LGTM!

The multi-line reformatting of the nested builtins expression improves readability without altering the substitution logic.

ansible/vars.yml (1)

13-15: These version changes are intentional as part of the nixpkgs update and require no action.

The version string updates on lines 13-15 reflect a rebuild with updated nixpkgs dependencies (commit acf3ba4: feat: update nixpkgs and fix plpgsql-check build with gcc 15). The build numbers (064, 021) represent iteration numbers for the nixpkgs update cycle (indicated by the -update-nixpkgs-5 suffix), not a rollback. These versions are generated by the nix build system and used directly in the AMI provisioning pipeline; they will exist after the build completes. No verification of pre-existing artifacts is needed.

Likely an incorrect or invalid review comment.

nix/ext/wal2json.nix (1)

13-13: This review comment is incorrect. The versions.json file for wal2json does not contain rev fields—only postgresql and hash entries. The parameter rename to _rev correctly reflects that the passed value is unused, and the revision is deterministically computed from the version string on line 20. No silent behavior change occurs because there were no rev values in versions.json to ignore in the first place.

Likely an incorrect or invalid review comment.

nix/ext/pg_jsonschema/default.nix (1)

45-45: BuildInputs simplification is sound. The change to include only postgresql aligns with the PR's goal of reducing platform-specific dependencies. Darwin-specific build configuration is preserved in the env block (lines 60-69) with appropriate RUSTFLAGS for dynamic linking on macOS.

testinfra/test_ami_nix.py (1)

435-437: LGTM: Assertion formatting is clear and concise.

The single-line assertion format improves readability while preserving the same truth conditions and error messages.

Also applies to: 560-562, 592-594, 627-629

nix/ext/plpgsql-check.nix (1)

43-44: The gnu17 C standard flag correctly resolves GCC 15 build issues.

GCC 15 defaults to C23 (gnu23), which breaks PostgreSQL extensions expecting gnu17 behavior. Setting -std=gnu17 is the recommended approach per GCC 15 porting documentation and is widely used across PostgreSQL extension builds. The fix is appropriate and compatible with all supported versions.

nix/packages/sync-exts-versions.nix (1)

14-15: nix_2_29 is available and stable in nixpkgs.

Verification confirms that nixVersions.nix_2_29 is available in nixpkgs with multiple stable releases (2.29.0, 2.29.1, 2.29.2, 2.29.3). The version jump from nix_2_20 to nix_2_29 is a significant upgrade, but the target version is a documented, stable release suitable for this tooling.

nix/ext/tests/pgroonga.nix (1)

20-27: LGTM! Passthru attributes align with the broader test environment pattern.

The added installedExtensions, withJIT, and withoutJIT passthru attributes provide consistency across PostgreSQL extension test environments. Having withJIT and withoutJIT point to the same pkg is appropriate for this test context where JIT toggling isn't exercised.

nix/postgresql/generic.nix (3)

118-126: Good: ICU version pinned to maintain collation stability.

Pinning ICU to version 75 with the clear comment about collation version 153.120 is a sound approach to prevent collation mismatch warnings during nixpkgs upgrades. This is important for database integrity.


69-69: Dynamic OrioleDB detection looks correct.

The pattern "[0-9][0-9]_.*" correctly matches OrioleDB version strings like "17_1" while not matching standard PostgreSQL versions like "17.6.1".


159-159: No compatibility issues identified with __structuredAttrs = true.

The derivation correctly implements structured attributes: environment variables are properly placed under the env attribute (e.g., env.NIX_CFLAGS_COMPILE), configureFlags and buildFlags are defined as lists, and no custom build hooks make assumptions about environment variable exports. PostgreSQL's standard autoconf configure script works correctly with this approach. The implementation follows nixpkgs best practices.

nix/ext/tests/lib.py (2)

24-42: Good addition of lib_name parameter for flexible library naming.

The lib_name parameter with fallback to extension_name maintains backward compatibility while supporting extensions where the shared library name differs from the extension name (e.g., timescaledb-loader). Clean implementation.


169-171: Address inconsistent lib_name parameter passing across test files.

The pattern change from {version}.so to {lib_name}-{version}.so is correct. However, test files use inconsistent approaches:

  • default.nix and timescaledb.nix explicitly pass lib_name to PostgresExtensionTest
  • Individual test files (pgjwt.nix, plv8.nix, pgmq.nix, etc.) omit the lib_name parameter, relying on the default where lib_name = extension_name

This works only when the library name matches the extension name. For consistency and to support extensions with different library names (like timescaledb-loader), all callers should explicitly pass lib_name from extension metadata, following the default.nix pattern.

nix/packages/supascan.nix (2)

1-4: Good simplification of Go toolchain binding.

Using pkgs.go_1_24 directly instead of a separate nixpkgs input aligns with the broader flake input consolidation in this PR.


41-42: Disabling CGO is appropriate for supascan.

CGO_ENABLED = "0" correctly removes Darwin framework dependencies while maintaining full functionality. Supascan is a Linux infrastructure tool that reads /proc, /sys, and filesystem metadata—all pure-Go operations. The gopsutil library used for optional process/port scanning is implemented in pure Go on Linux without requiring CGO. No functionality is lost with this change.

flake.nix (1)

22-25: Good: nixpkgs-oldstable pin with clear documentation.

The comment at lines 22-24 clearly explains why this specific nixpkgs version is pinned (compatible v8 and curl versions for extensions). This provides important context for future maintainers.

nix/nixpkgs.nix (1)

13-25: Good implementation addressing previous review feedback.

The overlay correctly uses a let-binding to import nixpkgs-oldstable and exposes only the specific packages needed (curl_8_6, v8_oldstable) rather than the full attribute set. This follows the suggestions from previous reviews.

nix/checks.nix (1)

4-10: LGTM! Cosmetic reordering of perSystem arguments.

Moving lib after pkgs in the destructuring pattern is a stylistic change with no functional impact.

nix/ext/pg_net.nix (2)

10-15: LGTM! Curl versioning approach addresses past feedback.

The curl_8_6 parameter with let-binding follows the overlay-based approach suggested in the past review comment, providing a cleaner interface than passing the full nixpkgs-oldstable attributeset.


52-59: LGTM! Version and platform-specific compiler flag handling is appropriate.

The multi-branch conditional correctly handles compiler warning suppression for different versions and platforms. The Darwin aarch64-specific case is well-documented.

nix/ext/tests/pgjwt.nix (1)

22-26: Verify withJIT and withoutJIT implementation.

Both withJIT and withoutJIT attributes point to the same pkg, which suggests JIT compilation is not yet differentiated. This pattern appears across multiple test files (plpgsql_check.nix, postgis.nix, and others mentioned in the AI summary).

Please confirm whether:

  • This is an intentional placeholder awaiting future JIT support implementation, or
  • JIT-enabled and JIT-disabled PostgreSQL builds should reference distinct packages

If these are intended as placeholders, consider adding a TODO comment to track the future implementation of proper JIT toggle support.

nix/cargo-pgrx/versions.json (1)

1-119: Cargo hash updates look correct.

The cargoHash values have been updated across multiple cargo-pgrx and Rust version combinations, which is expected when rebuilding with updated nixpkgs. No structural changes were made to the JSON.

nix/cargo-pgrx/default.nix (2)

73-73: Cargo hash updates are correct.

The cargoHash values for versions 0.12.6, 0.12.9, and 0.14.3 match the corresponding updates in versions.json.

Also applies to: 78-78, 83-83


36-36: Remove this comment — the premises are factually incorrect.

The Security framework is not present in the current code and has no history of being in this file. Additionally, lines 47–50 contain checkFlags with test directives, not Darwin-specific environment variables. The OPENSSL variables (lines 38–41) are set unconditionally for all platforms, not Darwin-specific.

Likely an incorrect or invalid review comment.

nix/ext/pgrouting/pgrouting-3.4.1-pg17.patch (1)

1-39: PostgreSQL 17 compatibility patch is correct.

The patch correctly addresses the PostgreSQL 17 API change where SPI_getvalue now returns char* directly instead of Datum. Removing the DatumGetCString wrapper resolves the compilation error and aligns with the PG17 API.

nix/ext/wrappers/default.nix (1)

39-42: Verify Darwin builds are supported without frameworks in buildInputs.

The wrappers package does not include Darwin frameworks in buildInputs—only openssl and postgresql. However, Darwin-specific support is properly maintained through environment variables at lines 47-50 (POSTGRES_LIB and RUSTFLAGS with dynamic_lookup), which are the correct approach for pgrx extensions on Darwin. Verify that the build succeeds on Darwin with this configuration.

nix/ext/pgroonga/groonga.nix (1)

1-1: groonga.nix is a necessary dependency file and should not be removed.

This file builds the Groonga library package (supabase-groonga) that is then consumed by nix/ext/pgroonga/default.nix, which builds the PGroonga PostgreSQL extension. These are two distinct, complementary files serving different purposes:

  • groonga.nix: Builds the Groonga C/C++ library using CMake
  • default.nix: Builds the PGroonga PostgreSQL extension that depends on the Groonga library

The file is actively referenced in nix/packages/default.nix:44 and cannot be merged or removed without breaking the build chain.

Likely an incorrect or invalid review comment.

nix/ext/tests/pgsodium.nix (1)

23-23: LGTM with a note on JIT attributes.

The addition of installedExtensions, withJIT, and withoutJIT passthru attributes follows a consistent pattern applied across multiple test files in this PR.

Note: Both withJIT and withoutJIT currently point to the same pkg. If these attributes are intended to provide distinct PostgreSQL builds with different JIT configurations, this may need adjustment. However, if JIT toggling occurs elsewhere or these are placeholders for future functionality, the current implementation is acceptable.

Also applies to: 26-27

nix/ext/tests/http.nix (1)

22-22: LGTM - consistent passthru attributes.

The passthru attributes (installedExtensions, withJIT, withoutJIT) match the pattern consistently applied across extension test files in this PR.

Also applies to: 25-26

nix/ext/tests/plv8.nix (1)

23-23: LGTM - consistent passthru attributes.

The passthru attributes align with the systematic changes applied across all extension test files.

Also applies to: 26-27

nix/ext/pgrouting/default.nix (2)

52-54: Patch file exists and is correctly referenced.

The conditional patch for pgrouting 3.4.1 with PostgreSQL 17 is correctly scoped to the specific version combination. The patch file exists at nix/ext/pgrouting/pgrouting-3.4.1-pg17.patch as expected.


15-15: The versions.json path is correct.

The relative path ../versions.json from nix/ext/pgrouting/default.nix correctly resolves to nix/ext/versions.json, which exists in the repository. The path change is aligned with the actual file location.

nix/ext/postgis.nix (1)

19-19: sfcgal integration is correct. The sfcgal dependency is properly added as a function parameter (line 19), included in buildInputs (line 81), and passed to the PostGIS configure script (line 94 with --with-sfcgal). The implementation correctly aligns with the PR goal of using nixpkgs packages as external dependencies. No version incompatibility issues are evident from the codebase.

nix/ext/tests/pg_plan_filter.nix (1)

22-22: LGTM! Passthru standardization.

The addition of installedExtensions, withJIT, and withoutJIT attributes standardizes the public interface for PostgreSQL extension tests, aligning with the pattern applied across multiple test files in this PR.

Also applies to: 25-26

nix/ext/tests/vault.nix (1)

23-23: LGTM! Passthru standardization.

The addition of installedExtensions, withJIT, and withoutJIT attributes standardizes the public interface, consistent with the pattern applied across extension test files.

Also applies to: 26-27

nix/ext/tests/default.nix (2)

32-34: LGTM! Passthru standardization.

The addition of withJIT, withoutJIT, and installedExtensions attributes aligns with the standardization pattern applied across extension test files.


171-171: PostgresExtensionTest signature correctly accepts lib_name parameter.

The call at line 171 passes lib_name as the seventh argument, which matches the constructor signature in nix/ext/tests/lib.py. The parameter is defined as lib_name: Optional[str] = None with a default value, and the implementation is correct.

nix/ext/tests/pg_repack.nix (1)

22-22: LGTM! Passthru standardization.

The addition of installedExtensions, withJIT, and withoutJIT attributes completes the standardization pattern applied consistently across PostgreSQL extension test files.

Also applies to: 25-26

nix/packages/packer.nix (1)

9-9: This review comment is based on an incorrect premise about the change.

The file nix/packages/packer.nix was newly created in this commit, not modified to remove a Go version override. There is no evidence of a Go 1.24 override being removed—the file was added fresh and contains no Go version specification. The derivation does use the default Go from nixpkgs via buildGoModule = pkgs.buildGoModule, but this was not a result of removing an override; it's simply how the package was initially defined.

If the intent is to verify that Packer 1.14.1 builds successfully with the updated nixpkgs, that is a reasonable verification step, but the comment's explanation about removing an override is incorrect.

Likely an incorrect or invalid review comment.

nix/ext/tests/pgrouting.nix (1)

26-26: LGTM: Passthru attributes extended consistently.

The addition of installedExtensions, withJIT, and withoutJIT to the passthru interface aligns with the PR's systematic pattern across multiple extension tests to expose extension metadata and JIT-toggle attributes.

Also applies to: 29-30

nix/ext/tests/pg_safeupdate.nix (1)

22-22: LGTM: Passthru attributes extended consistently.

The passthru additions mirror the pattern established across other extension tests in this PR, providing a uniform interface for extension metadata.

Also applies to: 25-26

nix/cargo-pgrx/buildPgrxExtension.nix (1)

88-108: Rustc wrapper implementation aligns with upstream fix backport.

The rustc wrapper correctly filters empty arguments and empty postmaster_stub.rs files for cargo-pgrx < 0.12.0, addressing build failures fixed upstream in pgcentralfoundation/pgrx#1435 and #1441. The version-gated activation (needsRustcWrapper) ensures the workaround only applies to affected versions.

Based on past review comments, this is a known workaround for an issue already fixed in pgrx >= 0.12.

Also applies to: 117-119, 173-177

nix/ext/timescaledb.nix (2)

42-43: Darwin-specific linter flag looks appropriate.

Conditionally disabling the linter on Darwin with -DLINTER=OFF avoids potential Darwin-specific build issues while preserving linter functionality on other platforms.


111-111: Loader naming changes coordinate consistently across build and runtime contexts.

The additions establish dual loader naming:

  • Line 111 creates timescaledb-loader.so symlink in the version-switching script (runtime context)
  • Lines 129-130 create the same symlink during postBuild (build-time/test context)
  • Line 150 exposes libName = "timescaledb-loader" in passthru for programmatic reference

This aligns with the test changes in nix/ext/tests/timescaledb.nix that reference the loader by this explicit name.

Also applies to: 129-130, 150-150

nix/ext/tests/timescaledb.nix (2)

22-22: LGTM: Passthru attributes extended consistently.

The passthru additions maintain consistency with other extension test files in this PR.

Also applies to: 25-26


91-91: Test invocation correctly aligned with loader naming changes.

The expanded PostgresExtensionTest call with "public" and "timescaledb-loader" arguments (line 91) and the updated switch path to timescaledb-loader.so (line 97) correctly coordinate with the loader naming changes in nix/ext/timescaledb.nix.

Also applies to: 97-97

nix/packages/default.nix (2)

44-44: LGTM!

The path change relocates supabase-groonga to ../ext/pgroonga/groonga.nix, which logically groups the groonga dependency with its related pgroonga extension.


51-51: Code consolidation correctly implemented.

The pg-backrest package is now properly sourced from the main inputs.nixpkgs (revision c12c63cd6c5e, Jan 2025) with no separate nixpkgs-pgbackrest input remaining. This aligns with the PR's goal of reducing multiple nixpkgs inputs.

nix/ext/plv8/default.nix (4)

57-65: LGTM!

The patches structure is clear: the system v8 patch is always applied, and the GUC redefinition fix is conditionally applied for versions ≤ 3.1.10.


67-78: LGTM on build inputs structure.

The Darwin-specific native build inputs (clang, xcbuild) are appropriately conditionally included. The buildInputs correctly applies version-conditional v8 selection (though the consistency issue with other v8 references was flagged above).


82-93: LGTM on makeFlags structure.

The Darwin-specific SHLIB_LINK with -headerpad_max_install_names is correct for enabling install_name_tool modifications post-build. The non-Darwin path uses the simpler -lv8 linkage.


95-110: LGTM!

The Darwin-specific NIX_LDFLAGS correctly includes framework dependencies and the -headerpad_max_install_names flag to allow post-build install_name_tool modifications.

buildInputs =
(args.buildInputs or [ ])
++ lib.optionals stdenv.hostPlatform.isDarwin [ darwin.apple_sdk.frameworks.Security ];
buildInputs = (args.buildInputs or [ ]);
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Description: Search for Darwin-specific build requirements or issues in cargo-pgrx usage

# Check for Darwin-specific configurations or dependencies in cargo-pgrx overlays
rg -n -C3 -g '*.nix' 'cargo-pgrx.*[Dd]arwin|[Dd]arwin.*cargo-pgrx' nix/

# Look for Darwin platform checks in cargo-pgrx related files
rg -n 'isDarwin' nix/cargo-pgrx/

# Check if there are any Darwin-specific frameworksInputs or apple_sdk references removed
rg -n -C3 'apple_sdk|frameworksInputs' nix/cargo-pgrx/ nix/overlays/

Repository: supabase/postgres

Length of output: 250


🏁 Script executed:

#!/bin/bash
# Check git history for the buildInputs change
git log --oneline -p nix/cargo-pgrx/buildPgrxExtension.nix | head -200

# Also check the full context around line 151
sed -n '145,160p' nix/cargo-pgrx/buildPgrxExtension.nix

# And the context around line 179 to understand the Darwin handling
sed -n '175,185p' nix/cargo-pgrx/buildPgrxExtension.nix

Repository: supabase/postgres

Length of output: 9069


🏁 Script executed:

#!/bin/bash
# Check if this file existed in different locations before
git log --all --full-history -- nix/cargo-pgrx/buildPgrxExtension.nix | head -20

# Also check the parent commit to see what was there before
git show acf3ba4^:nix/cargo-pgrx/ 2>/dev/null | head -50

# Look for any other buildPgrxExtension files in history
find . -name "*buildPgrxExtension*" -o -name "*cargo-pgrx*" 2>/dev/null | grep -E "\.(nix|md)$"

Repository: supabase/postgres

Length of output: 707


🏁 Script executed:

#!/bin/bash
# Get the full diff for this specific file in the commit
git show acf3ba4 -- nix/cargo-pgrx/buildPgrxExtension.nix | grep -A20 -B5 "buildInputs"

# Also check the previous version of the file
git show acb688ad:nix/cargo-pgrx/buildPgrxExtension.nix 2>/dev/null | grep -A5 -B5 "buildInputs"

Repository: supabase/postgres

Length of output: 1238


🏁 Script executed:

#!/bin/bash
# Check the commit message and see if there's any explanation for removing Darwin frameworks
git show acf3ba4 | head -50

# Also check if there were any test results or notes about Darwin compatibility
git log --all --grep="Darwin\|darwin\|buildInputs" --oneline | head -20

# Look at the broader diff to understand the context of changes
git show acf3ba4 --stat

Repository: supabase/postgres

Length of output: 46696


🏁 Script executed:

#!/bin/bash
# Check if there are Darwin-specific tests or CI configurations
rg -n "isDarwin" nix/cargo-pgrx/ -A3 -B3

# Check if there are any test results or comments about Darwin builds
git log --all --oneline | grep -i "darwin\|macos\|apple"

# Look for CI configuration that might test Darwin builds
find .github/workflows -name "*.yml" -type f | xargs grep -l "darwin\|macos" 2>/dev/null | head -5

Repository: supabase/postgres

Length of output: 802


Clarify why darwin.apple_sdk.frameworks.Security was removed from buildInputs.

The change removes the Darwin-specific darwin.apple_sdk.frameworks.Security framework that was previously included in buildInputs (line 151), but the commit message doesn't explain this removal. While Darwin-specific RUSTFLAGS handling remains (line 179), the Security framework is typically required for macOS system integrations. Either restore the framework with justification or confirm that Darwin builds continue to work without it.

🤖 Prompt for AI Agents
In @nix/cargo-pgrx/buildPgrxExtension.nix around line 151, The Darwin-specific
Security framework was removed from buildInputs (buildInputs and
darwin.apple_sdk.frameworks.Security) without explanation; either restore it for
macOS builds or add a clear comment/commit note proving it's unnecessary and
that RUSTFLAGS-based handling (RUSTFLAGS / stdenv.isDarwin) still satisfies all
macOS link-time requirements. Fix by making buildInputs conditional on
stdenv.isDarwin to include darwin.apple_sdk.frameworks.Security when building on
Darwin, or if you intentionally removed it, add a comment and update the commit
message explaining why macOS linking remains correct without the framework and
reference the RUSTFLAGS changes that replace it.

Comment on lines +61 to +69
cmakeFlags = [
"-DPOSTGRESQL_VERSION=${postgresql.version}"
"-DCMAKE_POLICY_VERSION_MINIMUM=3.5"
]
++ lib.optionals (stdenv.isDarwin && lib.versionAtLeast postgresql.version "16") [
"-DCMAKE_MACOSX_RPATH=ON"
"-DCMAKE_SHARED_MODULE_SUFFIX=.dylib"
"-DCMAKE_SHARED_LIBRARY_SUFFIX=.dylib"
];
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🌐 Web query:

CMAKE_POLICY_VERSION_MINIMUM CMake purpose and usage

💡 Result:

Purpose

  • CMAKE_POLICY_VERSION_MINIMUM lets an external user or parent project request a minimum CMake "policy version" for a build tree without changing the project’s CMakeLists (i.e., it affects which CMPxxxx policies default to NEW). It was added in CMake 4.0 and is intended for external/parent use, not for a project to set its own policy version. [1]

When to use it

  • As a user trying to configure a project that hasn’t declared a recent policy version (e.g., force at least policy version 3.5).
  • As a parent project before add_subdirectory() to set the policy version used by a third‑party subproject without editing its files. [1]

How it interacts with cmake_minimum_required / cmake_policy

  • cmake_minimum_required(...) implicitly calls cmake_policy(VERSION ...). The effective policy version is the project’s cmake_minimum_required value (or explicit cmake_policy(VERSION)), unless CMAKE_POLICY_VERSION_MINIMUM (if higher) overrides it for the build tree. [2]

How to set it

  • On the command line / cache: -DCMAKE_POLICY_VERSION_MINIMUM=3.5
  • As an environment variable (to initialize new build trees) or set in the cache before configuring. Use it before add_subdirectory() if you want to affect a nested project. [1]

Notes / best practice

  • Projects should use cmake_minimum_required() or cmake_policy(VERSION) themselves; CMAKE_POLICY_VERSION_MINIMUM is intended as an external override (or for parent projects to influence third‑party code). For fine control of individual policies use CMAKE_POLICY_DEFAULT_CMP or explicit cmake_policy(SET ...). [1][2]

Sources
[1] CMake variable CMAKE_POLICY_VERSION_MINIMUM documentation.
[2] cmake_minimum_required / policy behavior documentation.


🏁 Script executed:

# Check the pgrouting default.nix file context
cd nix/ext/pgrouting && cat -n default.nix | head -80

Repository: supabase/postgres

Length of output: 3016


🏁 Script executed:

# Look for CMakeLists.txt in the repo that might document CMake requirements
find . -name CMakeLists.txt -type f | head -10

Repository: supabase/postgres

Length of output: 43


🏁 Script executed:

# Search for any comments or context around the CMAKE_POLICY flag
cd nix/ext/pgrouting && cat -n default.nix | sed -n '50,75p'

Repository: supabase/postgres

Length of output: 1304


🏁 Script executed:

# Check git history for this line to understand why it was added
cd nix/ext/pgrouting && git log --oneline -p default.nix | grep -A5 -B5 CMAKE_POLICY_VERSION_MINIMUM | head -30

Repository: supabase/postgres

Length of output: 560


🌐 Web query:

pgrouting CMake minimum required version cmake_minimum_required

💡 Result:

Use the CMake minimum required that matches the pgRouting version:

  • Current pgRouting (repo REQUIREMENTS / main) — cmake_minimum_required(VERSION 3.12). [1]
  • Older pgRouting 3.0–3.3 series — CMake >= 3.2. [2][3]
  • Very old pgRouting 2.0 series — CMake >= 2.8.8. [4]

Sources: pgRouting GitHub REQUIREMENTS (CMake >= 3.12). [1] Installation docs for pgRouting 3.0 / 3.3 (CMake >= 3.2). [2][3] Build guide for pgRouting 2.0 (CMake >= 2.8.8). [4]


Remove or document the CMAKE_POLICY_VERSION_MINIMUM flag.

The flag is set to 3.5, but pgrouting's actual cmake_minimum_required() is 3.2 (for versions 3.0–3.3) or 3.12 (current). While CMAKE_POLICY_VERSION_MINIMUM is the correct mechanism for external policy override in nixpkgs, the arbitrary value of 3.5 lacks documented justification and doesn't align with pgrouting's actual requirements. Either remove it (since pgrouting declares its own minimum), or add a comment explaining why 3.5 is necessary for this build.

🤖 Prompt for AI Agents
In @nix/ext/pgrouting/default.nix around lines 61-69, The cmakeFlags array
includes an unexplained "-DCMAKE_POLICY_VERSION_MINIMUM=3.5" which conflicts
with pgrouting's own cmake_minimum_required (3.2 for older releases or 3.12
currently); either remove that flag from cmakeFlags or replace it with a brief
comment explaining why 3.5 is required for this package build and which
pgrouting versions need it; locate and edit the cmakeFlags definition (the
cmakeFlags variable and the "-DCMAKE_POLICY_VERSION_MINIMUM=3.5" entry) or add
an adjacent comment clarifying the rationale and affected pgrouting versions.

Comment on lines +38 to +48
# plv8 3.1 requires an older version of v8 (we cannot use nodejs.libv8)
v8 = v8_oldstable;

# Build function for individual versions
build =
version: hash:
stdenv.mkDerivation (finalAttrs: {
inherit pname version;
#version = "3.1.10";

v8 = (if (builtins.compareVersions "3.1.10" version >= 0) then v8 else nodejs_20.libv8);
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check plv8 versions in versions.json
if [ -f "nix/ext/versions.json" ]; then
  cat "nix/ext/versions.json" | jq '.plv8 // empty'
else
  echo "versions.json not found, searching for it..."
  find . -name "versions.json" -type f
fi

Repository: supabase/postgres

Length of output: 430


Dead code and inconsistent v8 selection should be cleaned up for maintainability.

All plv8 versions in versions.json (3.1.4, 3.1.5, 3.1.10) are ≤ 3.1.10, so the conditional logic at line 48 always evaluates to v8_oldstable, making it redundant with the outer binding. This doesn't cause a functional bug for current versions, but the inconsistent structure is confusing:

  • Line 39: Outer v8 = v8_oldstable
  • Line 48: Unused derivation attribute v8 with conditional (always equals v8_oldstable for all current versions)
  • Line 76: Duplicate conditional in buildInputs (also always v8_oldstable)
  • All other references use outer v8

Simplify by removing the unused attribute and inline conditional since no versions > 3.1.10 are currently configured. If future versions may require version-specific v8 selection, refactor to use a consistent approach throughout rather than splitting the logic across multiple locations.

🤖 Prompt for AI Agents
In @nix/ext/plv8/default.nix around lines 38-48, The inner per-derivation v8
attribute and duplicate conditional in buildInputs are dead/redundant because
all versions <= 3.1.10, so remove the conditional v8 attribute inside the build
(the v8 = (if (builtins.compareVersions "3.1.10" version >= 0) then v8 else
nodejs_20.libv8) line and any duplicate conditional in buildInputs) and rely on
the outer v8 = v8_oldstable binding; if you want future-proofing instead of
removing logic, centralize version-specific selection into a single helper
(e.g., selectV8(version)) used by the build function rather than duplicating
conditionals in stdenv.mkDerivation and buildInputs.

Comment on lines +790 to +792
assert False, (
"PostgREST logs contain 'session is not read-only' errors even though PostgreSQL is configured for read-only mode"
)
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Replace assert False with raise AssertionError().

Using assert False is problematic because Python's -O optimization flag removes all assert statements, which would silently skip this error condition in production or optimized environments.

🔎 Recommended fix
-            assert False, (
-                "PostgREST logs contain 'session is not read-only' errors even though PostgreSQL is configured for read-only mode"
-            )
+            raise AssertionError(
+                "PostgREST logs contain 'session is not read-only' errors even though PostgreSQL is configured for read-only mode"
+            )
🧰 Tools
🪛 Ruff (0.14.10)

790-790: Do not assert False (python -O removes these calls), raise AssertionError()

Replace assert False

(B011)

🤖 Prompt for AI Agents
In @testinfra/test_ami_nix.py around lines 790-792, The test currently uses
"assert False, (\"PostgREST logs contain 'session is not read-only' errors even
though PostgreSQL is configured for read-only mode\")" which can be stripped out
with Python's -O optimization; replace that assert with an explicit exception by
raising AssertionError with the same message (i.e., use raise
AssertionError("PostgREST logs contain 'session is not read-only' errors even
though PostgreSQL is configured for read-only mode")) so the failure cannot be
bypassed by optimizations and still provides the same diagnostic text.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

Fix all issues with AI Agents 🤖
In @nix/cargo-pgrx/default.nix:
- Line 36: The build config is inconsistent: openssl is only added to
buildInputs/nativeBuildInputs via lib.optionals stdenv.hostPlatform.isLinux, but
the OPENSSL_* environment variables are set unconditionally; update the file so
the OPENSSL_* environment variables are wrapped with the same
stdenv.hostPlatform.isLinux guard (or alternatively make openssl unconditional
in buildInputs/nativeBuildInputs) to ensure Darwin builds don’t reference
missing openssl paths; locate the OPENSSL_* settings near the
buildInputs/nativeBuildInputs change and apply the same conditional (or remove
the conditional from buildInputs), keeping compatibility with the existing
buildPgrxExtension.nix Darwin support.

In @nix/ext/pgroonga/groonga.nix:
- Line 48: The buildInputs entry is using lib.optional with a list which creates
a nested list when zlibSupport is true; replace the lib.optional call with
lib.optionals (plural) so the zlib entry is flattened like the other
conditionals (match usage on surrounding lines), e.g. change the lib.optional
zlibSupport [ zlib ] expression to use lib.optionals so buildInputs remains a
flat list.

In @nix/ext/tests/lib.py:
- Around line 169-171: The assertion error messages reference
self.extension_name but the checks validate filenames built from self.lib_name;
update the messages to mention self.lib_name for clarity. Locate the three
assertions that test ext_version.endswith(f"{self.lib_name}-{last_version}.so")
(and the similar checks at the other two places) and change the f-string in
their exception text to reference self.lib_name (e.g., "Expected {self.lib_name}
version {last_version}...") so the logged name matches the filename being
validated.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
nix/ext/pgroonga/groonga.nix (1)

1-106: This file should have been removed.

Per the previous review comment, this file was moved to nix/ext/pgroonga/default.nix and should no longer exist. The presence of modifications here suggests a rebase issue. Please verify:

  1. Whether this file should be deleted from the PR
  2. Whether the buildInputs changes (lines 39-52) need to be applied to nix/ext/pgroonga/default.nix instead

Based on past review comments indicating this file should be removed.

nix/ext/tests/orioledb.nix (1)

14-14: Consider using majorVersion variable instead of hardcoding "17".

Lines 14 and 18 hardcode "psql_orioledb-17" in the extension path, but line 8 already extracts majorVersion from postgresql.version. This creates a mismatch where the postgresqlWithExtension function appears generic but only works correctly with PostgreSQL version 17.

If orioledb supports multiple PostgreSQL major versions, consider using the majorVersion variable to make the function more maintainable and version-agnostic. This would also eliminate the code duplication between lines 14 and 18.

🔎 Suggested refactor
+    extensionPkg = self.legacyPackages.${pkgs.system}."psql_orioledb-${majorVersion}".exts.orioledb;
     pkg = pkgs.buildEnv {
       name = "postgresql-${majorVersion}-${pname}";
       paths = [
         postgresql
         postgresql.lib
-        (self.legacyPackages.${pkgs.system}."psql_orioledb-17".exts.orioledb)
+        extensionPkg
       ];
       passthru = {
         inherit (postgresql) version psqlSchema;
-        installedExtensions = [ (self.legacyPackages.${pkgs.system}."psql_orioledb-17".exts.orioledb) ];
+        installedExtensions = [ extensionPkg ];

Also applies to: 18-18

♻️ Duplicate comments (4)
testinfra/test_ami_nix.py (1)

790-792: Replace assert False with raise AssertionError().

This issue was flagged in a previous review but remains unresolved. Using assert False is problematic because Python's -O optimization flag removes all assert statements, which would silently skip this error condition.

🔎 Recommended fix
-            assert False, (
-                "PostgREST logs contain 'session is not read-only' errors even though PostgreSQL is configured for read-only mode"
-            )
+            raise AssertionError(
+                "PostgREST logs contain 'session is not read-only' errors even though PostgreSQL is configured for read-only mode"
+            )
nix/ext/pg_jsonschema/default.nix (1)

83-93: Verify version "0.3.3" should be excluded from previousVersions.

The doCheck conditional is well-documented and addresses the sandbox limitation. However, version "0.3.3" appears in the doCheck disabled list but is absent from the previousVersions array (lines 48-57). Versions "0.2.0" and "0.3.1" appear in both lists.

If "0.3.3" is not the latest version in ../versions.json, it should be added to previousVersions to ensure upgrade scripts are generated correctly.

#!/bin/bash
# Verify if 0.3.3 should be in previousVersions by checking versions.json

echo "Checking pg_jsonschema versions defined in versions.json:"
jq -r '.pg_jsonschema | keys[]' nix/ext/versions.json | sort -V

echo -e "\nVersions with tests disabled:"
echo "0.2.0, 0.3.1, 0.3.3"

echo -e "\nVersions in previousVersions array:"
echo "0.3.1, 0.3.0, 0.2.0, 0.1.4, 0.1.3, 0.1.2, 0.1.1, 0.1.0"

echo -e "\nIf 0.3.3 is NOT the latest version, it should be added to previousVersions."
nix/ext/plv8/default.nix (1)

38-48: The dead code and inconsistent v8 selection flagged in the previous review remains unaddressed.

As noted in the prior review, line 48 defines a per-derivation v8 attribute that is never referenced (all usage points to the outer v8 from line 39), and line 76 duplicates the conditional logic in buildInputs while the rest of the code uses the outer binding. This creates maintenance confusion and potential bugs if versions > 3.1.10 are added.

nix/ext/pgrouting/default.nix (1)

61-69: CMAKE_POLICY_VERSION_MINIMUM still lacks justification.

As flagged in a previous review, the -DCMAKE_POLICY_VERSION_MINIMUM=3.5 flag remains undocumented. Either remove it (pgrouting declares its own minimum) or add a comment explaining why 3.5 is required for this build.

🧹 Nitpick comments (4)
nix/ext/plv8/default.nix (1)

136-158: Simplify installPhase by removing redundant conditional.

The if/else branching on PostgreSQL version 15 executes identical install_name_tool and patchelf commands in both branches. Since the logic is the same regardless of PostgreSQL version, the conditional can be removed.

🔎 Proposed simplification
-        if [ "${lib.versions.major postgresql.version}" = "15" ]; then
-          ${lib.optionalString stdenv.isDarwin ''
-            install_name_tool -add_rpath "${v8}/lib" $out/lib/$LIB_NAME
-            install_name_tool -add_rpath "${postgresql}/lib" $out/lib/$LIB_NAME
-            install_name_tool -add_rpath "${libcxx}/lib" $out/lib/$LIB_NAME
-            install_name_tool -change @rpath/libv8_monolith.dylib ${v8}/lib/libv8_monolith.dylib $out/lib/$LIB_NAME
-          ''}
-
-          ${lib.optionalString (!stdenv.isDarwin) ''
-            ${patchelf}/bin/patchelf --set-rpath "${v8}/lib:${postgresql}/lib:${libcxx}/lib" $out/lib/$LIB_NAME
-          ''}
-        else
-          ${lib.optionalString stdenv.isDarwin ''
-            install_name_tool -add_rpath "${v8}/lib" $out/lib/$LIB_NAME
-            install_name_tool -add_rpath "${postgresql}/lib" $out/lib/$LIB_NAME
-            install_name_tool -add_rpath "${libcxx}/lib" $out/lib/$LIB_NAME
-            install_name_tool -change @rpath/libv8_monolith.dylib ${v8}/lib/libv8_monolith.dylib $out/lib/$LIB_NAME
-          ''}
-
-          ${lib.optionalString (!stdenv.isDarwin) ''
-            ${patchelf}/bin/patchelf --set-rpath "${v8}/lib:${postgresql}/lib:${libcxx}/lib" $out/lib/$LIB_NAME
-          ''}
-        fi
+        ${lib.optionalString stdenv.isDarwin ''
+          install_name_tool -add_rpath "${v8}/lib" $out/lib/$LIB_NAME
+          install_name_tool -add_rpath "${postgresql}/lib" $out/lib/$LIB_NAME
+          install_name_tool -add_rpath "${libcxx}/lib" $out/lib/$LIB_NAME
+          install_name_tool -change @rpath/libv8_monolith.dylib ${v8}/lib/libv8_monolith.dylib $out/lib/$LIB_NAME
+        ''}
+
+        ${lib.optionalString (!stdenv.isDarwin) ''
+          ${patchelf}/bin/patchelf --set-rpath "${v8}/lib:${postgresql}/lib:${libcxx}/lib" $out/lib/$LIB_NAME
+        ''}
nix/ext/pg_net.nix (1)

13-15: Consider removing the curl alias indirection.

The curl = curl_8_6 alias adds an extra level of indirection without clear benefit. Consider using curl_8_6 directly in buildInputs for simplicity.

🔎 Proposed simplification

Remove the alias and use curl_8_6 directly:

-let
-  curl = curl_8_6;
-in
 let
   pname = "pg_net";
   build =

Then update line 24 to reference curl_8_6 directly:

       buildInputs = [
-        curl
+        curl_8_6
         postgresql
       ]
flake.nix (1)

22-24: Consider enhancing the nixpkgs-oldstable comment.

The comment explains why the pin exists, but per a previous review suggestion, consider adding the date or nixpkgs version for this commit, and guidance on when/whether to update it (e.g., "update only if needed for specific extension compatibility").

nix/postgresql/generic.nix (1)

345-345: Clarify the empty teams list.

The teams = [ ] field has been added to metadata. If this is a placeholder for future team assignment, consider adding a comment. Otherwise, if no teams are responsible, this can remain empty.

📜 Review details

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between acf3ba4 and 42b0c29.

⛔ Files ignored due to path filters (1)
  • flake.lock is excluded by !**/*.lock
📒 Files selected for processing (54)
  • ansible/vars.yml
  • flake.nix
  • migrations/schema-15.sql
  • migrations/schema-17.sql
  • migrations/schema-orioledb-17.sql
  • nix/cargo-pgrx/buildPgrxExtension.nix
  • nix/cargo-pgrx/default.nix
  • nix/cargo-pgrx/versions.json
  • nix/checks.nix
  • nix/ext/gdal.nix
  • nix/ext/pg_jsonschema/default.nix
  • nix/ext/pg_net.nix
  • nix/ext/pgaudit.nix
  • nix/ext/pgroonga/default.nix
  • nix/ext/pgroonga/groonga.nix
  • nix/ext/pgrouting/default.nix
  • nix/ext/pgrouting/pgrouting-3.4.1-pg17.patch
  • nix/ext/plpgsql-check.nix
  • nix/ext/plv8/default.nix
  • nix/ext/postgis.nix
  • nix/ext/tests/default.nix
  • nix/ext/tests/http.nix
  • nix/ext/tests/lib.py
  • nix/ext/tests/orioledb.nix
  • nix/ext/tests/pg_plan_filter.nix
  • nix/ext/tests/pg_repack.nix
  • nix/ext/tests/pg_safeupdate.nix
  • nix/ext/tests/pgjwt.nix
  • nix/ext/tests/pgmq.nix
  • nix/ext/tests/pgroonga.nix
  • nix/ext/tests/pgrouting.nix
  • nix/ext/tests/pgsodium.nix
  • nix/ext/tests/plpgsql_check.nix
  • nix/ext/tests/plv8.nix
  • nix/ext/tests/postgis.nix
  • nix/ext/tests/timescaledb.nix
  • nix/ext/tests/vault.nix
  • nix/ext/timescaledb.nix
  • nix/ext/vault.nix
  • nix/ext/wal2json.nix
  • nix/ext/wrappers/default.nix
  • nix/nixpkgs.nix
  • nix/overlays/default.nix
  • nix/packages/default.nix
  • nix/packages/groonga/do-not-use-vendored-libraries.patch
  • nix/packages/groonga/fix-cmake-install-path.patch
  • nix/packages/lib.nix
  • nix/packages/packer.nix
  • nix/packages/postgres.nix
  • nix/packages/sfcgal.nix
  • nix/packages/supascan.nix
  • nix/packages/sync-exts-versions.nix
  • nix/postgresql/generic.nix
  • testinfra/test_ami_nix.py
💤 Files with no reviewable changes (4)
  • nix/packages/sfcgal.nix
  • nix/overlays/default.nix
  • nix/packages/groonga/fix-cmake-install-path.patch
  • nix/packages/groonga/do-not-use-vendored-libraries.patch
🚧 Files skipped from review as they are similar to previous changes (28)
  • nix/ext/pgroonga/default.nix
  • migrations/schema-orioledb-17.sql
  • nix/ext/tests/default.nix
  • nix/ext/tests/pgsodium.nix
  • nix/cargo-pgrx/buildPgrxExtension.nix
  • nix/packages/packer.nix
  • ansible/vars.yml
  • nix/ext/tests/http.nix
  • nix/ext/wrappers/default.nix
  • nix/nixpkgs.nix
  • migrations/schema-17.sql
  • nix/ext/tests/plv8.nix
  • nix/ext/tests/pg_repack.nix
  • nix/ext/tests/plpgsql_check.nix
  • nix/ext/tests/timescaledb.nix
  • nix/ext/tests/postgis.nix
  • nix/packages/default.nix
  • nix/ext/vault.nix
  • nix/ext/tests/pgjwt.nix
  • nix/ext/tests/pgmq.nix
  • nix/ext/tests/pgroonga.nix
  • nix/ext/pgaudit.nix
  • nix/ext/plpgsql-check.nix
  • nix/ext/tests/pg_plan_filter.nix
  • nix/ext/tests/vault.nix
  • nix/ext/tests/pgrouting.nix
  • nix/packages/lib.nix
  • nix/ext/gdal.nix
🧰 Additional context used
🪛 Ruff (0.14.10)
testinfra/test_ami_nix.py

790-790: Do not assert False (python -O removes these calls), raise AssertionError()

Replace assert False

(B011)

⏰ 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)
  • GitHub Check: nix-eval / eval
🔇 Additional comments (42)
migrations/schema-15.sql (1)

640-640: LGTM—change aligns with updated dbmate output.

The removal of the explicit length constraint (varchar(128) → varchar) is safe and non-breaking. As noted in the previous discussion, this change results from updating dbmate from 2.14.0 to 2.28.0, and the team has already decided to accept the newer tool's schema output format.

testinfra/test_ami_nix.py (1)

435-437: LGTM: Formatting changes to assert statements.

The reformatting of these multi-line assert statements to single-line format is acceptable. These are all conditional asserts with proper error messages.

Also applies to: 560-562, 592-594, 627-629

nix/checks.nix (1)

8-8: LGTM: Parameter reordering.

Reordering the lib parameter to appear after pkgs in the perSystem function signature has no functional impact. This is a style adjustment that may align with project conventions.

nix/ext/tests/pg_safeupdate.nix (1)

22-26: LGTM! Consistent interface standardization.

The new passthru attributes (installedExtensions, withJIT, withoutJIT) provide a uniform interface for extension test modules, matching the pattern applied across other test files in this PR.

Note: Both withJIT and withoutJIT currently point to the same pkg, which is acceptable if pg_safeupdate doesn't require JIT-specific build variants. This provides interface consistency even when functional variants aren't needed.

nix/ext/tests/orioledb.nix (1)

21-22: Verify that withJIT and withoutJIT pointing to the same package is intentional.

Both withJIT and withoutJIT attributes reference the same pkg variable. This appears to mirror the pattern from other extension tests according to the AI summary, but it's worth confirming this is the intended behavior for the test environment rather than an oversight.

nix/ext/tests/lib.py (3)

8-8: LGTM!

The addition of Optional to the imports is necessary for the new lib_name parameter type hint.


24-24: LGTM!

The new lib_name parameter is well-designed with a sensible fallback to extension_name. The implementation correctly handles the case where lib_name is not provided.

Note: The fallback self.lib_name = lib_name or extension_name will use extension_name if lib_name is None, empty string, or any other falsy value. This is likely the intended behavior.

Also applies to: 34-34, 42-42


99-101: LGTM!

The assertion reformatting maintains the same logic while being more compact.

nix/ext/pg_jsonschema/default.nix (2)

45-45: LGTM! Darwin removal aligns with PR cleanup.

The removal of darwin from buildInputs is appropriate—Darwin-specific configuration is already handled through the env attribute (lines 60-69). This aligns with the broader nixpkgs update that removes Darwin-specific platform augmentation from buildInputs across multiple derivations.


48-57: LGTM! previousVersions expansion is well-documented.

The expanded previousVersions list is properly ordered and will correctly generate extension update scripts through the preBuild logic (lines 95-98).

nix/packages/supascan.nix (2)

41-43: Good approach for cross-platform compatibility.

Disabling CGO is a reasonable trade-off to eliminate Darwin framework dependencies while maintaining cross-platform builds. For a system scanner tool, this should work well.

One consideration: with CGO_ENABLED=0, Go uses its pure-Go DNS resolver instead of the system resolver. This typically works fine but may behave slightly differently in edge cases (e.g., certain nsswitch.conf configurations on Linux). Given supascan's use case as a validation tool, this shouldn't be an issue.


1-5: LGTM! Clean simplification of Go 1.24 sourcing.

The removal of the inputs parameter and sourcing go_1_24 directly from pkgs simplifies the derivation by eliminating the dependency on a separate flake input. The updated nixpkgs (nixos-unstable) provides go_1_24, and the buildGoModule124 override is correctly applied to the supascan package at line 31.

nix/ext/pg_net.nix (2)

10-10: Good alignment with past feedback.

The addition of curl_8_6 as a dedicated input parameter addresses the previous review comment about declaring a specific curl version in the overlay rather than passing the full attributeset.


52-59: No actionable issues found. The version check using exact equality (version == "0.19.5") is correct: 0.19.5 is the only supported version ≥ 0.19.1 in the codebase. The curl_8_6 dependency is applied uniformly across all versions with no obvious incompatibilities evident in the code.

nix/cargo-pgrx/versions.json (1)

1-119: LGTM - Hash updates verified by successful e2e testing.

All cargoHash updates appear correct and have been validated by the comprehensive e2e testing reported in PR comments (engines tests passed 3/3 across PG versions 15, 17, and 17-oriole). The hash updates are mechanical and result from the nixpkgs upgrade and related toolchain changes.

nix/cargo-pgrx/default.nix (1)

73-83: LGTM - cargoHash values are consistent with versions.json.

The updated cargoHash values for cargo-pgrx versions 0.12.6, 0.12.9, and 0.14.3 match the corresponding entries in nix/cargo-pgrx/versions.json and have been validated by successful e2e testing.

nix/packages/sync-exts-versions.nix (1)

14-15: Version bump to Nix 2.29 is safe.

The script uses nix-prefetch-url for hash extraction (via cut), nix hash to-sri for conversion, and nix eval without JSON flags. The identified breaking change (JSON prettification in Nix 2.29) does not apply here since the script avoids --json flags and parses only yq output with jq. The fundamental nix-prefetch-url output format remains stable across the versions.

nix/ext/wal2json.nix (1)

13-13: LGTM: Unused parameter clearly marked.

The underscore prefix correctly signals that the _rev parameter is unused (line 20 constructs the revision from version instead). This improves code clarity.

nix/ext/pgrouting/default.nix (2)

52-54: LGTM: Patch applied conditionally.

The patch is correctly applied only to pgrouting 3.4.1 running on PostgreSQL 17+, matching the scope of the compatibility fix.


15-15: Path change to versions.json is correct.

The file exists at nix/ext/versions.json and contains the pgrouting version data as expected.

flake.nix (2)

25-25: LGTM: Using stable channel tarball URL.

The nixpkgs URL now points to the nixos-unstable channel tarball, which provides stable snapshot URLs appropriate for flake inputs.


10-21: Follow declarations correctly prevent duplicate dependencies.

The new inputs properly declare follows to avoid duplicate nixpkgs instances: git-hooks, nix-editor, nix-eval-jobs, and nix2container all follow the primary nixpkgs. Additionally, nix-eval-jobs follows treefmt-nix and flake-parts, ensuring consistent input versions throughout the flake without duplication.

nix/packages/postgres.nix (1)

28-28: LGTM: Import path updated for directory structure.

The pgrouting import changed from a file reference (../ext/pgrouting.nix) to a directory reference (../ext/pgrouting), aligning with the restructuring of pgrouting as a directory with default.nix. This is consistent with the path changes observed in nix/ext/pgrouting/default.nix.

nix/ext/pgrouting/pgrouting-3.4.1-pg17.patch (1)

1-39: Patch correctly addresses PostgreSQL 17 SPI_getvalue API change.

The patch accurately reflects that SPI_getvalue returns char* directly in PostgreSQL 17, not Datum. Removing the DatumGetCString wrapper is correct—the function expects a Datum but receives a char*, causing a type mismatch. The changes in both get_check_data.c and trsp.c fix this compilation issue.

nix/ext/postgis.nix (2)

82-83: Conditional Darwin dependency is appropriate.

The conditional inclusion of libiconv for Darwin and the formatting adjustment are correct.


19-19: This concern is not applicable. The sfcgal parameter is automatically resolved by the callPackageWith mechanism in Nix. Since extCallPackage is defined with pkgs (which includes nixpkgs' sfcgal package), all calls to postgis.nix will receive the sfcgal parameter automatically without requiring explicit parameter passing at each call site. The migration from a local sfcgal.nix to using nixpkgs' sfcgal is handled correctly.

nix/ext/timescaledb.nix (4)

42-43: LGTM: Formatting adjustment.

The CMake flags formatting change improves readability without altering behavior.


129-130: Document the loader symlink purpose.

The comment on line 129 indicates this symlink is "used by background worker tests," which is helpful context.


150-150: LGTM: Consistent libName exposure.

The libName passthru field correctly exposes the loader library name for external use.


111-111: Symlink consistency verified.

The loader symlink created at line 111 ($EXT_NAME-loader${postgresql.dlSuffix}) is correctly aligned with the libName = "timescaledb-loader" passthru (line 150) and the postBuild symlink (line 130, which includes a comment confirming it's for background worker tests). Tests in nix/ext/tests/timescaledb.nix confirm that both the test harness (line 91) and background worker checks (line 97) properly reference the timescaledb-loader library.

nix/postgresql/generic.nix (12)

15-15: LGTM: ICU version pinning is well-documented.

Pinning ICU to version 75 to maintain collation version 153.120 is appropriate for preventing collation mismatch warnings during nixpkgs upgrades. The comment clearly explains the rationale.

Also applies to: 123-125


109-110: LGTM: Hardening flag comment update.

The comment correctly notes that PIE is now enabled by default in compilers, and the hardeningEnable line is appropriately commented out rather than removed.


118-144: LGTM: buildInputs restructuring.

The buildInputs have been reformatted for clarity with proper conditional logic for:

  • Legacy libxcrypt support (< v13)
  • JIT support with LLVM
  • LZ4 (v14+) and ZSTD (v15+) compression
  • Platform-specific dependencies

The logic is correct and well-organized.


146-154: LGTM: nativeBuildInputs restructuring.

The conditional inclusion of JIT-specific native build inputs (LLVM dev, nukeReferences, patchelf) is properly structured.


167-184: LGTM: configureFlags restructuring.

The configure flags have been reformatted consistently with proper conditional logic. All necessary flags are present.


185-207: LGTM: Patches restructuring with proper musl handling.

The patches section correctly:

  • Applies version-specific patches for v16+ vs earlier
  • Uses replaceVars for locale binary path substitution
  • Handles musl patches using fetchurl (per comment about nixpkgs issue #240141)
  • Applies Linux-specific socket directory patches

211-220: LGTM: postPatch with JIT support.

The postPatch logic correctly substitutes paths for JIT components when JIT support is enabled.


222-269: LGTM: Comprehensive postInstall with JIT handling.

The extensive postInstall section properly:

  • Moves pgxs and libraries to appropriate outputs
  • Removes static libraries when dynamic versions exist
  • Handles JIT-specific bitcode and library relocation
  • Removes retained dependencies on clang-wrapper and LLVM dev outputs
  • Uses nuke-refs to eliminate unnecessary references
  • Conditionally patches rpath on non-Darwin platforms

This is thorough cleanup necessary for minimal closure sizes.


323-337: LGTM: Tests section restructuring.

The tests are properly structured with:

  • Standard tests (wal-receiver, pkg-config)
  • Conditional JIT-specific test

24-24: replaceVars usage for locale patch is correct.

The replaceVars function is properly imported (line 24) and correctly applied to substitute the locale binary path in the patch. The template variable @locale@ in the patch matches the locale key provided to replaceVars (lines 197-199), with the substitution correctly resolving to darwin.adv_cmds on macOS and the standard C library on Linux.


69-69: Pattern match logic is correct and safe.

The [0-9][0-9]_.* pattern correctly identifies OrioleDB versions. OrioleDB versions are versioned as two-digit major version followed by underscore and patchset number (e.g., 17_15), while standard PostgreSQL uses semantic versioning with dots (e.g., 17.6). Standard PostgreSQL versions cannot match this pattern and no false positives are possible.


159-159: No action required — __structuredAttrs is correctly implemented.

The code already follows best practices for __structuredAttrs = true:

  • Environment variables are correctly exported via env.NIX_CFLAGS_COMPILE (line 165), which is the documented pattern when using structured attributes with mkDerivation
  • PostgreSQL's standard autoconf/make build system is fully compatible with structured attributes
  • nixpkgs-unstable has supported this feature since Nix 2.16+
  • Extensions and downstream consumers are unaffected—structured attributes are transparent to users of the derivation's outputs and passthru attributes

Likely an incorrect or invalid review comment.

buildInputs =
lib.optionals stdenv.hostPlatform.isLinux [ openssl ]
++ lib.optionals stdenv.hostPlatform.isDarwin [ darwin.apple_sdk.frameworks.Security ];
buildInputs = lib.optionals stdenv.hostPlatform.isLinux [ openssl ];
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# First, check for Darwin-specific configurations in cargo-pgrx files
rg -n "darwin|Darwin|hostPlatform.isDarwin" nix/cargo-pgrx/ -i

Repository: supabase/postgres

Length of output: 250


🏁 Script executed:

# Also check the full default.nix to understand the buildInputs context
cat -n nix/cargo-pgrx/default.nix | head -60

Repository: supabase/postgres

Length of output: 2448


🏁 Script executed:

# Look for git history to see what was removed (if available)
git log --oneline -10 nix/cargo-pgrx/default.nix 2>/dev/null || echo "Git history not available"

Repository: supabase/postgres

Length of output: 129


🏁 Script executed:

# Check if there are other buildInputs configurations in the broader nix directory
rg -n "buildInputs.*lib\.optionals" nix/ -A 2 -B 2

Repository: supabase/postgres

Length of output: 433


🏁 Script executed:

# Check how OPENSSL variables are used - see if they're critical for the build
rg -n "OPENSSL_DIR|OPENSSL_INCLUDE|OPENSSL_LIB" nix/cargo-pgrx/ -A 2 -B 2

Repository: supabase/postgres

Length of output: 560


🏁 Script executed:

# Check if there's a separate Darwin configuration or if this is already broken
rg -n "openssl" nix/cargo-pgrx/default.nix -B 5 -A 5

Repository: supabase/postgres

Length of output: 885


Address inconsistent Darwin build configuration: conditional buildInputs with unconditional OPENSSL environment variables.

The buildInputs and nativeBuildInputs are now Linux-only (via lib.optionals stdenv.hostPlatform.isLinux), but the OPENSSL_* environment variables on lines 38-41 are set unconditionally. This inconsistency can cause Darwin builds to fail since buildInputs will be empty while still trying to reference openssl paths. Darwin support is still intended elsewhere in the codebase (buildPgrxExtension.nix), so either the OPENSSL environment variables should also be conditional, or openssl should remain in buildInputs for all platforms.

🤖 Prompt for AI Agents
In @nix/cargo-pgrx/default.nix around line 36, The build config is inconsistent:
openssl is only added to buildInputs/nativeBuildInputs via lib.optionals
stdenv.hostPlatform.isLinux, but the OPENSSL_* environment variables are set
unconditionally; update the file so the OPENSSL_* environment variables are
wrapped with the same stdenv.hostPlatform.isLinux guard (or alternatively make
openssl unconditional in buildInputs/nativeBuildInputs) to ensure Darwin builds
don’t reference missing openssl paths; locate the OPENSSL_* settings near the
buildInputs/nativeBuildInputs change and apply the same conditional (or remove
the conditional from buildInputs), keeping compatibility with the existing
buildPgrxExtension.nix Darwin support.

msgpack-c
]
++ lib.optionals lz4Support [ lz4 ]
++ lib.optional zlibSupport [ zlib ]
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Critical: lib.optional creates nested list.

Using lib.optional with a list argument will create a nested list [[ zlib ]] when zlibSupport is true, breaking the buildInputs array. Use lib.optionals (plural) for consistency with lines 47 and 49.

🔎 Proposed fix
-  ++ lib.optional zlibSupport [ zlib ]
+  ++ lib.optionals zlibSupport [ zlib ]
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
++ lib.optional zlibSupport [ zlib ]
+ lib.optionals zlibSupport [ zlib ]
🤖 Prompt for AI Agents
In @nix/ext/pgroonga/groonga.nix around line 48, The buildInputs entry is using
lib.optional with a list which creates a nested list when zlibSupport is true;
replace the lib.optional call with lib.optionals (plural) so the zlib entry is
flattened like the other conditionals (match usage on surrounding lines), e.g.
change the lib.optional zlibSupport [ zlib ] expression to use lib.optionals so
buildInputs remains a flat list.

Comment on lines +169 to +171
assert ext_version.endswith(f"{self.lib_name}-{last_version}.so"), (
f"Expected {self.extension_name} version {last_version}, but found {ext_version}"
)
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Consider updating error messages to reference lib_name for clarity.

The assertions now check for files named {lib_name}-{version}.so, but the error messages reference self.extension_name. When lib_name differs from extension_name, this could make debugging confusing.

For example, if extension_name="pg_cron" and lib_name="pgcron", the error message will say "Expected pg_cron version 1.0" but the actual file being checked is pgcron-1.0.so.

🔎 Suggested fix for clearer error messages
-        assert ext_version.endswith(f"{self.lib_name}-{last_version}.so"), (
-            f"Expected {self.extension_name} version {last_version}, but found {ext_version}"
-        )
+        assert ext_version.endswith(f"{self.lib_name}-{last_version}.so"), (
+            f"Expected {self.lib_name}-{last_version}.so, but found {ext_version}"
+        )

Apply similar changes to all three assertions (lines 169-171, 179-181, 187-189).

Also applies to: 179-181, 187-189

🤖 Prompt for AI Agents
In @nix/ext/tests/lib.py around lines 169-171, The assertion error messages
reference self.extension_name but the checks validate filenames built from
self.lib_name; update the messages to mention self.lib_name for clarity. Locate
the three assertions that test
ext_version.endswith(f"{self.lib_name}-{last_version}.so") (and the similar
checks at the other two places) and change the f-string in their exception text
to reference self.lib_name (e.g., "Expected {self.lib_name} version
{last_version}...") so the logged name matches the filename being validated.

nix 2.29 doesn't exist in the updated nixpkgs anymore
Replace deprecated system attribute accesses throughout the codebase with
stdenv.hostPlatform.system to resolve Nix deprecation warnings.

Changes:
- Replace inherit (stdenv) system with system = stdenv.hostPlatform.system
- Replace pkgs.system with pkgs.stdenv.hostPlatform.system (55 occurrences)
- Replace system function parameters with stdenv to avoid auto-injection
- Replace final.system in overlay with final.stdenv.hostPlatform.system

Affected files:
- nix/postgresql/generic.nix
- nix/packages/default.nix
- nix/ext/tests/*.nix (16 files)
- nix/packages/lib.nix
- nix/packages/dbmate-tool.nix
- nix/packages/show-commands.nix
- nix/cargo-pgrx/mkPgrxExtension.nix
- nix/overlays/default.nix
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🤖 Fix all issues with AI Agents
In @nix/postgresql/generic.nix:
- Around line 197-199: The patch uses bare stdenv in the replaceVars call;
change both occurrences to the JIT-aware stdenv' used elsewhere: replace
stdenv.isDarwin with stdenv'.isDarwin and stdenv.cc.libc with stdenv'.cc.libc
inside the replaceVars invocation (the expression building locale in the
locale-binary-path.patch replacement) so the platform checks and libc reference
are consistent with the rest of the file.
- Line 15: The pinned dependency icu75 in the Nix expression is flagged for
CVE-2025-5222; locate the icu75 symbol in this file and verify whether the Nix
package includes the upstream security fix or local backport; if it does not,
update the pin to a patched upstream version (e.g., a fixed minor release) or
apply the vendor security patch as a patch file in the derivation (add the patch
and include it in the src/patches list), then rebuild and run tests to confirm
collations still match and the CVE is mitigated.
🧹 Nitpick comments (2)
nix/ext/tests/pgrouting.nix (1)

49-49: LGTM: All PostgreSQL package references correctly migrated.

All references to PostgreSQL packages throughout the test configuration (pg_regress, services.postgresql.package, specialisation configurations, and migration scripts) now correctly use self.packages.${pkgs.stdenv.hostPlatform.system}.postgresql_* pattern. The migration is thorough and consistent.

Optional: Consider extracting the platform system pattern

The pattern self.packages.${pkgs.stdenv.hostPlatform.system} is repeated many times. For improved readability, you could optionally extract it into a let-binding:

let
  platformPackages = self.packages.${pkgs.stdenv.hostPlatform.system};
in
# Then use: platformPackages.postgresql_15, platformPackages.postgresql_17, etc.

This is purely optional and the current implementation is acceptable for a test file.

Also applies to: 76-76, 81-83, 97-102, 126-128, 156-158

nix/postgresql/generic.nix (1)

345-345: Consider removing the empty teams list if not required.

The empty teams = [ ] attribute doesn't provide value. If this isn't required by nixpkgs conventions, consider removing it to reduce clutter.

🔎 Proposed change
-        teams = [ ];
📜 Review details

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8115b54 and 8ee0403.

📒 Files selected for processing (23)
  • nix/cargo-pgrx/mkPgrxExtension.nix
  • nix/ext/tests/default.nix
  • nix/ext/tests/http.nix
  • nix/ext/tests/orioledb.nix
  • nix/ext/tests/pg_plan_filter.nix
  • nix/ext/tests/pg_repack.nix
  • nix/ext/tests/pg_safeupdate.nix
  • nix/ext/tests/pgjwt.nix
  • nix/ext/tests/pgmq.nix
  • nix/ext/tests/pgroonga.nix
  • nix/ext/tests/pgrouting.nix
  • nix/ext/tests/pgsodium.nix
  • nix/ext/tests/plpgsql_check.nix
  • nix/ext/tests/plv8.nix
  • nix/ext/tests/postgis.nix
  • nix/ext/tests/timescaledb.nix
  • nix/ext/tests/vault.nix
  • nix/overlays/default.nix
  • nix/packages/dbmate-tool.nix
  • nix/packages/default.nix
  • nix/packages/lib.nix
  • nix/packages/show-commands.nix
  • nix/postgresql/generic.nix
🚧 Files skipped from review as they are similar to previous changes (5)
  • nix/ext/tests/timescaledb.nix
  • nix/ext/tests/pg_plan_filter.nix
  • nix/packages/lib.nix
  • nix/packages/default.nix
  • nix/overlays/default.nix
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-06T01:52:31.331Z
Learnt from: soedirgo
Repo: supabase/postgres PR: 1987
File: migrations/db/migrations/20251215120934_supabase_superuser.sql:10-10
Timestamp: 2026-01-06T01:52:31.331Z
Learning: For the supabase/postgres repository, do not recommend adding down migrations or flag empty down migrations as issues.

Applied to files:

  • nix/postgresql/generic.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). (3)
  • GitHub Check: no packages to build (aarch64-linux)
  • GitHub Check: no packages to build (x86_64-linux)
  • GitHub Check: no packages to build (aarch64-darwin)
🔇 Additional comments (33)
nix/packages/dbmate-tool.nix (1)

3-3: LGTM! Migration to stdenv.hostPlatform.system is correct.

The addition of stdenv to the function parameters and the update to use stdenv.hostPlatform.system for the CURRENT_SYSTEM substitution properly migrates away from the deprecated system attribute.

Also applies to: 31-31

nix/packages/show-commands.nix (1)

5-5: LGTM! Platform reference correctly updated.

The migration from the system parameter to stdenv and the corresponding update to access apps via stdenv.hostPlatform.system properly aligns with Nix best practices for platform handling.

Also applies to: 17-17

nix/ext/tests/pgroonga.nix (5)

7-9: LGTM: Platform system migration.

The migration from deprecated pkgs.system to pkgs.stdenv.hostPlatform.system aligns with Nix best practices and is consistent with the PR's objective to remove deprecated system attribute usages.


24-24: LGTM: Useful API addition.

The installedExtensions passthru attribute improves the package API by allowing consumers to programmatically query which extensions are bundled.


44-45: LGTM: Platform system migration.

The migration to pkgs.stdenv.hostPlatform.system for accessing PostgreSQL packages is correct and consistent with the deprecated system attribute removal across the codebase.


86-86: LGTM: Platform system migration.

The environment path references correctly use pkgs.stdenv.hostPlatform.system, ensuring proper package resolution for the PostgreSQL service environment.

Also applies to: 90-90


27-28: No action needed. Both withJIT and withoutJIT correctly point to the same pkg in test files. This is intentional design: in the production PostgreSQL package (nix/postgresql/generic.nix), these attributes provide different builds (one with JIT, one without). However, test files are mock environments that only need to satisfy the passthru API expected by extensions—they don't require actual JIT variants. This pattern is consistent across all 16 test files in nix/ext/tests/, confirming it's deliberate test scaffolding.

nix/ext/tests/pgmq.nix (3)

7-9: LGTM: Correct migration to hostPlatform.system.

The replacement of pkgs.system with pkgs.stdenv.hostPlatform.system correctly addresses the deprecation of the system attribute and aligns with Nix best practices for cross-platform builds.


44-45: LGTM: Package selections correctly use hostPlatform.system.

The psql_15 and psql_17 package selections now reference pkgs.stdenv.hostPlatform.system, ensuring consistent platform-specific resolution across the test suite.


24-24: This aliasing is intentional for test environments.

Both withJIT and withoutJIT are aliased to the same package throughout all extension test files. Unlike the canonical PostgreSQL derivation in nix/postgresql/generic.nix where these attributes conditionally select different builds based on JIT support, test environments provide a unified wrapper that doesn't require separate JIT variants. The tests focus on extension compatibility, not JIT functionality.

nix/ext/tests/postgis.nix (1)

7-9: LGTM: Consistent platform and passthru updates.

The migration to pkgs.stdenv.hostPlatform.system and the addition of installedExtensions, withJIT, and withoutJIT passthru attributes are consistent with the PR's objectives and match the pattern across other test files.

Note: As flagged in pgmq.nix, verify whether withJIT and withoutJIT aliasing the same package is intentional for the test environment.

Also applies to: 24-24, 27-28, 66-66, 71-73, 87-92

nix/ext/tests/http.nix (1)

7-9: LGTM: Consistent platform and passthru updates.

The changes mirror the updates in pgmq.nix and postgis.nix, correctly migrating to hostPlatform.system and extending the passthru surface with installedExtensions, withJIT, and withoutJIT.

Also applies to: 24-24, 27-28, 66-66, 71-73, 87-92

nix/ext/tests/pg_repack.nix (1)

7-9: LGTM: Consistent platform and passthru updates.

The changes are consistent with the other test files, correctly applying the hostPlatform.system migration and passthru attribute extensions.

Also applies to: 24-24, 27-28, 66-66, 88-90, 104-109

nix/ext/tests/default.nix (2)

15-17: LGTM: Consistent platform and passthru updates.

The migration to hostPlatform.system and the addition of passthru attributes (withJIT, withoutJIT, installedExtensions) are consistent with the other test files and correctly implement the PR's objectives.

Also applies to: 34-36, 52-53


163-163: No action required — PostgresExtensionTest already accepts and properly handles the lib_name parameter.

The PostgresExtensionTest class in lib.py (line 24) accepts lib_name as an optional parameter, documented as "Name of the shared library (defaults to extension_name)" (line 34), and correctly assigns it with a fallback to the extension name (line 42). The parameter is actively used in version validation assertions throughout the test class (lines 169, 179, 187).

nix/ext/tests/orioledb.nix (2)

14-24: LGTM! Platform and passthru changes are correct.

The migration from pkgs.system to pkgs.stdenv.hostPlatform.system correctly addresses the deprecated API. The hardcoded psql_orioledb-17 reference is appropriate since OrioleDB only supports PostgreSQL 17. The new installedExtensions, withJIT, and withoutJIT passthru attributes extend the package interface consistently with other extension tests.


40-42: LGTM!

The psql_orioledb binding correctly uses hostPlatform.system for the platform-specific package lookup.

nix/ext/tests/pg_safeupdate.nix (1)

7-9: LGTM! Consistent platform migration and passthru extension.

The changes correctly migrate from the deprecated pkgs.system to pkgs.stdenv.hostPlatform.system across all package lookups (installedExtension, psql_15, psql_17). The new passthru attributes (installedExtensions, withJIT, withoutJIT) align with the pattern established across other extension test files.

Also applies to: 24-24, 27-28, 44-45

nix/ext/tests/pgsodium.nix (2)

7-9: LGTM! Platform migration applied consistently including dependencies.

The changes correctly update all platform-specific lookups including the hypopg dependency at line 21. The passthru additions are consistent with other extension tests.

Also applies to: 21-21, 25-25, 28-29


69-69: LGTM!

All service package references and migration script bindings correctly use hostPlatform.system.

Also applies to: 78-80, 94-99

nix/ext/tests/vault.nix (1)

7-9: LGTM! Platform migration and passthru additions are correct.

All platform-specific lookups including the pgsodium dependency have been correctly migrated to use hostPlatform.system. The passthru interface extensions are consistent with the other test files.

Also applies to: 21-21, 25-25, 28-29, 50-51

nix/ext/tests/plv8.nix (1)

8-10: LGTM! Platform migration correctly applied to PG15-only extension.

The changes correctly migrate to hostPlatform.system. The absence of psql_17 is intentional as noted in the file comment (line 1) — plv8 doesn't support PostgreSQL 17. The passthru additions are consistent with other test files.

Also applies to: 25-25, 28-29, 45-45, 68-68

nix/ext/tests/pgjwt.nix (1)

7-9: LGTM! Standard platform migration pattern applied.

The installedExtension lookup and psql_15/psql_17 bindings correctly use hostPlatform.system. The passthru additions are consistent with the established pattern.

Also applies to: 24-24, 27-28, 44-45

nix/ext/tests/plpgsql_check.nix (1)

7-9: LGTM! Platform migration and passthru extension complete.

All platform-specific lookups correctly migrated to hostPlatform.system. The passthru attributes (installedExtensions, withJIT, withoutJIT) are consistent with the other extension test files.

Also applies to: 24-24, 27-28, 44-45

nix/ext/tests/pgrouting.nix (3)

7-9: LGTM: Deprecated system attribute correctly replaced.

The migration from pkgs.system to pkgs.stdenv.hostPlatform.system is correct and aligns with the PR's objective to eliminate deprecated system attribute usage.


17-25: LGTM: Path resolution correctly updated to use hostPlatform.

The buildEnv paths now correctly reference extensions using hostPlatform.system, maintaining consistency with the platform migration. The conditional OrioleDB inclusion logic is sound.


28-28: LGTM: Passthru surface expanded appropriately.

The installedExtensions field provides extension metadata and aligns with the standardized passthru interface introduced across test files in this PR.

nix/cargo-pgrx/mkPgrxExtension.nix (2)

36-53: Correct system derivation in fetchTarball.

The explicit derivation of system from stdenv.hostPlatform.system at line 45 is correct and ensures the imported nixos-22.11 packages match the host platform. This properly replaces the previous deprecated pattern while maintaining the bindgenHook workaround for older pgrx versions.


7-7: Modern Nix pattern correctly implemented.

The migration from system parameter to stdenv follows current Nixpkgs conventions. All call sites (nix/ext/wrappers/default.nix, nix/ext/pg_jsonschema/default.nix, and nix/ext/pg_graphql/default.nix) have stdenv available in scope, and callPackages will automatically inject it to mkPgrxExtension.nix.

nix/postgresql/generic.nix (4)

159-159: __structuredAttrs enables structured attribute handling.

The addition of __structuredAttrs = true is likely required for compatibility with the updated nixpkgs version. This changes how attributes are passed to build scripts but the extensive E2E testing confirms compatibility.


323-337: Correct migration to hostPlatform.system.

The tests section correctly uses stdenv.hostPlatform.system instead of the deprecated system attribute references. This aligns with the PR's objective of replacing deprecated system attributes throughout the codebase.


109-110: Good documentation of the removed hardening flag.

The comment clearly explains why the pie hardening flag was removed (now enabled by default in compilers). This helps future maintainers understand the change.


69-69: The isOrioleDB pattern fallback is defensive but never executes in practice.

The default parameter isOrioleDB ? (builtins.match "[0-9][0-9]_.*" version) != null only applies if isOrioleDB is not explicitly provided. However, default.nix always explicitly passes isOrioleDB based on the flavor definition (either false for standard PostgreSQL or true for OrioleDB). Consequently, the pattern matching fallback is never used and poses no risk of unintended matches.

readline,
openssl,
icu,
icu75,
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

# First, check if the file exists and examine the relevant sections
git ls-files nix/postgresql/generic.nix

Repository: supabase/postgres

Length of output: 87


🏁 Script executed:

# Read the file to verify the ICU 75 changes and documentation
cat -n nix/postgresql/generic.nix | head -160

Repository: supabase/postgres

Length of output: 5433


🌐 Web query:

ICU 75 security vulnerabilities advisories

💡 Result:

Brief summary:

  • ICU 75 (release 75.1) was published April 17, 2024. [1]
  • A notable recent security advisory for ICU is CVE-2025-5222: a stack-buffer-overflow in the genrb tool (SRBRoot::addTag) that can cause memory corruption / local code execution; NVD entry and vendor advisories list affected ICU releases (fixed in later releases). [2][3]
  • Multiple Linux vendors have published security updates addressing CVE-2025-5222 (examples: SUSE, Debian/Ubuntu advisories and package updates). Apply vendor patches or upgrade libicu to a fixed version. [4][3]
  • Note: “ICU” can also mean Johnson Controls’ iSTAR Configuration Utility (ICU) — that is a different product with its own CISA advisory (stack-based buffer overflow, April 2025). If you meant that ICU, see the CISA advisory. [5]

Sources:
[1] ICU 75 release page.
[2] NVD — CVE-2025-5222.
[3] Debian/Ubuntu security trackers (CVE-2025-5222).
[4] SUSE security update for icu (CVE-2025-5222).
[5] CISA ICS advisory for Johnson Controls iSTAR Configuration Utility (different “ICU”).


Address CVE-2025-5222 vulnerability in pinned ICU 75.

ICU 75 has a documented security vulnerability (CVE-2025-5222: stack-buffer-overflow in genrb tool) that can cause memory corruption or local code execution. While the ICU 75 pinning is well-documented here for collation stability, confirm whether this vulnerability is patched in the Nix package or has been otherwise mitigated. If not patched, consider upgrading to a fixed ICU version or applying the vendor security patch.

🤖 Prompt for AI Agents
In @nix/postgresql/generic.nix at line 15, The pinned dependency icu75 in the
Nix expression is flagged for CVE-2025-5222; locate the icu75 symbol in this
file and verify whether the Nix package includes the upstream security fix or
local backport; if it does not, update the pin to a patched upstream version
(e.g., a fixed minor release) or apply the vendor security patch as a patch file
in the derivation (add the patch and include it in the src/patches list), then
rebuild and run tests to confirm collations still match and the CVE is
mitigated.

Comment on lines +197 to +199
(replaceVars ./patches/locale-binary-path.patch {
locale = "${if stdenv.isDarwin then darwin.adv_cmds else lib.getBin stdenv.cc.libc}/bin/locale";
})
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check uses of stdenv vs stdenv' throughout the file
echo "=== All uses of stdenv' ===" 
rg -n "stdenv'" nix/postgresql/generic.nix | head -30

echo -e "\n=== All uses of bare stdenv.* (checking after line 85) ==="
rg -n "stdenv\." nix/postgresql/generic.nix | head -30

echo -e "\n=== Context around line 85 (stdenv' definition) ==="
sed -n '80,90p' nix/postgresql/generic.nix

echo -e "\n=== Context around line 198 (the questioned line) ==="
sed -n '195,202p' nix/postgresql/generic.nix

Repository: supabase/postgres

Length of output: 2799


Use stdenv' instead of bare stdenv for consistency.

At line 198, the code uses stdenv.isDarwin while the entire rest of the file uses stdenv' (defined at line 85 as JIT-aware). All other platform checks in the patches section and throughout the derivation use stdenv'. Change both stdenv.isDarwin and stdenv.cc.libc to use stdenv' for consistency.

🤖 Prompt for AI Agents
In @nix/postgresql/generic.nix around lines 197 - 199, The patch uses bare
stdenv in the replaceVars call; change both occurrences to the JIT-aware stdenv'
used elsewhere: replace stdenv.isDarwin with stdenv'.isDarwin and stdenv.cc.libc
with stdenv'.cc.libc inside the replaceVars invocation (the expression building
locale in the locale-binary-path.patch replacement) so the platform checks and
libc reference are consistent with the rest of the file.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Fix all issues with AI Agents
In @ansible/vars.yml:
- Around line 13-15: The version values for keys postgresorioledb-17, postgres17
and postgres15 include a test suffix "-update-nixpkgs-6" that will break the AMI
build; revert those three strings to their canonical base versions by removing
the "-update-nixpkgs-6" suffix so they become "17.6.0.024-orioledb" for
postgresorioledb-17, "17.6.1.067" for postgres17 and "15.14.1.067" for
postgres15.
📜 Review details

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8ee0403 and fa26b67.

📒 Files selected for processing (1)
  • ansible/vars.yml
⏰ 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)
  • GitHub Check: nix-eval / eval
🔇 Additional comments (1)
ansible/vars.yml (1)

13-15: Clarify the nixpkgs commit IDs and verify IPv6 failures independently.

The specific nixpkgs commit IDs referenced (a76c4553d7e741e17f289224eda135423de0491d → f61125a668a320878494449750330ca58b78c557) cannot be located in public nixpkgs repositories, which prevents verification of whether IPv6/DNS changes were introduced.

Before attributing the "Network is unreachable" errors to the nixpkgs update, confirm:

  • The commit IDs belong to a public repository, or provide the full compare URL/diff
  • The failures reproduce consistently with the previous nixpkgs pin
  • Other upgrade tests with the new nixpkgs version pass without IPv6 errors

If the commits are correct and contain actual changes, ensure IPv6 stack tests pass independently to rule out system-level networking configuration issues.

Comment on lines +13 to +15
postgresorioledb-17: "17.6.0.024-orioledb-update-nixpkgs-6"
postgres17: "17.6.1.067-update-nixpkgs-6"
postgres15: "15.14.1.067-update-nixpkgs-6"
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Search for references to these version strings and postgres_release usage
echo "=== Searching for '-update-nixpkgs-6' references ==="
rg -n "update-nixpkgs-6" --type yaml --type yml

echo "=== Searching for 'postgres_release' usage ==="
rg -n "postgres_release" --type yaml --type yml -A 2

echo "=== Searching for version strings in shell/deployment scripts ==="
rg -n "postgresorioledb-17|postgres17|postgres15" --type sh --type yaml --type yml | head -30

echo "=== Looking for related version identifiers with '-update-nixpkgs' ==="
rg -n "update-nixpkgs" --type yaml --type yml

echo "=== Checking for any release or version documentation ==="
fd -e md -e txt | xargs rg -l "postgre.*version|release.*version" 2>/dev/null | head -10

Repository: supabase/postgres

Length of output: 763


🏁 Script executed:

#!/bin/bash
# Read the ansible/vars.yml file to see context
echo "=== Contents of ansible/vars.yml ===" 
cat -n ansible/vars.yml 2>/dev/null || echo "File not found"

echo ""
echo "=== Searching for postgres_release (all file types) ==="
rg -n "postgres_release" -A 2 -B 2 2>/dev/null | head -40

echo ""
echo "=== Checking git status to see if file is new/modified ==="
git log --oneline -n 1 -- ansible/vars.yml 2>/dev/null || echo "Not in git"

echo ""
echo "=== Searching for any YAML variable definitions ==="
fd -e yaml -o -e yml 2>/dev/null | head -10

echo ""
echo "=== Looking for vars files broadly ==="
find . -name "*vars*" -type f 2>/dev/null | grep -E "\.(yml|yaml|sh)$" | head -15

Repository: supabase/postgres

Length of output: 3679


🏁 Script executed:

#!/bin/bash
echo "=== Git history of ansible/vars.yml (last 10 commits) ==="
git log --oneline -n 10 -- ansible/vars.yml

echo ""
echo "=== Recent changes to postgres_release versions ==="
git log -p -n 5 -- ansible/vars.yml | head -100

echo ""
echo "=== Examining build-test-ami.nix usage ==="
cat -n nix/packages/build-test-ami.nix | sed -n '90,110p'

echo ""
echo "=== Search for other references to postgres_release ==="
rg -n "postgres_release" -B 2 -A 2

echo ""
echo "=== Check for any deployment/provisioning scripts that use these versions ==="
fd -type f -name "*.sh" -o -name "*.nix" | xargs rg -l "PG_VERSION|postgres.*version" | head -10

Repository: supabase/postgres

Length of output: 2449


🏁 Script executed:

#!/bin/bash
echo "=== Search for version format documentation ==="
rg -n "version.*format|version.*scheme|postgres.*version" nix/docs/ -A 2 -B 2

echo ""
echo "=== Check README files for version info ==="
rg -n "postgres_release|version.*string" README.md CONTRIBUTING.md 2>/dev/null || echo "Not found in main docs"

echo ""
echo "=== Look for examples of postgres versions without test suffixes ==="
rg -n "postgres.*[0-9]+\.[0-9]+\.[0-9]+" --no-heading | grep -v "update-nixpkgs" | head -20

echo ""
echo "=== Check git diff to see what changed in this commit ==="
git show fa26b67 --stat

echo ""
echo "=== Full diff of the version bump commit ==="
git show fa26b67

Repository: supabase/postgres

Length of output: 50378


Revert test version suffixes before merge.

The version strings in lines 13-15 contain the -update-nixpkgs-6 suffix, which the git commit message confirms are test identifiers ("chore: bump versions for testing"). These versions are consumed by the AMI build pipeline (nix/packages/build-test-ami.nix) and will cause build failures if the suffixed versions don't exist in artifact registries. Revert to base version strings:

  • "17.6.0.024-orioledb" (from "17.6.0.024-orioledb-update-nixpkgs-6")
  • "17.6.1.067" (from "17.6.1.067-update-nixpkgs-6")
  • "15.14.1.067" (from "15.14.1.067-update-nixpkgs-6")
🤖 Prompt for AI Agents
In @ansible/vars.yml around lines 13 - 15, The version values for keys
postgresorioledb-17, postgres17 and postgres15 include a test suffix
"-update-nixpkgs-6" that will break the AMI build; revert those three strings to
their canonical base versions by removing the "-update-nixpkgs-6" suffix so they
become "17.6.0.024-orioledb" for postgresorioledb-17, "17.6.1.067" for
postgres17 and "15.14.1.067" for postgres15.

@LGUG2Z
Copy link

LGUG2Z commented Jan 6, 2026

The engines smoke test which brings up a Supabase project with each Postgres engine at the AMI versions in this PR and then runs the engine-appropriate tests from nix/tests against them all passed

❯ ./scripts/test-postgres-engines-with-smoke.sh https://github.com/supabase/postgres/pull/1714

============================================================
Supadev PostgreSQL Engine Smoke Test Suite
============================================================
[INFO] Testing PostgresEngines: 15 17 17-oriole
[INFO] Using region: us-east-1
[INFO] Project prefix: supadev-engine-smoke-1767735965
[INFO] Cache directory: /Users/lgug2z/.cache/supadev/postgres
[INFO] GitHub PR URL: https://github.com/supabase/postgres/pull/1714

[STEP] Resolving postgres commit hash...
[SUCCESS] Using postgres commit: fa26b67c6e367b8b147ba27f79f763a5fe0e6ce8
[SUCCESS] Using cached postgres repo at fa26b67c
[INFO] Found 62 SQL test files

============================================================
Test Summary
============================================================

ENGINE          RESULT
------          ------
15              PASSED (379s)
17              PASSED (360s)
17-oriole       PASSED (415s)

============================================================
Total: 3 | Passed: 3 | Failed: 0
============================================================
[INFO] Postgres commit: fa26b67c6e367b8b147ba27f79f763a5fe0e6ce8
[INFO] Cache location: /Users/lgug2z/.cache/supadev/postgres/fa26b67c6e367b8b147ba27f79f763a5fe0e6ce8
[SUCCESS] All tests passed!

The pause/restore smoke tests which bring up a Supabase project, pause it (taking a backup, retiring the VM), and then restore it (to a new VM, restoring the backup) for every Postgres engine all passed.

❯ ./scripts/test-postgres-pause-restore.sh https://github.com/supabase/postgres/pull/1714

============================================================
Supadev PostgreSQL Pause/Restore Test Suite
============================================================
[INFO] Testing PostgresEngines: 15 17 17-oriole
[INFO] Using region: us-east-1
[INFO] Project prefix: supadev-pause-1767744640
[INFO] GitHub PR URL: https://github.com/supabase/postgres/pull/1714

============================================================
Test Summary
============================================================

ENGINE          RESULT
------          ------
15              PASSED (686s)
17              PASSED (869s)
17-oriole       PASSED (806s)

============================================================
Total: 3 | Passed: 3 | Failed: 0
============================================================
[SUCCESS] All tests passed!

The 15->17 and 17->17 upgrade smoke tests both passed (output truncated)

❯ ./scripts/test-postgres-upgrade-with-smoke.sh https://github.com/supabase/postgres/pull/1714

============================================================
Supadev PostgreSQL Smoke Upgrade Test Suite
============================================================
[INFO] Testing PostgresEngines: 15 17
[INFO] Using region: us-east-1
[INFO] Project prefix: supadev-upgrade-smoke-1767748118
[INFO] SQL smoke directory: /Users/lgug2z/supadev/sql/smoke
[INFO] GitHub PR URL: https://github.com/supabase/postgres/pull/1714

============================================================
Testing PostgreSQL Smoke Upgrade: 15
============================================================
[INFO] Project name: supadev-upgrade-smoke-1767748118-pg15
[INFO] Region: us-east-1
[INFO] Start time: 2026-01-06 17:08:38
[SUCCESS] Resolved version: 17.6.1.067-update-nixpkgs-6
[STEP] Creating project with PostgresEngine 15 (ensuring version < 17.6.1.067-update-nixpkgs-6)...
Creating project on release channel "ga" with version "supabase-postgres-15.14.1.063"
[STEP] Printing project information before upgrade...
{
  "dbVersion": "supabase-postgres-15.14.1.063"
}

[SUCCESS] Version 17.6.1.067-update-nixpkgs-6 inserted as GA
[STEP] Printing project upgrade eligibility...
{
  "target_upgrade_versions": [
    {
      "postgres_version": "17",
      "release_channel": "ga",
      "app_version": "supabase-postgres-17.6.1.067-update-nixpkgs-6"
    }
  ],
}
[STEP] Upgrading project to PostgresEngine 17 version 17.6.1.067-update-nixpkgs-6...
[STEP] Printing project information after upgrade...
{
  "dbVersion": "supabase-postgres-17.6.1.067-update-nixpkgs-6"
}

[SUCCESS] Version 17.6.1.067-update-nixpkgs-6 marked as withdrawn
[SUCCESS] PostgresEngine 15 smoke upgrade test completed successfully
[INFO] Total test duration: 1180 seconds

============================================================
Testing PostgreSQL Smoke Upgrade: 17
============================================================
[INFO] Project name: supadev-upgrade-smoke-1767748118-pg17
[INFO] Region: us-east-1
[INFO] Start time: 2026-01-06 17:28:18
[SUCCESS] Resolved version: 17.6.1.067-update-nixpkgs-6
[STEP] Creating project with PostgresEngine 17 (ensuring version < 17.6.1.067-update-nixpkgs-6)...
Creating project on release channel "ga" with version "supabase-postgres-17.6.1.063"
[STEP] Printing project information before upgrade...
{
  "dbVersion": "supabase-postgres-17.6.1.063"
}

[SUCCESS] Version 17.6.1.067-update-nixpkgs-6 inserted as GA
[STEP] Printing project upgrade eligibility...
{
  "target_upgrade_versions": [
    {
      "postgres_version": "17",
      "release_channel": "ga",
      "app_version": "supabase-postgres-17.6.1.067-update-nixpkgs-6"
    }
  ],
}
[STEP] Upgrading project to PostgresEngine 17 version 17.6.1.067-update-nixpkgs-6...
[STEP] Printing project information after upgrade...
{
  "dbVersion": "supabase-postgres-17.6.1.067-update-nixpkgs-6"
}

[SUCCESS] Version 17.6.1.067-update-nixpkgs-6 marked as withdrawn
[SUCCESS] PostgresEngine 17 smoke upgrade test completed successfully
[INFO] Total test duration: 653 seconds

============================================================
Test Summary
============================================================

ENGINE          RESULT
------          ------
15              PASSED (1180s)
17              PASSED (653s)

============================================================
Total: 2 | Passed: 2 | Failed: 0
============================================================
[SUCCESS] All tests passed!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants