Repository navigation
turnloop-postgres: host API gaps from Perry's pg binding (#57–#62) - #99
Merged
Merged
Conversation
Every event from `next_event` borrows the connection's input buffer until the next mutable call. `types::Value::into_owned` existed, but `Row` yields `Result<Option<&[u8]>>` and `Fields` yields `Field<'a>`, so every host that wants to keep a row, a field list or a diagnostic wrote the same ~80-line layer copying them out value by value. `Event::into_owned()` now returns an `OwnedEvent`, mirroring `Event` with its borrowed payloads copied out. Rows, field lists and server errors become `OwnedRow`, `OwnedFields` and `OwnedServerError`, also reachable directly as `Row/Fields/ServerError::into_owned()`. Each is one copy of the already validated wire bytes (one allocation, like `ConnectionFailure`), and each lends back the existing borrowed iterator (`row()`, `fields()`, `server_error()`), so no second decoding path exists. `OwnedRow::get` indexes a column directly. `OwnedEvent::as_event()` lends a retained event back as a borrowed `Event`, so a host handler written for one form consumes both. The borrowed, allocation-free path is unchanged. The new protocol test retains fields, a row with a NULL, a notice and a notification, then feeds a second batch whose `receive` compacts the input buffer over the bytes those events borrowed, and asserts every owned value (and the `as_event` round trip) is unchanged. Closes #58
`Connection::new` consumes the `Config`, and nothing handed the password back. The README asks the host to build `ScramSha256` when `ScramNeeded` fires, so every host kept its own second copy of the database password purely to feed that constructor; the bundled async client did exactly that with a clone. The issue offers two fixes. This takes the accessor, `Connection::config()`, rather than `start_scram(password, binding)`. The upstream `ScramSha256::new` is the only public constructor and draws its nonce from `rand::rng()`; building it inside the core would make the core read entropy, breaking this crate's stated sans-I/O guarantee (it "never ... generates entropy") and the host-supplied entropy wiring for wasm32-unknown and WASI p3. The accessor removes the duplicated credential while the entropy read stays in the host, and it also exposes the rest of the effective configuration. The password was already retained inside the connection for cleartext and MD5 authentication, so nothing new is kept. The async client no longer clones the password, and the real-server fixture no longer stores one. The SCRAM protocol test now configures the password only in `Config`, builds `ScramSha256` from `config().password`, and completes both plain and PLUS exchanges whose server signatures are derived from "secret" (and still rejects the corrupted verifier). Closes #57
`abort(reason)` could only report a transport failure as the unit `Error::Transport`, which always displays "Connection terminated unexpectedly". The real cause the host had in hand when it aborted (say `connect ECONNREFUSED 127.0.0.1:5432`) had nowhere to go inside the core's own `Outcome::Aborted` completions, so every host shadowed it in state of its own. `Error::Transport` now carries `Option<TransportFailure>`: the host's `io::ErrorKind`, an optional error code (OS errno or libuv-style code) and a message. `Error::transport(&io::Error)` builds it from an I/O error, keeping `raw_os_error`; `TransportFailure::new` takes the three parts directly. The diagnostic reaches every pending token's `Outcome::Aborted` and the `Closed` reason, displays as "Connection terminated unexpectedly: <message>", and converting to `io::Error` preserves the host's kind instead of flattening it to `ConnectionAborted`. The message is an `Arc<str>`, so, like `ConnectionFailure`, fanning one abort out to a pipeline allocates nothing. `Error::Transport(None)` keeps the old generic message and remains the default reason. Breaking: `Error::Transport` is now a tuple variant; existing `Error::Transport` becomes `Error::Transport(None)`. Tests: a protocol test aborts three pending tokens with an io::Error-derived and an explicitly coded diagnostic and asserts kind, code and message on every completion and the close, plus Display and io::Error conversion; the allocation test fans a coded diagnostic out to 64 tokens with zero allocations. Closes #59
`types::decode` fell through to `Value::Text` for any text-format OID it had
no codec for, the same variant a real text/varchar column produces. A host
matching on the variant, the natural way to consume the API, could not tell
"this is text" from "no codec, here is the string", and silently widened its
own type surface: time, interval, inet, oid, xml or an enum all looked like
text columns.
Text-format values now become `Value::Text` only for the OIDs the binary codec
also treats as text (name 19, text 25, bpchar 1042, varchar 1043); everything
else becomes `Value::Unknown { oid, text }`, the text-format counterpart of the
existing binary `Value::Raw { oid, bytes }`. It stays borrowed and
allocation-free, and `into_owned` covers it.
Breaking: `Value` has a new variant, and text-format values of OIDs without a
codec (including the single-byte `"char"`, OID 18, already `Raw` in binary)
no longer match `Value::Text`.
The new unit test pins the four text OIDs to `Text`, five codec-less OIDs
(time, interval, inet, oid, an extension OID) to `Unknown` with their OID and
text, including through `into_owned`, and binary unknowns to `Raw`. The
real-server conversion test now also rejects `Unknown` for every type it
claims a codec for.
Closes #60
After `ScramNeeded`, `next_event()` returned `Ok(None)` in `State::Scram`,
the same value a host sees when there is simply no input yet. A host that did
not specifically handle `ScramNeeded` therefore stalled silently: no error,
no event, a connection that looks alive but never authenticates.
Pulling in that state now returns `Error::State("SCRAM authentication
pending: answer ScramNeeded with start_scram")`. The error was chosen over
re-emitting `ScramNeeded`: a host that ignores the event would spin forever on
a repeated event, whereas an error ends its pull loop and names the missing
call. The state is left untouched, so a host that pulls early can still call
`start_scram` (or `abort`). Hosts that answer the event before pulling again,
including the bundled async client (the driver returns after each event), are
unaffected.
Behaviour change: a host that collected events in a pull loop and called
`start_scram` only after the loop drained must now answer `ScramNeeded` before
its next pull.
The legacy-TLS SCRAM test now pulls twice before answering, asserts the exact
error both times and that no output was produced, then completes the SCRAM
initial response as before. Without the fix both pulls return Ok(None).
Closes #61
`Connection::new` queues the StartupMessage (or, with TLS, the SSLRequest) before the host has a transport, so `output()` is non-empty at construction. The README described setup as ordered steps (build the connection, then hand it a transport) without saying step 1 already produced output, so a host that assumed `output()` starts empty until it drives the connection would never send those bytes and wait forever for a server reply. The README's first step now states that construction already queues output and that the host must transmit it first once connected. `Connection::new` gains a doc comment saying the same, with a doctest asserting the exact queued bytes: a StartupMessage carrying the `user` parameter, and the 8-byte SSLRequest for `SslMode::Require`. No behaviour changes. Closes #62
|
Warning Review limit reachedNext included review available in 42 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (8)
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 |
This was referenced Sep 22, 2026
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
1 participant
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.
Six fixes to
turnloop-postgresfrom Perry's P7 lane (thepgbinding). One commit per issue.Connection::config()hands the password back whenScramNeededfires, so a host no longer keeps a second copy of the credential.start_scram(password, binding)was rejected:ScramSha256::newis the only public constructor and it reads entropy itself, which would break the crate's rule that the host supplies entropy. The async client no longer clones the password.Event::into_owned() -> OwnedEvent, plusOwnedRow,OwnedFieldsandOwnedServerError. Each is one copy of the wire bytes and lends back the borrowed iterators. The test checks that owned values survive a laterreceiveoverwriting the input buffer.Error::Transport(Option<TransportFailure>)carries the host's diagnostic (kind, code, message) throughabortinto everyOutcome::Aborted/Closed. Converting back toio::Errorkeeps the host's kind. An allocation test confirms spreading it across 64 pending queries allocates nothing.Value::Unknown { oid, text }.Value::Textnow comes only from the text OIDs (19, 25, 1042, 1043).next_event()returnsError::Stateinstead ofOk(None), and the connection can still takestart_scram.Connection::newdocument that construction already queues the StartupMessage or SSLRequest. A doctest pins the exact bytes.Breaking changes
Error::Transportis now a tuple variant (Error::Transport(None)for the old meaning).Value::Unknownvariant: text values of OIDs with no codec, including"char"(OID 18), no longer matchValue::Text.ScramNeededbefore pulling events again.Checks
Local: fmt, workspace clippy, workspace doc, and
cargo test -p turnloop-postgres --all-featuresall pass. The local MSRV check didn't finish because the disk filled up, and the real-server tests (edited for #57/#60) need the CI fixture; CI covers both.Closes #57
Closes #58
Closes #59
Closes #60
Closes #61
Closes #62