-
-
Notifications
You must be signed in to change notification settings - Fork 228
feat: add older versions of the wrappers extension
#1748
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
wrappers extensionwrappers extension
ecf641f to
ff4cef2
Compare
1eb74b8 to
db1e5e4
Compare
0878bd2 to
b842e89
Compare
fa26b67 to
0131d17
Compare
afb2c34 to
c719c72
Compare
📝 WalkthroughWalkthroughUpdates 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
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this 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: MakeVERSIONS_JSON_PATHlocation-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
⛔ Files ignored due to path filters (17)
nix/ext/wrappers/v0.1.10/Cargo.lockis excluded by!**/*.locknix/ext/wrappers/v0.1.11/Cargo.lockis excluded by!**/*.locknix/ext/wrappers/v0.1.12/Cargo.lockis excluded by!**/*.locknix/ext/wrappers/v0.1.14/Cargo.lockis excluded by!**/*.locknix/ext/wrappers/v0.1.15/Cargo.lockis excluded by!**/*.locknix/ext/wrappers/v0.1.16/Cargo.lockis excluded by!**/*.locknix/ext/wrappers/v0.1.17/Cargo.lockis excluded by!**/*.locknix/ext/wrappers/v0.1.18/Cargo.lockis excluded by!**/*.locknix/ext/wrappers/v0.1.19/Cargo.lockis excluded by!**/*.locknix/ext/wrappers/v0.1.4/Cargo.lockis excluded by!**/*.locknix/ext/wrappers/v0.1.5/Cargo.lockis excluded by!**/*.locknix/ext/wrappers/v0.1.6/Cargo.lockis excluded by!**/*.locknix/ext/wrappers/v0.1.7/Cargo.lockis excluded by!**/*.locknix/ext/wrappers/v0.1.8/Cargo.lockis excluded by!**/*.locknix/ext/wrappers/v0.1.9/Cargo.lockis excluded by!**/*.locknix/ext/wrappers/v0.2.0/Cargo.lockis excluded by!**/*.locknix/ext/wrappers/v0.3.0/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
ansible/vars.ymlnix/cargo-pgrx/versions.jsonnix/docs/update-extension.mdnix/ext/scripts/update_versions_json.pynix/ext/versions.jsonnix/ext/wrappers/default.nixnix/ext/wrappers/v0.1.10/0001-bump-aws-config-to-0.55.3.patchnix/ext/wrappers/v0.1.4/0001-bump-gcp-bigquery-client-to-0.16.8.patchnix/ext/wrappers/v0.1.4/0002-bump-pgrx-to-0.6.1.patchnix/ext/wrappers/v0.2.0/0001-bump-pgrx-to-0.11.3.patchnix/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
optionalflags are preserved for thes3_fdwfeature.
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 ofsyn 2.xalongsidesyn 1.xmaintains 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/cargoHashwill break Nix builds. Please re-run your hash generation workflow to validate these values.ansible/vars.yml (1)
13-15: Verify the-old-wrappers-extsuffix 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-clientfrom 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 theall_fdwsfeature 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:
Datumimport path updated topgx::pg_sys::Datum- Logging helpers updated to use
ereport!macro andlog!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
pgrxBinaryNameattribute 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
timecrate is helpful context.
201-205: LGTM!The updated filter correctly excludes versions marked with
ignore: truein 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.
There was a problem hiding this 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-remoteandnix-prefetch-gitcould 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()
... which is handy when it comes to compute hashes for every source tarball in a bulk :)
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
bb1f5aa to
8e8b98e
Compare
There was a problem hiding this 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.
| POSTGRES_VERSIONS: List[str] = ["15", "17"] | ||
| VERSIONS_JSON_PATH = "versions.json" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| 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.
| cargoLock = { | ||
| lockFile = "${src}/Cargo.lock"; | ||
| lockFile = | ||
| if builtins.pathExists ./v${version} then ./v${version}/Cargo.lock else "${src}/Cargo.lock"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 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 -50Repository: 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 1Repository: 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"
doneRepository: 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.
| 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"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 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";
}
EOFRepository: supabase/postgres
Length of output: 43
🏁 Script executed:
rg "compareVersions" -A 2 -B 2Repository: 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.
| 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.
| 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); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 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 levellog! 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_messagestoDEBUG1(orDEBUG2/…/DEBUG5) or lower. [2] - Client (psql) output: set
client_min_messagestoDEBUG1(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.
This PR add a few commits on top of #1743
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.