GeezerVison

🟡 Changes recommended

Non-ASCII error and notice fields use inconsistent encodings for lengths and payloads, which can stall client reads.

_{Review effort: Balanced}

Recommendation

The core direction is justified. I found no correctness blocker in the exercised paths. I would keep the replication mock and smoke tests, trim unused future-facing helpers, and preserve the important transport fixes with focused regression coverage.

Problem and expected approach

There is a real testability gap: the existing physical replication fixture is explicitly disabled for flakiness, and its streaming tests depend on cluster-wide WAL activity. The shared replication fixture also requires a specially configured live server. A mock cannot replace those integration tests, but it can make client protocol sequencing deterministic.

Independently of this implementation, I would extend the existing wire mock with CopyBothResponse, WAL and keepalive writers, feedback parsing, and a small connection helper that disables automatic feedback. A few hundred lines of C# test-only code is reasonable; no production API or user-documentation change is needed. The essential scenarios are startup/IDENTIFY_SYSTEM, a streamed payload, requested feedback, and cancellation followed by connection reuse.

Alignment

The PR follows that design without adding a separate mocking framework. Its physical smoke test drives the real replication API while scripting the peer. For example, after starting a client MoveNextAsync:

await server.ExpectSimpleQuery("START_REPLICATION PHYSICAL 0/1000000");
await server.WriteCopyBothResponse()
    .WriteXLogData(new(0x1000000), new(0x1000003), "abc"u8)
    .FlushAsync();

The NULL DataRow support is relevant to physical IDENTIFY_SYSTEM. The adjacent length corrections are also legitimate: ParameterDescription previously counted the message-type byte, CopyInResponse declared 5 bytes while emitting 9, and ErrorResponse omitted three string terminators from its declared length. The asynchronous skip fixes consumption of fragmented frontend bodies rather than merely changing style.

Gaps and scope

The new tests do not exercise the changed ExpectMessage skip or the large-payload flushing writer; the inline comments suggest narrow regression cases. Also, CopyOutResponse, raw CopyData/CopyDone writers, and server-version configurability have no consuming tests in this PR. I would defer that unused surface until a concrete scenario needs it rather than treating this as a general protocol-mock expansion. These are coverage/scope recommendations, not demonstrated runtime defects. The smoke tests establish the foundation; they do not yet cover timeline switches, server shutdown, or logical decoding streams.

Manual validation

I built a separate console program against the exact PR mock sources and drove real logical and physical replication connections, independently of NUnit. It verified a 24,593-byte WAL payload, high-bit LSNs, an exact server timestamp, notice handling, all three feedback LSNs, cancellation routing, connection reuse, a NULL database name, and Terminate. A separate raw TCP peer checked the corrected frame lengths, COPY/NULL encodings, a WAL header near the buffer boundary, and a 20 KB frontend body delivered separately from its header, including alignment of subsequent messages. All passed. The two new smoke tests and eight selected existing mock tests also passed. Validation used an extracted source copy and the installed .NET 11 preview SDK because the pinned 10.0.402 SDK was unavailable; no reviewed source was changed.

Posted by copilot

Please see agentic review below, am otherwise approving as this is test infra only.

🟢 Approval recommended

No active review issues were identified in the PostgreSQL replication mock changes.

No open issues 🎉

_{Review effort: Balanced}