Fix sequential buffered seek

[NinoFloris]

Fixes #5439 and likely also #5430

Besides there being a real bug here of my own making (first commit) this 'cargo culted' for loop is also dangerous when side-effects follow the read into the column.

Consider:

for (; _column < ordinal - 1; _column++)
{
    await buffer.Ensure(4, async).ConfigureAwait(false);
    var len = buffer.ReadInt32();
    if (len != -1)
        await buffer.Skip(len, async).ConfigureAwait(false);
}

Now if buffer.Skip ends up throwing an IO exception we'll end up with the same bug (not incrementing _column while having read into it). That will then cause protocol desync during dispose.

Second commit changes that for that particular bit of code.

[vonzshik]

Now if buffer.Skip ends up throwing an IO exception we'll end up with the same bug (not incrementing _column while having read into it). That will then cause protocol desync during dispose.

I do not really understand this part since we explicitly say that any IO errors while reading columns in a row are non-recoverable.

npgsql/src/Npgsql/NpgsqlDataReader.cs

Lines 1550 to 1552 in 7b515c7

	async ValueTask<T> Core(int ordinal, CancellationToken cancellationToken)
	{
	using var registration = Connector.StartNestedCancellableOperation(cancellationToken, attemptPgCancellation: false);

The issue here is that in 7.0 it was done for both sync and async, and that's what we've lost.

npgsql/src/Npgsql/NpgsqlDataReader.cs

Lines 1745 to 1747 in eff6b18

	async ValueTask<T> GetFieldValueSequential<T>(int column, bool async, CancellationToken cancellationToken = default)
	{
	using var registration = Connector.StartNestedCancellableOperation(cancellationToken, attemptPgCancellation: false);

Thinking a bit more, any error at NpgsqlReadBuffer's level is treated as critical and immediately breaks the connection anyway. So even with the cancellation enabled we still should have exited gracefully.

[NinoFloris]

I do not really understand this part since we explicitly say that any IO errors while reading columns in a row are non-recoverable.
Thinking a bit more, any error at NpgsqlReadBuffer's level is treated as critical and immediately breaks the connection anyway. So even with the cancellation enabled we still should have exited gracefully.

Sure that's fair. Though when perf cost is negligible (we only end up here if TrySeekBuffered failed) I'm of the opinion we should keep state valid.

In general it's something I want to improve in 9.0, mostly to improve multiplexing stability. We need more localized error handling (per command for a start) to reduce the cases where we have to flush the entire pipeline due to protocol sync loss.

[vonzshik]

Sure that's fair. Though when perf cost is negligible (we only end up here if TrySeekBuffered failed) I'm of the opinion we should keep state valid.

I'm just pointing out that there is not a lot of reasons to actually try to do that. Any IO error while reading columns means that we've probably lost physical connection with the database (DataRow includes it's own length, which does indicate that PG either has everything in memory already or accessing that data is quite cheap).

In general it's something I want to improve in 9.0, mostly to improve multiplexing stability. We need more localized error handling (per command for a start) to reduce the cases where we have to flush the entire pipeline due to protocol sync loss.

Protocol desync (if you mean getting unexpected messages) shouldn't happen, ever. You can't really guard against them, at that point pretty much anything can happen (not to the extent of OOM but relatively close). The best we can do is to improve our diagnostics via asserts/switches to help us determine the reason.

[NinoFloris]

Any IO error while reading columns means that we've probably lost physical connection with the database

Yes and I think we should do better at recovering from them.

Protocol desync (if you mean getting unexpected messages) shouldn't happen, ever

With protocol desync I mean everything that causes Npgsql to think we're *somewhere* while we're actually off position.

E.g. interpreting something as a message while it's column data or in this case reading an int worth of bytes of column data interpreted as the column length.

Either way let me lead by saying that I'm not married to improving this particular instance in this particular way.

I also know it's only one instance of many, so for that reason I'll take it out of this PR. I will want to improve this more holistically in 9.0 though.

To improve our reliability for multiplexing we require accurate state to be able to recover our sync, including under intermittent failure conditions. If we think we're at the start of a column while we're not, that's bad.

The overall idea is that in such cases you should be able to conclude: this particular reader/command wasn't successful due to an IO exception but let's get our connection state back to a known good point and get on with the next one.

To make progress on multiplexing recoverability we have to stop breaking the entire connection for:

• Intermittent - recoverable - network errors
• Command scoped failures (bad sql, incorrect param type etc)

The former is probably harder than the latter and more complex but IMO still worthwhile.

[roji]

@NinoFloris I'm also trying to understand which error situations exactly you'd like us to handle better...

Like @vonzshik wrote, in my mind protocol sync loss (which is not IO errors) is an Npgsql bug (i.e. developer error) and should never happen; I also think of it like trying to recover from a stack overflow, or null reference or whatever.

IO errors are slightly different, but AFAIK they typically cause the socket to become truly unusable (in which case this whole conversation is moot). One exception to that is a timeout exception; the data could come later, so we could conceivably remember that we need to skip it first, and attempt to continue using the socket. But with our soft timeout/cancellation mechanism, timeout very probably indicates a network partition, so again not super interesting.

Can you give a scenario or two which you have in mind, which we'd be recovering from?

[roji]

(of course, no particular objection to improving general state tracking etc.)

[NinoFloris]

Indeed protocol sync loss is not IO errors, however we have a good amount of cases where the latter can cause the former.

If you take a look at the sample I put in my initial post, any IO error in Skip will cause us to lose protocol sync, as it corrupts our reader state. We have read 4 bytes that we will now forget due to not incrementing _column, next time we'll be lost.

Now today we'll have broken the connection anyway, so no harm done. In the future though I'd like to be able to handle these IO errors more locally and break just the current command/batch, letting the connection try to get to the next command.

This can still fail of course and we'll have to kill the connection but only once we get for instance an ECONNRESET, or an EOF.

Our status quo however means that if we would even want to start to try to recover from transient IO exceptions we would have incorrect state in a lot of places.

[roji]

If you take a look at the sample I put in my initial post, any IO error in Skip will cause us to lose protocol sync, as it corrupts our reader state. We have read 4 bytes that we will now forget due to not incrementing _column, next time we'll be lost.

Sure, so this is just a bug that leads to sync loss, rather than trying to deal/recover with sync loss.

In the future though I'd like to be able to handle these IO errors more locally and break just the current command/batch, letting the connection try to get to the next command.

That's exactly where I'm trying to better understand what you have in mind. But I guess it's not specifically relevant to this PR.

[vonzshik]

Now today we'll have broken the connection anyway, so no harm done. In the future though I'd like to be able to handle these IO errors more locally and break just the current command/batch, letting the connection try to get to the next command.

Thinking about that, one way to implement this is by having some sort of 2 buffer system. The first one operates on the socket and only gives outside messages as ReadOnlySpan<byte> (or a part of them for sequential reads, but the main point is that user code has to explicitly request to move on to next message). And the second buffer operates on the message itself.

[NinoFloris]

Sure, so this is just a bug that leads to sync loss, rather than trying to deal/recover with sync loss.

Exactly.

Thinking about that, one way to implement this is by having some sort of 2 buffer system. The first one operates on the socket and only gives outside messages as ReadOnlySpan (or a part of them for sequential reads, but the main point is that user code has to explicitly request to move on to next message). And the second buffer operates on the message itself.

Yup, I don't think it's strictly necessary but I did have a design somewhat like this in Slon https://github.com/NinoFloris/Slon/blob/main/Slon/Protocol/PgV3/PgV3Protocol.cs#L406

[NinoFloris]

Backported to 8.0.1 via 8c182c0