Repository navigation
Single-pass writing - #6580
Single-pass writing#6580NinoFloris wants to merge 4 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Implements the first iteration of “single-pass writing” by introducing measuring-capable write scopes in PgWriter and updating parameter binding/writing to support measured byte counts (particularly for JSON), along with targeted test coverage.
Changes:
- Add measuring write scopes to
PgWriter(including backpatching and variable-length “chained” prefix support). - Update
NpgsqlParameter.Bind/Bind message sizing to support measured byte counts via materialization. - Add new tests covering measuring behavior and JSON-in-composite scenarios.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 5 comments.
Show a summary per file
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| => BeginLengthPrefixingScope(async: true, bufferRequirement, size, state, cancellationToken); | ||
|
|
||
| public NestedWriteScope BeginLengthPrefixingScope(Size bufferRequirement, Size size, object? state, Action<PgWriter, int> prefixingAction) | ||
| => BeginLengthPrefixingScope(async: true, bufferRequirement, size, state, prefixingAction, CancellationToken.None).Result; |
| MeasuringSideBufferWriter Materialize(PgConcreteTypeInfo concrete) | ||
| { | ||
| var buffer = new MeasuringSideBufferWriter(); | ||
| var measureWriter = new PgWriter(buffer).Init(concrete.Options.DatabaseInfo, _binding.Size.GetValueOrDefault().GetValueOrDefault()); |
| public void Dispose() | ||
| { | ||
| if (_writer is null) | ||
| return; | ||
| switch (_placeholderOffset) | ||
| { | ||
| case ChainedScope: | ||
| _writer.EndVariableLengthPrefixingScope(async: false, CancellationToken.None).GetAwaiter().GetResult(); | ||
| break; | ||
| default: | ||
| _writer.EndLengthPrefixingScope(_placeholderOffset); | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
We may be able to throw here if I can guarantee that WriteAsync isn't ever used for measuring conversions. The complexity isn't so much in the current code but in the code where we may only materialize a few elements, as those writes may later be mixed with normal converter writes that should be done async if the caller was. We'd have to make the envelope that contains the raw bytes sync only to prevent easy logic issues where we'd just call WriteAsync because the caller did too.
First version of single-pass writing. Which would fix #1727
For now this just gets implemented for JsonConverter (and the composing converters) as it unconditionally requires measuring. This helps in e.g. arrays where currently one measuring element poisons the entire array to require measuring too, even if it was the only element.
For conditional cases like strings we want to figure out two things:
Furthermore this PR still requires benchmarks and perf tuning (and maybe some more cleanup work) but I wanted to get the shape out to show what it means to implement this.