Drop psqlpy-python forks; resolve all deps through crates.io#173
Draft
Dev-iL wants to merge 4 commits into
Draft
Drop psqlpy-python forks; resolve all deps through crates.io#173Dev-iL wants to merge 4 commits into
Dev-iL wants to merge 4 commits into
Conversation
Removes the 7-fork constellation (rust-postgres, rust-postgres-array,
rust-postgres-interval, pgvector-rust, rust-decimal, deadpool,
pyo3-async-runtimes) and rewires the call sites that depended on
fork-only API. Cargo.toml/Cargo.lock now resolve every dep through
crates.io. INV-G1/G2 pass.
Source-side replacements:
- col_buffer: new RawBuf<'a> FromSql newtype in
src/value_converter/raw_buf.rs; row.try_get::<usize, RawBuf>(i)
reproduces the fork's raw-bytes accessor without the upstream patch.
- DEALLOCATE PREPARE: removed the explicit deallocate path
(Connection::drop_prepared trait method gone); tokio-postgres
Drop for StatementInner emits Close('S') on the last Arc clone drop,
so cache eviction = autoclose.
- BinaryCopyInWriter empty-buffer trio: bypassed entirely in
src/driver/common.rs; the Python side already builds the binary-format
payload, so the path collapses to sink.send(bytes).await?; sink.finish().
- SslMode VerifyCa/VerifyFull: enforcement moved into build_tls at the
postgres-openssl SslConnector layer (PEER + per-connection hostname
callback). to_internal() collapses to the three upstream-supported
variants.
- SslMode::Allow libpq-faithful semantics: new PsqlpyManager wrapper
(src/driver/psqlpy_manager.rs) implementing deadpool::managed::Manager
with a primary plaintext manager + optional TLS-fallback manager.
is_ssl_required_rejection walks the error source() chain matching
SqlState::INVALID_AUTHORIZATION_SPECIFICATION + "no encryption"
substring. Threaded a PsqlpyPool / PsqlpyClient pair through all
connection-pool consumers; non-Allow modes wrap a single inner
Manager, identical behavior to the old direct deadpool_postgres::Manager
path.
- postgres_array: replaced fork's from_parts_no_panic at
src/value_converter/from_python.rs by pre-validating size before calling
upstream Array::from_parts.
pyo3 ecosystem: 0.25 -> 0.26 (Python 3.10 still in scope, so 0.27's
3.10 drop blocks going further). Changes that would only land cleanly
on pyo3 0.28+ are marked with TODO(python-3.10-drop) comments next to
their reverted form (Python::attach, FromPyObject<'a, 'py>::extract,
OnceLockExt, etc.). Now uses PyOnceLock (0.26-deprecated GILOnceCell)
and avoids the deprecated root PyObject alias by re-aliasing Py<PyAny>
where the type sites are most concentrated.
Project gates:
- cargo build --release --all-features green.
- cargo clippy -p psqlpy --all-features -- -W clippy::all -W clippy::pedantic -D warnings clean (INV-G7).
- cargo fmt -- --check --config use_try_shorthand=true,imports_granularity=Crate clean (INV-G6).
- Existing pytest suite (247 cases, sans test_ssl_mode and test_binary_copy
which need certs / optional pgpq) green against a live postgres.
Deferred for CI / follow-up:
- INV-G3 libpq-Allow e2e pytest, INV-G4 statement-cache eviction
pytest: both need hostssl-only / pg matrix docker infra that only the
CI runners cover; covered by INV-G8 tox matrix run.
- Per-AC review-agent gates (G9-G16) not invoked in this commit;
expecting CI / reviewer to surface anything mechanical.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Backs up the DEALLOCATE-removal in src/connection/impls.rs with a pg_prepared_statements-based check that prepared=false statements don't leak server-side, plus a dual that confirms parameterised prepared=true queries do persist through deadpool's StatementCache. Ride-along: a ruff-format reflow inside test_value_converter.py that the pre-commit hook had been wanting to apply. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
`build_tls` previously returned `NoTls` for `SslMode::Allow`, which left
`PsqlpyManager` without a fallback inner manager — the plaintext attempt
would fail and there was no second side to retry through. The
`Disable | Allow → NoTls` shortcut was inherited from the
`build_psqlpy_manager` shape and was wrong: `Allow` is the only mode that
actively needs the TLS connector even though the *primary* attempt is
plaintext.
`build_tls` now returns a TLS connector for `Allow` (loaded with the
`ca_file` if provided, otherwise accept-any-cert like `Require`) so the
`PsqlpyManager` Allow-pair has a real TLS side to fall back to.
New `python/tests/test_ssl_mode_allow_retry.py` is the manifest's INV-G3
covered end-to-end against a hostssl-only postgres (opted into via
`PSQLPY_HOSTSSL_PORT`):
- `ssl_mode=Allow` + ca_file succeeds via the PsqlpyManager retry path
(verified by query result, not just absence of exception).
- `ssl_mode=Disable` raises against hostssl-only (control: rejection
path).
- `ssl_mode=Require` + ca_file succeeds (control: never plaintext).
Locally verified against:
- port 25434 (default pg_hba, ssl=on) for the full `test_ssl_mode`
parametrisation: 13/13 pass.
- port 25435 (hostssl-only) for the new tests: 3/3 pass.
- Combined full pytest run: 267 passed.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
G14 (test-quality): - Allow-retry test now observes the retry path firing by reading the hostssl-only postgres' container logs for the `no encryption` rejection in the test window. Without that probe, a degenerate `Allow → Require` mapping would also pass the success-only assertion (the precise gap G14 surfaced). The probe is opt-in via PSQLPY_HOSTSSL_DOCKER_CONTAINER; if unset the test still passes the green-query assertion but skips the retry-firing check with skip-text that names the gap. - Disable test narrowed: `pytest.raises(BaseConnectionPoolError)` only, no bare Exception fallback. Adds the same docker-log probe to verify the failure is specifically the `no encryption` diagnostic rather than any failure mode. - test_statement_lifecycle.py: removed the dead `_backend_pid` / `_prepared_count_for_pid` helpers; replaced them with a comment explaining the in-band same-connection approach the tests actually use. G15 (maintainability): - Removed `pool_error_is_ssl_required` from src/driver/psqlpy_manager.rs. It had zero call sites, its docstring claimed a fictional layer of unwrapping (`is_ssl_required_rejection` already accepts &dyn Error and walks .source() itself), and the wrapper added no actual logic. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
rust-postgres,rust-postgres-array,rust-postgres-interval,pgvector-rust,rust-decimal,deadpool,pyo3-async-runtimes) fromCargo.tomland rewires every fork-only call site against in-tree Rust idioms.Cargo.tomlandCargo.locknow resolve every dep through crates.io.PsqlpyManager(src/driver/psqlpy_manager.rs) implementsdeadpool::managed::Managerwith a libpq-faithful plaintext-first / TLS-fallback pair forSslMode::Allow;is_ssl_required_rejectionwalks thesource()chain matchingINVALID_AUTHORIZATION_SPECIFICATION+"no encryption"to decide when to switch sides. Non-Allowmodes wrap a single innerdeadpool_postgres::Manager— identical behavior to the pre-change direct path.RawBuf<'a>FromSqlnewtype (src/value_converter/raw_buf.rs) replaces the fork-onlyRow::col_bufferaccessor viarow.try_get::<usize, RawBuf>(i).DEALLOCATE PREPAREcalls dropped; rely on tokio-postgresDrop for StatementInner→Close('S', name) + Syncfor autoclose. TheConnection::drop_preparedtrait method is gone.BinaryCopyInWriterempty-buffer trio replaced by directCopyInSink<Bytes>::send + finish(Python side already builds the binary-format payload, so the wire framing layer was redundant).SslMode::VerifyCa/VerifyFullverification moved intobuild_tlsat thepostgres-opensslSslConnectorlayer:SslVerifyMode::PEERon the builder, plus a per-connectionset_verify_hostname(false)callback forVerifyCa(theVerifyFullbranch keeps openssl's default hostname-on behavior).to_internal()collapses to the three upstream-supported variants (Disable/Prefer/Require).postgres_array::Array::from_parts_no_panicreplaced by pre-validation + upstreamArray::from_partsat the single call site insrc/value_converter/from_python.rs. The pre-validation reproduces the fork's empty-empty acceptance case while still using the panicking upstream constructor only after the size check passes.TODO(python-3.10-drop)markers next to each — coveringPython::attach,FromPyObject<'a, 'py>::extract,OnceLockExt, the root-levelPyObjectrename, theFromPyObjectBound→FromPyObject<'a, 'py>move, thePyClassGuardErrorconversion, and thepyo3-async-runtimes::future_into_pyT: Send + 'statictightening.Why now / motivation
The forks under
psqlpy-python/had drifted from upstream — the deepest (rust-postgres) was 202 commits behind — and the maintenance cost was paid every time we tried to track an upstream security fix or feature. The actual delta we needed turned out to be fivepubexposures and three convenience methods inrust-postgres, plus one workaround inrust-postgres-array. All of them now live in psqlpy itself, in well-named in-tree Rust idioms that are easier to reason about than out-of-tree forks. Six of the seven forks were Cargo.toml-only repointers; once the rust-postgres delta is gone, they collapse straight to crates.io.Test plan
Verified locally
cargo build --release --all-features— greencargo clippy -p psqlpy --all-features -- -W clippy::all -W clippy::pedantic -D warnings— clean (INV-G7)cargo fmt -- --check --config use_try_shorthand=true,imports_granularity=Crate— clean (INV-G6)mypy,trim trailing whitespace,rust fmt,rust clippy,rust cargo check) — all passed undergit commitgrep -nE 'git\s*=\s*"https?://github.com/psqlpy-python' Cargo.toml Cargo.lock— 0 hits (INV-G1)git grep -nE 'SummitSG-LLC|psqlpy-python/(rust-postgres|deadpool|pyo3-async-runtimes|rust-decimal|rust-postgres-array|rust-postgres-interval|pgvector)'— 0 hits (INV-G2)maturin develop --releasesucceeds; module imports; all sixSslModevariants exposedtest_binary_copy(the path that exercises the newCopyInSink::send + finishbypass) passes.test_ssl_modewas skipped — the local postgres doesn't have TLS enabled, noPOSTGRES_CERT_FILE.Deferred to CI / follow-up
tox -v -c tox.inimatrix across py3.10/3.11/3.12/3.13/3.14 × pg14/15/16/17 (INV-G8). The fmt and clippy gates already passed locally; the gap is multi-Python + multi-pg-version coverage.test_ssl_modecert-backed run against pg14/15/16/17. Exercises the newbuild_tlsPEER + hostname-callback split forVerifyCavsVerifyFull, plus the newPsqlpyManagerAllow-mode plaintext-first wiring.Risk flags worth a second look
DEALLOCATE PREPAREremoval banks on tokio-postgres dropping the lastArc<StatementInner>clone. Any path that holds a clone ofStatementpast the consumer'sResultwould silently leak the prepared statement server-side. INV-G4 (the eviction-leak test) would catch this; it's deferred.postgres-openssl 0.5.3exposesMakeTlsConnector::set_callback(|cfg, _| cfg.set_verify_hostname(false))— that's what theVerifyCabranch uses. If the opensslConnectConfiguration::set_verify_hostnameshape ever changes, this is the line that breaks (src/driver/utils.rs::build_tls).is_ssl_required_rejectionsubstring-matches"no encryption"in the postgres SQLSTATE 28000 error message. Empirically present on pg 12–17 inauth.c:ClientAuthentication, but if a pg version differs, the libpq-Allow retry path silently regresses to never-retry. INV-G3 across pg14–17 would catch this; deferred.RawBufhot-path cost.row.try_get::<usize, RawBuf>(i)adds aFromSqldispatch hop relative to the directcol_buffer(i)field read. The trait impl returns the slice unchanged, so the optimizer should inline through it, but no benchmark was run.Notes for the reviewer
[patch.crates-io]workaround mentioned in PG-6 of the manifest didn't end up being needed in the final landing form — once every direct dep moved to crates.io together with the source-side fork-API removals, thepostgres-types/FromSqltrait-unification issue disappeared on its own.TODO(python-3.10-drop)markers are intentionally co-located with their reverted form so the next pyo3 bump becomes a single grep-and-replace pass rather than a re-derivation.Connection::drop_preparedwas removed from the trait, not deprecated. If any out-of-tree consumer relied on this method, that's a breaking change at the Rust API boundary (the Python API is unaffected).