Simplify field reads by NinoFloris · Pull Request #5480 · npgsql/npgsql

NinoFloris · 2023-12-06T16:12:23Z

This moves the stream handling into the Ado resolver.

Based on #5476
Any reviews should start from the last commit Simplify by resolving Stream by default as a converter

After this pack of changes (though that ideally includes #5471) we are able to fully inline everything on the hot path into GetFieldValueCore.

DPGO was even happy to make one huge 8000 bytes method (arm64) out of my local reproduction of the fortunes scenario, inlining even GetFieldValueCore itself.

Imprecise local benchmarking shows about a 3% improvement in fortunes RPS.

Copilot

Pull request overview

This PR simplifies field reads in NpgsqlDataReader by removing Stream-specific special-casing and moving Stream handling into the ADO type info resolver/converter pipeline, with additional reader state tweaks to support the new flow and improve inlining/hot-path performance.

Changes:

Replace Stream special-casing in NpgsqlDataReader.GetFieldValue* with resolver-based Stream conversion (StreamByteaConverter).
Introduce PgReader.StreamCanSeek and propagate sequential/non-sequential seekability decisions into PgReader.GetStream().
Cleanup and small refactors: unify -1 null sentinel usage, tighten cleanup/reset behavior, and rename nested-read revert API.

Reviewed changes

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

Show a summary per file

File	Description
src/Npgsql/ThrowHelper.cs	Changes `ThrowIndexOutOfRangeException` formatting overload to take an `int` argument.
src/Npgsql/Replication/PgOutput/ReplicationValue.cs	Refactors replication value reads to use a shared generic `Get<T>(async, ...)` path; exposes sync `GetStream`/`GetTextReader` via blocking.
src/Npgsql/NpgsqlDataReader.cs	Removes Stream fast path from `GetFieldValue*`, introduces `DbNullSentinel`, threads `DataFormat` through `GetInfo`, sets stream seekability via `PgReader.StreamCanSeek`, and resets reader/buffer references on cleanup.
src/Npgsql/Internal/ResolverFactories/AdoTypeInfoResolverFactory.cs	Adds fallback Stream type info resolution and enables Stream converter text-format support for relevant mappings.
src/Npgsql/Internal/PgReader.cs	Adds `StreamCanSeek`, replaces `GetStream(canSeek, ...)` with `GetStream()` using the new flag, and renames nested-scope revert method.
src/Npgsql/Internal/Converters/Primitive/TextConverters.cs	Adjusts `ConsumeChars` end-of-stream condition logic.
src/Npgsql/Internal/Converters/Primitive/ByteaConverters.cs	Implements `StreamByteaConverter` as an actual Stream reader via `PgReader.GetStream()`, with format support controlled by `supportsTextFormat`.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/Npgsql/NpgsqlDataReader.cs

Copilot

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

src/Npgsql/Replication/PgOutput/ReplicationValue.cs:169

ThrowIfInitialized() only checks PgReader.Initialized, but Consume() sets _isConsumed = true after committing (which clears PgReader.Initialized). After Consume() has been called, Get<T>() can be invoked again and will re-Init and read from the wrong position. Consider also checking _isConsumed (or setting a separate flag) and throwing the same "Column has already been consumed" exception when true.

    void ThrowIfInitialized()
    {
        if (PgReader.Initialized)
            throw new InvalidOperationException("Column has already been consumed");
    }

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/Npgsql/Internal/Converters/Primitive/TextConverters.cs

Copilot

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.

Comments suppressed due to low confidence (1)

src/Npgsql/Replication/PgOutput/ReplicationValue.cs:169

ThrowIfInitialized() only checks PgReader.Initialized, but Consume(...) sets _isConsumed = true after committing the reader (so Initialized becomes false). That allows Get* methods to be called again after consumption, potentially reading from an already-consumed buffer position. Add a guard for _isConsumed (or reintroduce the previous active-state check) and ensure the exception message matches the condition.

    void ThrowIfInitialized()
    {
        if (PgReader.Initialized)
            throw new InvalidOperationException("Column has already been consumed");
    }

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/Npgsql/Replication/PgOutput/ReplicationValue.cs

src/Npgsql/NpgsqlDataReader.cs

Copilot

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/Npgsql/Internal/Converters/Primitive/TextConverters.cs

Copilot

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/Npgsql/NpgsqlDataReader.cs

# Conflicts: # src/Npgsql/NpgsqlDataReader.cs

NinoFloris · 2026-03-25T15:45:19Z

src/Npgsql/NpgsqlDataReader.cs

-        if ((uint)ordinal > (uint)ColumnCount)
-            ThrowHelper.ThrowIndexOutOfRangeException("Ordinal must be between 0 and " + (ColumnCount - 1));
+        if ((uint)ordinal >= (uint)ColumnCount)
+            ThrowHelper.ThrowIndexOutOfRangeException("Ordinal is out of range, value must be between 0 and {0} (exclusive).", ColumnCount);


This guard condition change was a copilot suggestion, as we were dropping through to the array indexer bounds check for the last column. To do so the message needed a rework as well (otherwise you'd read e.g. "must be between 0 and 0" for a one column row).

NinoFloris · 2026-03-25T15:45:28Z

src/Npgsql/BackendMessages/RowDescriptionMessage.cs

        get
        {
-            if ((uint)ordinal < (uint)Count)
+            if ((uint)ordinal >= (uint)Count)


This guard condition change was a copilot suggestion, as we were dropping through to the array indexer bounds check for the last column. To do so the message needed a rework as well (otherwise you'd read e.g. "must be between 0 and 0" for a one column row).

NinoFloris · 2026-03-25T15:45:58Z

I've rebased and minimized the diff @roji @vonzshik PTAL.

NinoFloris requested review from Brar, roji and vonzshik as code owners December 6, 2023 16:12

NinoFloris added the enhancement label Dec 6, 2023

NinoFloris force-pushed the simplify-field-reads branch 2 times, most recently from 368cc98 to c9212f3 Compare December 8, 2023 13:26

NinoFloris mentioned this pull request Mar 13, 2024

Further investigate and improve perf for CommandBehavior.SequentialAccess #3453

Closed

NinoFloris force-pushed the simplify-field-reads branch from c9212f3 to a22a718 Compare March 17, 2024 16:17

This was referenced Mar 17, 2024

Rework row reading #5631

Draft

Improve binary exporter perf #5630

Merged

NinoFloris force-pushed the simplify-field-reads branch 2 times, most recently from 5b91717 to cf122e5 Compare March 26, 2024 00:59

NinoFloris force-pushed the simplify-field-reads branch 4 times, most recently from aab2833 to e950992 Compare June 28, 2024 14:52

roji removed the enhancement label Jun 14, 2025

NinoFloris force-pushed the simplify-field-reads branch 2 times, most recently from 5a8a530 to 86af6d4 Compare October 29, 2025 23:56

NinoFloris mentioned this pull request Oct 30, 2025

Add SupportsReading plumbing #5472

Merged

NinoFloris added this to the 10.0.0 milestone Nov 3, 2025

NinoFloris modified the milestones: 10.0.0, 11.0.0 Nov 19, 2025

Copilot AI review requested due to automatic review settings March 12, 2026 06:31

NinoFloris force-pushed the simplify-field-reads branch from 86af6d4 to 078b193 Compare March 12, 2026 06:31

Copilot started reviewing on behalf of NinoFloris March 12, 2026 06:32 View session

Copilot AI reviewed Mar 12, 2026

View reviewed changes

src/Npgsql/NpgsqlDataReader.cs Outdated Show resolved Hide resolved

src/Npgsql/NpgsqlDataReader.cs Show resolved Hide resolved

NinoFloris force-pushed the simplify-field-reads branch 3 times, most recently from 8365094 to dd543b8 Compare March 12, 2026 07:34

Copilot AI review requested due to automatic review settings March 25, 2026 12:06

NinoFloris force-pushed the simplify-field-reads branch from dd543b8 to eadb2f1 Compare March 25, 2026 12:06

Copilot started reviewing on behalf of NinoFloris March 25, 2026 12:07 View session

Copilot AI reviewed Mar 25, 2026

View reviewed changes

src/Npgsql/Internal/Converters/Primitive/TextConverters.cs Outdated Show resolved Hide resolved

NinoFloris mentioned this pull request Mar 25, 2026

Decouple PgReader GetTextReader and GetChars #6490

Draft

Copilot AI review requested due to automatic review settings March 25, 2026 14:06

Copilot started reviewing on behalf of NinoFloris March 25, 2026 14:06 View session

Copilot AI reviewed Mar 25, 2026

View reviewed changes

NinoFloris force-pushed the simplify-field-reads branch from 34efa01 to dcbfdd5 Compare March 25, 2026 14:16

Copilot AI review requested due to automatic review settings March 25, 2026 14:19

Copilot started reviewing on behalf of NinoFloris March 25, 2026 14:19 View session

Copilot AI reviewed Mar 25, 2026

View reviewed changes

src/Npgsql/Internal/Converters/Primitive/TextConverters.cs Outdated Show resolved Hide resolved

Copilot AI review requested due to automatic review settings March 25, 2026 14:37

NinoFloris force-pushed the simplify-field-reads branch from fe41bb8 to 5a9300d Compare March 25, 2026 14:37

Copilot started reviewing on behalf of NinoFloris March 25, 2026 14:38 View session

NinoFloris force-pushed the simplify-field-reads branch from 5a9300d to 6e1cf1e Compare March 25, 2026 14:43

Copilot AI reviewed Mar 25, 2026

View reviewed changes

src/Npgsql/NpgsqlDataReader.cs Show resolved Hide resolved

NinoFloris added 9 commits March 25, 2026 15:52

Simplify by resolving Stream by default as a converter

cc38628

Rename and move StreamByteaConverter

451c91f

Split replication value implementation into sync and async

0bb9d1b

Pull reader into a local

f587a91

# Conflicts: # src/Npgsql/NpgsqlDataReader.cs

Flip args in GetInfo, should save a write barrier

844844d

Improve jitted code size of GetInfo

77c978c

Introduce a db null size const

f4a6ba4

Fix check and test ordinal message

b689a09

Change can seek default to false for replication and other places

6d4e08b

NinoFloris force-pushed the simplify-field-reads branch from 6e1cf1e to 6d4e08b Compare March 25, 2026 14:53

NinoFloris commented Mar 25, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify field reads#5480

Simplify field reads#5480
NinoFloris wants to merge 9 commits intomainfrom
simplify-field-reads

NinoFloris commented Dec 6, 2023 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

NinoFloris Mar 25, 2026

Uh oh!

NinoFloris Mar 25, 2026

Uh oh!

NinoFloris commented Mar 25, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

NinoFloris commented Dec 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

NinoFloris Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

NinoFloris Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

NinoFloris commented Mar 25, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

NinoFloris commented Dec 6, 2023 •

edited

Loading