Single-pass writing#6580
Open
NinoFloris wants to merge 4 commits into
Open
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
| File | Description |
|---|---|
| test/Npgsql.Tests/WriteStateTests.cs | Updates tests for new PgWriter.Init signature and clarifies write-state comments. |
| test/Npgsql.Tests/Types/CompositeTests.cs | Adds test for composite type containing jsonb. |
| test/Npgsql.Tests/MeasuringPgWriterTests.cs | New unit tests for PgWriter measuring and scope framing/backpatch behavior. |
| test/Npgsql.Tests/MeasuringConverterTests.cs | New integration-ish tests for measuring protocol through parameter Bind/Write orchestration. |
| src/Npgsql/NpgsqlParameter.cs | Changes Bind to return a measured byte count and materializes non-exact values for Bind message sizing. |
| src/Npgsql/Internal/PgWriter.cs | Adds measuring side buffer writer, length-prefixing and passthrough scopes, and capacity-hint support. |
| src/Npgsql/Internal/PgConverter.cs | Removes guards that forbade Unknown/UpperBound sizes from bind returns. |
| src/Npgsql/Internal/NpgsqlWriteBuffer.cs | Updates PgWriter.Init call site for the new capacityHint parameter. |
| src/Npgsql/Internal/NpgsqlConnector.FrontendMessages.cs | Updates Bind message length summation to use byte counts from Bind. |
| src/Npgsql/Internal/Converters/*.cs | Switches nested framing to new scope APIs to enable measuring/backpatching. |
| src/Npgsql/Internal/BufferRequirements.cs | Updates IsBindOptional semantics comment to decouple from write size kind. |
💡 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()); |
Comment on lines
925
to
938
| 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; | ||
| } | ||
| } |
Comment on lines
108
to
+113
| public override ValueTask WriteAsync(PgWriter writer, T? value, CancellationToken cancellationToken = default) | ||
| => JsonConverter.Write(_jsonb, async: true, writer, cancellationToken); | ||
| { | ||
| Debug.Assert(writer.Current.Size == Size.Unknown, "We expected to be measuring, which is safely sync only."); | ||
| Write(writer, value); | ||
| return new(); | ||
| } |
Member
Author
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.
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
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.
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.