Cleanups surfaced by row decode perf work (PLP validation, cfg-gated duplication, sentinels)

[saurabh500]

Sub-issue of #247. These surfaced while prototyping the performance work but are independent of it — none require the perf changes to land first, and each is small. Grouped into one issue because individually they are too small to track separately.

Problem statement

Six pieces of duplication and near-duplication in the row decode path. Several of them actively increased the cost or risk of the perf spikes in #247.

Proposed solution

• Extract a shared PlpChunkValidator.

  ensure_active_chunk (mssql-tds/src/datatypes/decoder.rs:242) performs 6 validation checks: chunk count, chunk size, overflow, MAX_PLP_SIZE, exceeds-declared-length, and ended-before-declared-length.

  PR POC: reduce per-row cost in the TDS row decode path #238's validate_plp_chunk reimplements only 2 of those 6 and re-declares the constants locally. The Add RowWriter reserve/commit accumulator API to remove double/triple buffering (row decode perf, axis: value handoff) #253 spike's stream_plp_into now duplicates all 6 a third time.

  A sync PlpChunkValidator { declared, total_read, chunks_seen } with accept_chunk(&mut self, len: usize) -> TdsResult<bool> would serve the async path, the streaming path, and any future sans-I/O path from one implementation.

  Checks "exceeds declared" and "ended before declared" are load-bearing given that LenHint::Exact is trusted downstream by Add RowWriter reserve/commit accumulator API to remove double/triple buffering (row decode perf, axis: value handoff) #253.

  Constants for reference: MAX_PLP_SIZE = 64 KiB fuzzing / i32::MAX; MAX_PLP_CHUNKS = 1000 / 100_000; MAX_PLP_CHUNK_SIZE = 8 KiB / 16 MiB.

• Intern strings as Rc<str> / Arc<str>.

  mssql-js/src/binary_row_writer.rs:82 does string_map.insert(s.clone(), idx) — a full String clone used only as a dedup-map key. This is the last remaining string allocation after Add RowWriter reserve/commit accumulator API to remove double/triple buffering (row decode perf, axis: value handoff) #253 lands.

• Avoid columns.to_vec() per paused row.

  read_row_header in token_stream.rs clones the entire Vec<ColumnMetadata>, including every column_name: String, on each pause. Substantially more expensive than the 6-byte bitmap Keep the NBCROW null bitmap on the stack (row decode perf, axis: precomputation) #255 removes — though it is on the cursor/pause path rather than the bulk-scan path, so it does not show up in the POC: reduce per-row cost in the TDS row decode path #238 benchmark at all.

• Collapse the cfg-gated trait/struct duplication.

  packet_reader.rs defines TdsPacketReader twice with identical bodies (pub(crate) vs pub, cfg-gated). Same pattern for ColumnPolicy, RowReadResult, and RowPauseState in token_stream.rs.

  This doubled the edit cost of both Convert TdsPacketReader to RPITIT to remove per-read boxing (row decode perf, axis: dispatch) #252 and Precompute a per-column decode plan from COLMETADATA (row decode perf, axis: precomputation) #254 and is a standing correctness hazard — the two copies can silently drift. Open question: whether macro_rules! can collapse them without breaking the fuzz targets.

• Replace raw 0xFFFF literals with LENGTH_NULL.

  packet_reader.rs:9 already defines pub const LENGTH_NULL: u16 = 0xffff, yet decoder.rs contained 22 raw literals. The Remove #[async_trait] from SqlTypeDecode (row decode perf, axis: dispatch) #251/Add RowWriter reserve/commit accumulator API to remove double/triple buffering (row decode perf, axis: value handoff) #253 spikes converted two; the rest remain.

• Consolidate PLP sentinels, currently declared in three places: decoder.rs:490, sqltypes.rs:146, tds_value_serializer.rs:26.

Affected crate

mssql-tds

Alternatives considered

Doing these opportunistically inside the perf PRs. Rejected — it would inflate diffs on changes that are already wide, and make the perf review harder. They should land separately, ideally before the perf work where they reduce its cost (the trait duplication in particular).

Additional context

Also worth noting for anyone in this area: SqlTypeDecode::decode and decode_into are two complete ~40-arm type switches that must be kept in agreement by hand. #254 risks adding a third. That is called out in #254 rather than here because the resolution is a design decision for that issue, not a mechanical cleanup.

[saurabh500]

One more cleanup surfaced by the benchmarking pass

Benchmarking the spikes (results on #247) turned up an API item that belongs here:

Keep write_str/write_blob alongside the reserve/commit_value accumulator. #253's measurements show the accumulator API costs DefaultRowWriter +3% to +13% while buying contiguous-buffer writers −14% to −30%. The cause is structural: a writer producing Vec<ColumnValues> needs each value in its own allocation regardless, and reserve must zero-fill what it returns, so the accumulator adds a zero-fill on top of the allocation and copy that bytes.to_vec() already did.

Retaining the borrowed-slice methods as a fast path costs nothing — the decoder already has the assembled bytes in the non-chunked case — and lets each writer pick the shape that suits it. Without it, every native Vec<ColumnValues> consumer pays a regression so that the FFI consumers can gain. Details and numbers in the #253 comment.

Also worth folding in, from the same pass: DiscardRowWriter cannot stay zero-sized under a pull model (it needs a scratch Vec to hand out from reserve). A fn wants_values(&self) -> bool { true } hook on the trait would let the decoder skip the value machinery entirely for discarding writers, restoring the zero-sized property and speeding up SkipAll-style paths.

[saurabh500]

Porting two items here from #262 and #261, which duplicated parts of this issue and are being closed.

PLP validation: the six checks, and why two of them are load-bearing

PlpChunkStreamReader::ensure_active_chunk (decoder.rs:242-307) performs six distinct checks:

1. terminator handling, plus "chunk stream ended before declared length"
2. chunks_seen > MAX_PLP_CHUNKS
3. chunk_len > MAX_PLP_CHUNK_SIZE
4. checked_add overflow on the running total
5. next_total > MAX_PLP_SIZE
6. next_total > declared

The PoC's validate_plp_chunk keeps only 2 and 3, and re-declares the limit constants locally.

Checks 1 and 6 matter beyond hygiene. Together they are what makes a declared PLP length trustworthy: the stream is guaranteed neither to end early nor to overrun what it declared. The RowWriter accumulator in #253 proposes that LenHint::Exact be trusted by implementors for pre-sizing a destination buffer, which is only sound if these two checks are guaranteed to run. That is the actual coupling between these two issues.

Proposed shape, so both the async path and any future buffered path share one implementation rather than drifting:

struct PlpChunkValidator {
    declared: u64,
    total_read: u64,
    chunks_seen: usize,
}

impl PlpChunkValidator {
    fn new(declared: u64) -> Self;
    /// `Ok(true)` to continue reading, `Ok(false)` at a clean end of stream.
    fn accept_chunk(&mut self, chunk_len: u32) -> TdsResult<bool>;
    /// Enforces "ended before declared length".
    fn finish(&self) -> TdsResult<()>;
}

Sync and allocation-free on purpose: validation is pure arithmetic over header bytes that have already been read, and keeping it out of async is exactly what makes it shareable and unit-testable without standing up a reader. Relevant to the 85% diff-coverage gate, which is otherwise awkward to hit on this code.

Limits should stay centralized rather than re-declared per call site:

Constant	Normal	cfg(fuzzing)
MAX_PLP_SIZE	i32::MAX	64 KB
MAX_PLP_CHUNKS	100,000	1,000
MAX_PLP_CHUNK_SIZE	16 MiB	8 KB

cfg-gated duplication: a concrete trap in TdsPacketReader

Relevant to the cfg-gated duplication item in this issue's title. TdsPacketReader is defined twice, at io/packet_reader.rs:13 under cfg(not(fuzzing)) and at :46 under cfg(fuzzing), with a Box<dyn ...> blanket impl at :76. Any method added to that trait must be added in all three places or the fuzzing build breaks, and a normal cargo build will not tell you.

#261 proposed adding sync buffered-read primitives (buffered_slice, consume_buffered, try_read_byte) there. That is only useful to the deferred Buffering axis, so it is closed rather than folded in, but the duplication trap it surfaced is worth fixing here regardless of whether those methods ever arrive.