Skip to content

Sans-I/O TDS core (4b/N): invert PLP/LOB column streaming to the sync core - #196

Closed
Saurabh Singh (saurabh500) wants to merge 2 commits into
dev/saurabh/sans-io-l4a-fixed-width-stepfrom
dev/saurabh/sans-io-l4b-variable-plp
Closed

Sans-I/O TDS core (4b/N): invert PLP/LOB column streaming to the sync core#196
Saurabh Singh (saurabh500) wants to merge 2 commits into
dev/saurabh/sans-io-l4a-fixed-width-stepfrom
dev/saurabh/sans-io-l4b-variable-plp

Conversation

@saurabh500

@saurabh500 Saurabh Singh (saurabh500) commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

Summary

Layer 4b of the sans-I/O restructure. Inverts the eager PLP/MAX column collect path to run synchronously over the in-memory PacketBuffer, reusing the existing PlpChunkStreamReader resumable state — no new state machine. The async decode_into PLP branch and the buffer-driven sync driver converge on one shared framing body (two refill drivers differing only by .await).

Base: dev/saurabh/sans-io-l4a-fixed-width-step (frozen tip 8bddc4fc).

Related Issues

N/A — internal sans-I/O refactor, no tracking issue (consistent with #189/#191/#194/#195).

What changed

  • Factor byte-source-agnostic framing leaves out of PlpChunkStreamReader (decoder.rs): classify_length, on_chunk_header, note_read, chunk_remaining, reached_end, total_read. The async begin/ensure_active_chunk/read_into are rewritten to call these leaves — single-copy framing, no drift.
  • Sync PLP driver (sync_decoder.rs): plp_collect_step + PlpProgress step the PlpChunkStreamReader over an owned PacketBuffer one chunk at a timeensure(4) for a chunk header or a slice of the current chunk body, never the whole value — so wire residency stays bounded to ≲ one packet regardless of total value size. NeedMore carries the absolute header/body width so refill always makes forward progress across packet-straddling prefixes.
  • Reader seam (packet_reader.rs): collect_plp_bytes — async framing default for non-buffer readers, sync driver override on PacketReader and NetworkTransport. All GenericDecoder::collect_plp call sites rewired through the seam; value arms unchanged → byte-identical.
  • Always-Encrypted over varbinary(max) (token_stream.rs): collect ciphertext via the sync seam then decrypt_cipher_value, folding collect+decrypt into the sync path (mirrors L4a's non-PLP AE fold).

Scope boundary

text/ntext/image are is_plp() == false legacy text-pointer LOBs (not the PLP chunk protocol) and stay on the legacy async path, unchanged and byte-identical — consistent with L4a's boundary. (Coordinator-tracked as a separate p4d follow-up.)

Tests (buffer-driven PacketReader)

  • plp_single_chunk_mid_chunk_resume_is_byte_identical_across_refill_boundary — refill boundary swept across every interior offset incl. inside the 8-byte PLP header and 4-byte chunk-length prefix.
  • plp_multi_chunk_across_refills_is_byte_identical_with_bounded_residency — multi-chunk to the zero-length terminator + a large value exceeding the 8192-byte buffer capacity decoded under packet-sized refills, proving chunk-bounded residency by construction.
  • fully_sync_mixed_row_with_plp_is_byte_identical_across_refill_boundary[int4][varchar][varbinary(max)] with every cell on the sync core, exact values asserted across every refill offset.

Validation

  • cargo btest (nextest) terminates; FAIL name-set byte-for-byte identical to base (the 7 known cert fixtures).
  • cargo bfmt + cargo bclippy clean; scripts/bfmt.ps1 + scripts/bclippy.ps1 clean (mssql-py-core builds).

No PLP framing logic duplicated; no new resumable machine introduced.

Co-authored-by: Copilot App 223556219+Copilot@users.noreply.github.com
Copilot-Session: e2c378f8-3ba1-4b48-9ebe-5a4ea2bd2761

Route the eager PLP/MAX collect path through a synchronous chunk-atomic
driver over the owned PacketBuffer, reusing PlpChunkStreamReader as the sole
resumable state. Factor the byte-source-agnostic framing leaves
(classify_length, on_chunk_header, note_read, chunk_remaining, reached_end,
total_read) out of the async begin/ensure_active_chunk/read_into so both the
async default and the buffer-driven override share one framing body.

Add plp_collect_step over PacketBuffer: ensure one 4-byte chunk header or a
slice of the current chunk body at a time, never the whole value, so wire
residency stays bounded to ~one packet for arbitrarily large columns.
NeedMore carries the absolute header/body width so the driver's ensure()
always makes forward progress across packet-straddling prefixes.

Add collect_plp_bytes to the reader seam: async framing default for
non-buffer readers, sync driver override on PacketReader and NetworkTransport.
Rewire GenericDecoder::collect_plp callers through the seam; value arms are
unchanged, so output is byte-identical. Fold Always-Encrypted-over-
varbinary(max) collect+decrypt into the sync PLP branch in
decode_or_decrypt_column.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: e2c378f8-3ba1-4b48-9ebe-5a4ea2bd2761
Cover the inverted PLP path on the buffer-owning PacketReader:
- single-chunk varbinary(max) with the refill boundary swept across every
  interior offset, including inside the 8-byte PLP header and the 4-byte
  chunk-length prefix, byte-identical to the single-packet baseline;
- multi-chunk value swept across between-chunk and mid-chunk boundaries to the
  zero-length terminator, plus a large multi-chunk value whose total wire size
  exceeds the reader's 8192-byte buffer capacity, decoded byte-identically
  under packet-sized refills to prove chunk-bounded residency (a whole-value
  ensure could never be satisfied and would trip the forward-progress guard);
- a fully-synchronous mixed row [int4][varchar][varbinary(max)] whose every
  cell decodes on the sync core, with exact values asserted across every
  refill offset.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: e2c378f8-3ba1-4b48-9ebe-5a4ea2bd2761
@saurabh500
Saurabh Singh (saurabh500) force-pushed the dev/saurabh/sans-io-l4b-variable-plp branch from d52716f to 149fa6f Compare August 9, 2026 00:22
@saurabh500
Saurabh Singh (saurabh500) marked this pull request as ready for review August 10, 2026 06:38
@saurabh500
Saurabh Singh (saurabh500) requested a review from a team as a code owner August 10, 2026 06:38
Copilot AI balanced review requested due to automatic review settings August 10, 2026 06:38

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Moves eager PLP/LOB collection into the synchronous sans-I/O core while preserving shared framing state.

Changes:

  • Adds synchronous chunk-at-a-time PLP collection.
  • Routes buffer-backed readers through the sync PLP path.
  • Integrates PLP Always Encrypted decoding and boundary tests.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
mssql-tds/src/datatypes/decoder.rs Exposes shared PLP framing operations.
mssql-tds/src/datatypes/sync_decoder.rs Adds synchronous PLP collection steps.
mssql-tds/src/io/packet_reader.rs Adds the PLP collection seam and test-reader override.
mssql-tds/src/connection/transport/network_transport.rs Implements production buffer-driven PLP collection.
mssql-tds/src/io/token_stream.rs Routes PLP decryption and adds refill-boundary tests.
Suppressed comments (1)

mssql-tds/src/io/token_stream.rs:1937

  • This is another copy of the PLP row-decoding helper already added at lines 1656-1675 and 1820-1839. Reuse a module-level test helper instead of adding a third implementation so setup and protocol assertions cannot drift between boundary tests.
        async fn decode(read_data: Vec<u8>, columns: &[ColumnMetadata]) -> Vec<ColumnValues> {

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment on lines +482 to +486
let cipher = match reader.collect_plp_bytes().await? {
Some(bytes) => crate::datatypes::column_values::ColumnValues::Bytes(bytes),
None => crate::datatypes::column_values::ColumnValues::Null,
};
let value = decrypt_cipher_value(meta, dec, cipher)?;
let second = second_builder.append_bytes(&payload[split..]).build();
[first, second].concat()
}
async fn decode_pr(read_data: Vec<u8>, columns: &[ColumnMetadata]) -> Vec<ColumnValues> {
@saurabh500
Saurabh Singh (saurabh500) marked this pull request as draft August 10, 2026 06:49
@saurabh500
Saurabh Singh (saurabh500) marked this pull request as ready for review August 10, 2026 07:39
@saurabh500
Saurabh Singh (saurabh500) marked this pull request as draft August 10, 2026 13:28
@saurabh500

Copy link
Copy Markdown
Contributor Author

Closing: the sans-I/O restructuring is not going to be productionized.

The row-decode performance work that motivated much of this has been re-scoped around #247, where benchmarked spikes show the dominant win comes from a far smaller change — converting TdsPacketReader to RPITIT to remove per-read boxing (#252, measured at −61.6% decode time) — rather than from inverting the core to a sync step() driver. Given that, this stack is a large amount of surface area for a win that is already available more cheaply.

This is cleanup rather than a rejection of the analysis. The branch is deliberately not deleted, so the work remains recoverable if the direction is revisited.

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.

2 participants