Skip to content

Conversation

@yvan-sraka
Copy link
Collaborator

@yvan-sraka yvan-sraka commented Aug 11, 2025

This PR add a few commits on top of #1743

Summary by CodeRabbit

  • Chores
    • Appended a suffix to select PostgreSQL version strings and expanded the wrappers registry with many new releases and metadata.
    • Updated wrapper packaging and dependency sets across multiple release lines, regenerating lockfile data to broaden supported toolchains and PG versions.
  • New Features
    • Added a CLI helper to discover and maintain extension-version mappings automatically.
  • Documentation
    • Added guidance for automating version updates with example usage and verification steps.

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

@yvan-sraka yvan-sraka requested a review from jfroche August 11, 2025 15:47
@yvan-sraka yvan-sraka self-assigned this Aug 11, 2025
@yvan-sraka yvan-sraka changed the title feat: add version 0.2.0 of wrappers extension feat: add older versions of the wrappers extension Aug 11, 2025
@yvan-sraka yvan-sraka force-pushed the multi-version-ext/wrappers-v0.2.0 branch from ecf641f to ff4cef2 Compare September 12, 2025 13:54
@yvan-sraka yvan-sraka changed the base branch from develop to custom-github-runners September 12, 2025 13:55
@yvan-sraka yvan-sraka force-pushed the custom-github-runners branch 16 times, most recently from 1eb74b8 to db1e5e4 Compare September 29, 2025 14:29
@jfroche jfroche force-pushed the custom-github-runners branch 8 times, most recently from 0878bd2 to b842e89 Compare October 3, 2025 10:53
Base automatically changed from update-nixpkgs to develop January 16, 2026 00:23
@jfroche jfroche force-pushed the multi-version-ext/wrappers-v0.2.0 branch from afb2c34 to c719c72 Compare January 16, 2026 11:36
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 16, 2026

📝 Walkthrough

Walkthrough

Updates Ansible PostgreSQL release identifiers, extends cargo-pgrx version mappings, adds a new CLI Python script and docs to automate updating nix/ext/versions.json, inserts many new wrapper version entries and metadata, modifies the wrappers Nix derivation (Cargo.lock handling, pgrxBinaryName, filtering), and includes multiple wrapper dependency-bump patches (Cargo.toml/Cargo.lock and small source import adjustments).

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

  • Update nixpkgs #1714: Updates the same ansible/vars.yml postgres release keys and related packaging changes.

Suggested labels

extensions, nix, docs, infrastructure

Suggested reviewers

  • samrose
🚥 Pre-merge checks | ✅ 1 | ❌ 2
❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Description check ⚠️ Warning The description is minimal and only references a prior PR, without providing context about changes made in this PR. It does not follow the available template structure or explain the actual modifications to versions, patches, or documentation. Expand the description to include what versions were added, why they are important, and summarize the key changes (e.g., new patches for v0.1.10, v0.1.4, v0.2.0; updated documentation; helper script addition).
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (1 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the main change: adding older versions of the wrappers extension. It is concise, single-sentence, and directly relates to the substantial additions in nix/ext/versions.json and the supporting infrastructure changes.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch multi-version-ext/wrappers-v0.2.0

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@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

Caution

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

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

86-100: Duplicate condition for version 0.5.4 will cause unreachable code.

Lines 86-90 and 91-95 both check for version "0.5.4". The second block (lines 91-95) will never be reached. Based on the pattern, this appears to be a copy-paste error.

Looking at the sequence: 0.5.2, 0.5.3, 0.5.4, 0.5.4 (duplicate), 0.5.5, 0.5.6, 0.5.7 — the second 0.5.4 block should likely be removed or updated to a different version if there's a missing version in the chain.

🐛 Proposed fix - remove duplicate condition
             else if builtins.compareVersions "0.5.4" version == 0 then
               {
                 "clickhouse-rs-1.1.0-alpha.1" = "sha256-nKiGzdsAgJej8NgyVOqHaD1sZLrNF1RPfEhu2pRwZ6o=";
                 "iceberg-catalog-s3tables-0.5.1" = "sha256-1JkB2JExukABlbW1lZPolNQCYb9URi8xNYY3APmiGq0=";
               }
-            else if builtins.compareVersions "0.5.4" version == 0 then
-              {
-                "clickhouse-rs-1.1.0-alpha.1" = "sha256-nKiGzdsAgJej8NgyVOqHaD1sZLrNF1RPfEhu2pRwZ6o=";
-                "iceberg-catalog-s3tables-0.5.1" = "sha256-1JkB2JExukABlbW1lZPolNQCYb9URi8xNYY3APmiGq0=";
-              }
             else if builtins.compareVersions "0.5.5" version == 0 then
🤖 Fix all issues with AI agents
In `@nix/ext/scripts/update_versions_json.py`:
- Around line 1-2: The shebang on the first line has an extra space ("#!
/usr/bin/env ...") which can break execution; edit the top of the script to
remove the space so the first line reads "#!/usr/bin/env nix-shell" (leave the
second nix-shell directive with flags unchanged) to ensure the script is
executable by the system interpreter lookup.
- Around line 22-37: The get_tags function currently ignores lines containing
"^{ }" and therefore uses tag-object hashes for annotated tags; update get_tags
to prefer peeled commit hashes by collecting both normal and peeled entries from
git ls-remote --tags output and, when a peeled line (ref ending with "^{ }")
exists for a tag, use its commit hash instead of the tag-object hash; keep the
existing behavior for lightweight tags by falling back to the non-peeled hash
when no peeled entry is present and ensure you still validate tags via
parse_version/InvalidVersion as before.
🧹 Nitpick comments (1)
nix/ext/scripts/update_versions_json.py (1)

13-15: Make VERSIONS_JSON_PATH location-independent.

Line 14 and the load/save logic are CWD-relative; running from repo root would write a stray versions.json. Resolving the path relative to this script avoids accidental miswrites.

♻️ Proposed refactor
-POSTGRES_VERSIONS: List[str] = ["15", "17"]
-VERSIONS_JSON_PATH = "versions.json"
+POSTGRES_VERSIONS: List[str] = ["15", "17"]
+VERSIONS_JSON_PATH = Path(__file__).resolve().parents[1] / "versions.json"
@@
-    if not Path(VERSIONS_JSON_PATH).exists():
+    if not VERSIONS_JSON_PATH.exists():
         return {}
-    with open(VERSIONS_JSON_PATH, "r", encoding="utf-8") as f:
+    with VERSIONS_JSON_PATH.open("r", encoding="utf-8") as f:
         return json.load(f)
@@
-    with open(VERSIONS_JSON_PATH, "w", encoding="utf-8") as f:
+    with VERSIONS_JSON_PATH.open("w", encoding="utf-8") as f:
         json.dump(sorted_data, f, indent=2)
         f.write("\n")

Also applies to: 46-50, 74-75

📜 Review details

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between aa78cb3 and c719c72.

⛔ Files ignored due to path filters (17)
  • nix/ext/wrappers/v0.1.10/Cargo.lock is excluded by !**/*.lock
  • nix/ext/wrappers/v0.1.11/Cargo.lock is excluded by !**/*.lock
  • nix/ext/wrappers/v0.1.12/Cargo.lock is excluded by !**/*.lock
  • nix/ext/wrappers/v0.1.14/Cargo.lock is excluded by !**/*.lock
  • nix/ext/wrappers/v0.1.15/Cargo.lock is excluded by !**/*.lock
  • nix/ext/wrappers/v0.1.16/Cargo.lock is excluded by !**/*.lock
  • nix/ext/wrappers/v0.1.17/Cargo.lock is excluded by !**/*.lock
  • nix/ext/wrappers/v0.1.18/Cargo.lock is excluded by !**/*.lock
  • nix/ext/wrappers/v0.1.19/Cargo.lock is excluded by !**/*.lock
  • nix/ext/wrappers/v0.1.4/Cargo.lock is excluded by !**/*.lock
  • nix/ext/wrappers/v0.1.5/Cargo.lock is excluded by !**/*.lock
  • nix/ext/wrappers/v0.1.6/Cargo.lock is excluded by !**/*.lock
  • nix/ext/wrappers/v0.1.7/Cargo.lock is excluded by !**/*.lock
  • nix/ext/wrappers/v0.1.8/Cargo.lock is excluded by !**/*.lock
  • nix/ext/wrappers/v0.1.9/Cargo.lock is excluded by !**/*.lock
  • nix/ext/wrappers/v0.2.0/Cargo.lock is excluded by !**/*.lock
  • nix/ext/wrappers/v0.3.0/Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (11)
  • ansible/vars.yml
  • nix/cargo-pgrx/versions.json
  • nix/docs/update-extension.md
  • nix/ext/scripts/update_versions_json.py
  • nix/ext/versions.json
  • nix/ext/wrappers/default.nix
  • nix/ext/wrappers/v0.1.10/0001-bump-aws-config-to-0.55.3.patch
  • nix/ext/wrappers/v0.1.4/0001-bump-gcp-bigquery-client-to-0.16.8.patch
  • nix/ext/wrappers/v0.1.4/0002-bump-pgrx-to-0.6.1.patch
  • nix/ext/wrappers/v0.2.0/0001-bump-pgrx-to-0.11.3.patch
  • nix/ext/wrappers/v0.3.0/0001-bump-pgrx-to-0.11.3.patch
🧰 Additional context used
🪛 Ruff (0.14.11)
nix/ext/scripts/update_versions_json.py

1-1: Shebang should contain python, pytest, or uv run

(EXE003)


2-2: Shebang should be at the beginning of the file

(EXE005)


18-18: subprocess call: check for execution of untrusted input

(S603)

⏰ 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 (14)
nix/ext/wrappers/v0.1.10/0001-bump-aws-config-to-0.55.3.patch (2)

1172-1186: Cargo.toml dependency updates are correct.

The AWS SDK dependency bumps are straightforward and the optional flags are preserved for the s3_fdw feature.


1-1171: Cargo.lock updates are safe and appropriate for the AWS SDK dependency bump.

The lockfile changes correctly reflect the version upgrades from the patch:

  • aws-config: 0.54.1 → 0.55.3 (no CVE)
  • aws-sdk-s3: 0.24.0 → 0.25.1 (no CVE)
  • tokio: 1.25.0 → 1.29.1 (no CVE; CVE-2024-27308 only affects 1.30.0+)
  • hyper: 0.14.20 → 0.14.32 (safe; older CVEs fixed by 0.14.10)

The removal of test-only dependencies (aws-smithy-protocol-test, pretty_assertions) reduces the attack surface. The addition of syn 2.x alongside syn 1.x maintains compatibility with both old and new macro crates. No security advisories were found for the versions introduced in this patch.

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

2-35: Confirm the new cargo hashes are reproducible.

Line 2-35 introduces new versions and toolchain mappings; a stale hash/cargoHash will break Nix builds. Please re-run your hash generation workflow to validate these values.

ansible/vars.yml (1)

13-15: Verify the -old-wrappers-ext suffix matches published artifacts.

Line 13-15 now appends the suffix to release identifiers; confirm downstream package names or download lookups are produced with the same suffix to avoid missing binaries.

nix/ext/versions.json (1)

584-773: Validate new wrappers version entries against real tags and assets.

Line 584-773 adds many historical wrappers releases. Please ensure each hash corresponds to the correct tag and that the per-version patch/lockfile artifacts expected by the Nix build exist for those versions.

nix/ext/wrappers/v0.2.0/0001-bump-pgrx-to-0.11.3.patch (1)

8-218: Confirm the pgrx/bindgen bump still builds with the pinned toolchain.

Line 8-218 updates pgrx to 0.11.3 and refreshes the lockfile (bindgen 0.69.5, etc.). These bumps can change MSRV/clang requirements, so please run the v0.2.0 wrapper build with rust 1.76.0 to ensure the Nix env still satisfies dependencies.

nix/ext/scripts/update_versions_json.py (1)

17-19: Core CLI flow and hashing logic look solid.

Also applies to: 40-43, 53-66, 79-87

nix/docs/update-extension.md (1)

228-245: LGTM!

Clear documentation for the new automation script with a practical two-step workflow. The example commands and verification steps are helpful.

nix/ext/wrappers/v0.1.4/0001-bump-gcp-bigquery-client-to-0.16.8.patch (1)

1-936: LGTM!

The patch correctly bumps gcp-bigquery-client from a git source to version 0.16.8 on crates.io, which improves reproducibility. The transitive dependency updates are consistent with this change, and the addition of the all_fdws feature is useful for building all FDWs together.

nix/ext/wrappers/v0.1.4/0002-bump-pgrx-to-0.6.1.patch (1)

1-1137: LGTM!

The patch correctly upgrades from pgx 0.5.6 to 0.6.1 with appropriate API changes:

  • Datum import path updated to pgx::pg_sys::Datum
  • Logging helpers updated to use ereport! macro and log! macro
  • Default PostgreSQL version bumped from pg14 to pg15

The removal of pg10 support and shutdown_hooks dependency aligns with pgrx 0.6.x requirements.

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

57-66: LGTM!

The postPatch and lockFile resolution logic correctly handles version-specific Cargo.lock files. This is necessary because patches may modify dependencies, causing the original lockfile to mismatch. The conditional copy in postPatch ensures the patched lockfile is used when available.


118-119: LGTM!

The pgrxBinaryName attribute correctly handles the pgx → pgrx binary rename that occurred around version 0.7.4. For older pgrx versions (< 0.7.4), the binary is named "pgx"; for newer versions, it's "pgrx".


181-187: Version-specific patches look appropriate.

Each version has isolated patches in its own directory, which is maintainable. The comment on lines 181-182 noting the Rust 1.80.0 inference issue with time crate is helpful context.


201-205: LGTM!

The updated filter correctly excludes versions marked with ignore: true in versions.json, which is useful for temporarily disabling versions (e.g., for CI as mentioned in the PR objectives) without removing them entirely.

✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.

Copy link
Contributor

@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 `@nix/ext/scripts/update_versions_json.py`:
- Around line 46-49: The for-loop over tags uses an unused variable commit_hash;
change the loop to for tag, _ in tags.items(): (or for tag in tags:) so the
intent is clear and the unused variable is removed—update the loop that
references tags and peeled_tags (the block starting with "for tag, commit_hash
in tags.items():") to use an underscore or iterate only over tag when checking
and assigning peeled_tags[tag].
🧹 Nitpick comments (1)
nix/ext/scripts/update_versions_json.py (1)

17-19: Consider adding a timeout to prevent indefinite hangs.

Network operations like git ls-remote and nix-prefetch-git could hang indefinitely on network issues. Adding a timeout improves robustness for this maintenance tool.

♻️ Proposed fix
-def run(cmd: List[str]) -> str:
-    result = subprocess.run(cmd, capture_output=True, text=True, check=True)
+def run(cmd: List[str], timeout: int = 300) -> str:
+    result = subprocess.run(cmd, capture_output=True, text=True, check=True, timeout=timeout)
     return result.stdout.strip()

@yvan-sraka yvan-sraka force-pushed the multi-version-ext/wrappers-v0.2.0 branch from bb1f5aa to 8e8b98e Compare January 19, 2026 12:49
Copy link
Contributor

@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: 4

🤖 Fix all issues with AI agents
In `@nix/ext/scripts/update_versions_json.py`:
- Around line 13-14: The VERSIONS_JSON_PATH currently points to "versions.json"
which is relative to the current working directory; update it to be anchored to
the script file so it always targets the nix/ext/versions.json file. In
update_versions_json.py, import Path from pathlib and change VERSIONS_JSON_PATH
to compute Path(__file__).resolve().parent / "versions.json" (keep
POSTGRES_VERSIONS unchanged) and use that Path for open/read/write operations so
the script writes to the script directory regardless of CWD.

In `@nix/ext/wrappers/default.nix`:
- Around line 64-66: Align the existence check used by cargoLock.lockFile with
postPatch by testing the file path instead of the directory: update the
conditional in cargoLock.lockFile to use builtins.pathExists
./v${version}/Cargo.lock (the full file path) and fall back to
"${src}/Cargo.lock" when that file is absent; this mirrors the check in
postPatch and ensures the exact Cargo.lock file is validated.
- Line 95: The version comparison for pgrxBinaryName is inverted: change the
builtins.compareVersions call so pgrxVersion is the first argument and "0.7.4"
is the second (i.e., use builtins.compareVersions pgrxVersion "0.7.4" >= 0) so
that pgrxBinaryName resolves to "pgx" for pgrx versions >= 0.7.4; update the
expression referencing pgrxBinaryName accordingly.

In `@nix/ext/wrappers/v0.1.4/0002-bump-pgrx-to-0.6.1.patch`:
- Around line 535-547: The log level was unintentionally raised by replacing
elog(PgLogLevel::DEBUG1, ...) with log!(...), which emits at PostgreSQL's LOG
level; in the log_debug function (and any similar uses) restore the original
DEBUG1 verbosity by replacing the log! call with the pgx debug1! macro (i.e.,
use debug1!("wrappers: {}", msg)); ensure you update only the call in log_debug
(and any other places where elog(..., DEBUG1) was replaced) to use debug1! to
preserve the original DEBUG1 behavior.

Comment on lines +13 to +14
POSTGRES_VERSIONS: List[str] = ["15", "17"]
VERSIONS_JSON_PATH = "versions.json"
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Fix versions.json path resolution (currently relative to CWD).
Running the script from repo root (as documented) will write to ./versions.json, not nix/ext/versions.json. Anchor the path to the script location to avoid updating the wrong file.

🔧 Proposed fix
-VERSIONS_JSON_PATH = "versions.json"
+VERSIONS_JSON_PATH = Path(__file__).resolve().parents[1] / "versions.json"
@@
-    if not Path(VERSIONS_JSON_PATH).exists():
+    if not VERSIONS_JSON_PATH.exists():
         return {}
📝 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
POSTGRES_VERSIONS: List[str] = ["15", "17"]
VERSIONS_JSON_PATH = "versions.json"
POSTGRES_VERSIONS: List[str] = ["15", "17"]
VERSIONS_JSON_PATH = Path(__file__).resolve().parents[1] / "versions.json"
🤖 Prompt for AI Agents
In `@nix/ext/scripts/update_versions_json.py` around lines 13 - 14, The
VERSIONS_JSON_PATH currently points to "versions.json" which is relative to the
current working directory; update it to be anchored to the script file so it
always targets the nix/ext/versions.json file. In update_versions_json.py,
import Path from pathlib and change VERSIONS_JSON_PATH to compute
Path(__file__).resolve().parent / "versions.json" (keep POSTGRES_VERSIONS
unchanged) and use that Path for open/read/write operations so the script writes
to the script directory regardless of CWD.

Comment on lines 64 to +66
cargoLock = {
lockFile = "${src}/Cargo.lock";
lockFile =
if builtins.pathExists ./v${version} then ./v${version}/Cargo.lock else "${src}/Cargo.lock";
Copy link
Contributor

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, explore the structure of nix/ext/wrappers
cd nix/ext/wrappers && find . -maxdepth 2 -type f -o -type d | head -50

Repository: supabase/postgres

Length of output: 808


🏁 Script executed:

# Check for version directories matching pattern v[0-9]
fd -t d '^v[0-9]' nix/ext/wrappers -d 1

Repository: supabase/postgres

Length of output: 494


🏁 Script executed:

# Check the full file context around lines 64-66
cat -n nix/ext/wrappers/default.nix | sed -n '50,80p'

Repository: supabase/postgres

Length of output: 1795


🏁 Script executed:

# Run the proposed check from the review comment
cd nix/ext/wrappers && fd -t d '^v[0-9]' -d 1 | while read -r d; do
  [ -f "$d/Cargo.lock" ] || echo "missing Cargo.lock: $d"
done

Repository: supabase/postgres

Length of output: 43


Use file-level path check for consistency with postPatch.

The postPatch section (line 59) checks builtins.pathExists ./v${version}/Cargo.lock directly, but cargoLock.lockFile (line 66) checks only builtins.pathExists ./v${version}. Checking the directory is weaker than checking the file—align the logic by verifying the file path exists, matching the defensive approach already used in postPatch.

🔧 Proposed fix
-          lockFile =
-            if builtins.pathExists ./v${version} then ./v${version}/Cargo.lock else "${src}/Cargo.lock";
+          lockFile =
+            if builtins.pathExists ./v${version}/Cargo.lock then ./v${version}/Cargo.lock else "${src}/Cargo.lock";
📝 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
cargoLock = {
lockFile = "${src}/Cargo.lock";
lockFile =
if builtins.pathExists ./v${version} then ./v${version}/Cargo.lock else "${src}/Cargo.lock";
cargoLock = {
lockFile =
if builtins.pathExists ./v${version}/Cargo.lock then ./v${version}/Cargo.lock else "${src}/Cargo.lock";
🤖 Prompt for AI Agents
In `@nix/ext/wrappers/default.nix` around lines 64 - 66, Align the existence check
used by cargoLock.lockFile with postPatch by testing the file path instead of
the directory: update the conditional in cargoLock.lockFile to use
builtins.pathExists ./v${version}/Cargo.lock (the full file path) and fall back
to "${src}/Cargo.lock" when that file is absent; this mirrors the check in
postPatch and ensures the exact Cargo.lock file is validated.

};
};

pgrxBinaryName = if builtins.compareVersions "0.7.4" pgrxVersion >= 0 then "pgx" else "pgrx";
Copy link
Contributor

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:

cat -n nix/ext/wrappers/default.nix | sed -n '85,105p'

Repository: supabase/postgres

Length of output: 1116


🏁 Script executed:

cat > /tmp/test_versions.nix << 'EOF'
# Testing builtins.compareVersions behavior
# compareVersions a b returns:
# -1 if a < b
#  0 if a == b
#  1 if a > b

let
  v1 = "0.7.3";
  v2 = "0.7.4";
  v3 = "0.7.5";
in
{
  # Test: compareVersions "0.7.4" "0.7.3" should return 1 (0.7.4 > 0.7.3)
  test1 = builtins.compareVersions "0.7.4" "0.7.3";
  # Test: compareVersions "0.7.4" "0.7.5" should return -1 (0.7.4 < 0.7.5)
  test2 = builtins.compareVersions "0.7.4" "0.7.5";
  # Test: compareVersions "0.7.3" "0.7.4" should return -1 (0.7.3 < 0.7.4)
  test3 = builtins.compareVersions "0.7.3" "0.7.4";
}
EOF

Repository: supabase/postgres

Length of output: 43


🏁 Script executed:

rg "compareVersions" -A 2 -B 2

Repository: supabase/postgres

Length of output: 5810


🏁 Script executed:

cat -n nix/ext/wrappers/default.nix | sed -n '85,110p'

Repository: supabase/postgres

Length of output: 1333


Fix argument order in compareVersions for pgrxBinaryName.
The current expression builtins.compareVersions "0.7.4" pgrxVersion >= 0 inverts the logic—it selects "pgx" for versions ≤0.7.4 instead of ≥0.7.4.

🔧 Proposed fix
-pgrxBinaryName = if builtins.compareVersions "0.7.4" pgrxVersion >= 0 then "pgx" else "pgrx";
+pgrxBinaryName = if builtins.compareVersions pgrxVersion "0.7.4" >= 0 then "pgx" else "pgrx";
📝 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
pgrxBinaryName = if builtins.compareVersions "0.7.4" pgrxVersion >= 0 then "pgx" else "pgrx";
pgrxBinaryName = if builtins.compareVersions pgrxVersion "0.7.4" >= 0 then "pgx" else "pgrx";
🤖 Prompt for AI Agents
In `@nix/ext/wrappers/default.nix` at line 95, The version comparison for
pgrxBinaryName is inverted: change the builtins.compareVersions call so
pgrxVersion is the first argument and "0.7.4" is the second (i.e., use
builtins.compareVersions pgrxVersion "0.7.4" >= 0) so that pgrxBinaryName
resolves to "pgx" for pgrx versions >= 0.7.4; update the expression referencing
pgrxBinaryName accordingly.

Comment on lines +535 to +547
pub fn report_error(code: PgSqlErrorCode, msg: &str) {
- ereport(PgLogLevel::ERROR, code, msg, "Wrappers", 0, 0);
+ ereport!(PgLogLevel::ERROR, code, msg, "Wrappers");
}

/// Log debug message to Postgres log.
@@ -29,7 +29,7 @@ pub fn report_error(code: PgSqlErrorCode, msg: &str) {
/// messages.
#[inline]
pub fn log_debug(msg: &str) {
- elog(PgLogLevel::DEBUG1, &format!("wrappers: {}", msg));
+ log!("wrappers: {}", msg);
}
Copy link
Contributor

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

🌐 Web query:

pgrx 0.6.x logging macros log! debug1 PgLogLevel

💡 Result:

In pgrx 0.6.x, the common pattern is:

1) Use the level-specific macros (no PgLogLevel needed)

These are exported via pgrx::prelude::* (and also exist under pgrx::pg_sys). They take format!-style arguments:

use pgrx::prelude::*;

log!("Hello from LOG");        // LOG level
debug1!("Hello from DEBUG1");  // DEBUG1 level

log! logs at PostgreSQL LOG level, and debug1! logs at PostgreSQL DEBUG1 level. [1][2][3]

2) Use ereport! when you need an explicit PgLogLevel (and usually an SQL error code)

PgLogLevel is the enum of Postgres log levels (e.g., DEBUG1, LOG, WARNING, ERROR, etc.). [4]

use pgrx::prelude::*;
use pgrx::pg_sys::elog::PgLogLevel;
use pgrx::PgSqlErrorCode;

ereport!(
    PgLogLevel::DEBUG1,
    PgSqlErrorCode::ERRCODE_SUCCESSFUL_COMPLETION,
    "message with explicit PgLogLevel"
);

ereport! is also what you use when you must attach a specific SQLSTATE / PgSqlErrorCode. [5][4]

3) Seeing messages depends on PostgreSQL log settings

Whether DEBUG1/LOG output appears is controlled by PostgreSQL configuration (e.g., log_min_messages, client_min_messages). The macros emit at those levels; Postgres decides whether to display/store them. [2][1]

Sources:
[1] https://docs.rs/pgrx/latest/i686-pc-windows-msvc/pgrx/macro.log.html
[2] https://docs.rs/pgrx/latest/i686-pc-windows-msvc/pgrx/macro.debug1.html
[3] https://docs.rs/pgrx/latest/pgrx/prelude/index.html
[4] https://docs.rs/pgrx/latest/pgrx/pg_sys/submodules/elog/enum.PgLogLevel.html
[5] https://docs.rs/pgrx/latest/pgrx/prelude/macro.ereport.html


🌐 Web query:

pgx crate 0.6 documentation logging elog debug level

💡 Result:

In pgx 0.6.x, Postgres “elog-style” logging is done via pgx’s logging macros (re-exported in pgx::prelude::*), e.g. debug1!debug5!, plus log!, info!, notice!, warning!, error!, and the lower-level ereport! macro. [1]

Example (DEBUG level):

use pgx::prelude::*;

#[pg_extern]
fn demo() {
    debug1!("hello from pgx debug1");
    debug3!("more detail at debug3");
}

If your DEBUG* messages aren’t showing up, you must configure PostgreSQL to emit them:

  • Server log: set log_min_messages to DEBUG1 (or DEBUG2/…/DEBUG5) or lower. [2]
  • Client (psql) output: set client_min_messages to DEBUG1 (or lower). [3]

For example:

SET client_min_messages = DEBUG1;
-- and/or
SET log_min_messages = DEBUG1;

Sources: [1] https://docs.rs/pgx/latest/pgx/ , [2] https://postgrespro.com/docs/postgresql/current/runtime-config-logging , [3] https://www.postgresql.org/docs/9.1/runtime-config-logging.html


Change in logging level from DEBUG1 to LOG. The code changed from elog(PgLogLevel::DEBUG1, ...) (DEBUG level) to log!(...) (LOG level). In pgx 0.6.x, log! emits at PostgreSQL's LOG level, which is higher in verbosity than DEBUG1. To maintain DEBUG1 level, use debug1!("wrappers: {}", msg); instead.

🤖 Prompt for AI Agents
In `@nix/ext/wrappers/v0.1.4/0002-bump-pgrx-to-0.6.1.patch` around lines 535 -
547, The log level was unintentionally raised by replacing
elog(PgLogLevel::DEBUG1, ...) with log!(...), which emits at PostgreSQL's LOG
level; in the log_debug function (and any similar uses) restore the original
DEBUG1 verbosity by replacing the log! call with the pgx debug1! macro (i.e.,
use debug1!("wrappers: {}", msg)); ensure you update only the call in log_debug
(and any other places where elog(..., DEBUG1) was replaced) to use debug1! to
preserve the original DEBUG1 behavior.

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.

3 participants