Skip to content

Commit e067d82

Browse files
committed
fix: prevent backend segfault from user-mapping FFI and harden the FDW
A scan running under a role with no user mapping could crash the backend with SIGSEGV (signal 11). In new(), GetUserMapping() raises a Postgres ERROR (a C longjmp) when no mapping exists; this unwound past the already constructed multi-threaded tokio runtime, skipping its Drop and leaking worker threads. Such a worker pool can also receive job-control signals on a thread other than the main backend thread. pg_timetable's repeated scheduling (CALL timetable.f_control_importers()) made it reliably fatal. Crash fix: - Read the user mapping BEFORE creating the runtime, so a longjmp from GetUserMapping can no longer skip a live runtime. - Use the framework's create_async_runtime() (a current-thread runtime): a Postgres backend is single-threaded and the FDW only block_on's, so a worker pool is both unnecessary and unsafe here. - Connect to etcd lazily in begin_scan / the modify path instead of in new(), which the framework may call at planning time. Robustness: - insert/update/delete: read the key/value out of the Cell directly instead of Display + trim_matches('), which corrupted any key/value that legitimately contained single quotes. - iter_scan: convert non-UTF-8 etcd keys/values lossily instead of panicking, and iterate by index instead of draining the vec front (was O(n^2) per scan). - begin_scan: do not panic when incrementing a prefix yields invalid UTF-8; fall back to the wider, still-correct range. - insert/update: return an error on NULL key/value cells instead of panicking; do not panic on an empty user-mapping options list. - update/delete: error on a missing key instead of silently creating it. Tests: - Regression tests: a scan with no user mapping returns a clean error instead of crashing; keys/values containing single quotes round-trip exactly. Build / CI / reproducibility: - CI: always install cargo-pgrx. Its binary lives in ~/.cargo/bin (the deps cache, keyed on Cargo.lock), not the pgrx cache, so gating the install on the pgrx cache left it missing once Cargo.lock was committed. - Make testcontainers an optional dependency enabled by the pg_test feature (built for cargo pgrx test, not for release); drop unused serde. - Track Cargo.lock (pins the wrappers git dependency) for reproducible builds. - pg_regress: CREATE EXTENSION etcd_fdw (was a stale template name).
1 parent 268054e commit e067d82

7 files changed

Lines changed: 4253 additions & 160 deletions

File tree

.github/workflows/build_test.yaml

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -49,9 +49,13 @@ jobs:
4949
- uses: gerlero/apt-install@v1
5050
with:
5151
packages: build-essential bison flex clang protobuf-compiler libreadline8 libreadline-dev
52+
# Always ensure cargo-pgrx is installed. Its binary lives in ~/.cargo/bin
53+
# (restored by the deps cache, which is keyed on Cargo.lock), NOT in the
54+
# pgrx cache -- so gating the install on the pgrx cache leaves cargo-pgrx
55+
# missing whenever the deps cache key changes (e.g. after Cargo.lock is
56+
# committed/updated). `cargo install` is a fast no-op when already present.
5257
- name: Install pgrx
5358
run: cargo install cargo-pgrx --version 0.16.1 --locked
54-
if: steps.cache_pgrx.outputs.cache-hit != 'true'
5559
- run: cargo pgrx init
5660
if: steps.cache_pgrx.outputs.cache-hit != 'true'
5761
- run: cargo build --verbose

.gitignore

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,6 @@
33
/target
44
*.iml
55
**/*.rs.bk
6-
Cargo.lock
76
pg_regress/results
87
pg_regress/regression.diffs
98
pg_regress/regression.out

0 commit comments

Comments
 (0)