Skip to content

Initial implementation of inner DbDataReader #3604

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
roji merged 7 commits into from
Apr 14, 2021
Merged

Initial implementation of inner DbDataReader #3604

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
194187d
Initial implementation of inner DbDataReader
Emill Mar 16, 2021
3b1a982
Fix empty array in NpgsqlRecordDataReader
Emill Mar 18, 2021
ea42e9e
Enhance and optimize NpgsqlNestedDataReader
Emill Mar 21, 2021
55212da
Make NpgsqlNestedDataReader tests pass
Emill Mar 21, 2021
037c8d0
Implement GetBytes in NpgsqlNestedDataReader and some cleanups
Emill Mar 22, 2021
76af114
NpgsqlNestedDataReader: address some review comments
Emill Apr 11, 2021
d3e4edc
Add support for column names for composite types in nested data reader
Emill Apr 13, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 57 additions & 2 deletions src/Npgsql/NpgsqlDataReader.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ public sealed class NpgsqlDataReader : DbDataReader, IDbColumnSchemaGenerator

/// <summary>
/// The position in the buffer at which the current data row message ends.
/// Used only when the row is consumed non-sequentially.
/// Used only when the row is consumed non-sequentially.
/// </summary>
int _dataMsgEnd;

Expand Down Expand Up @@ -140,6 +140,14 @@ bool CanConsumeRowNonSequentially
/// </summary>
char[]? _tempCharBuf;

/// <summary>
/// Used to keep track of every unique row this reader object ever traverses.
/// This is used to detect whether nested DbDataReaders are still valid.
/// </summary>
internal ulong UniqueRowId;

@roji roji Mar 19, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Take a look at how manage this problem for GetStream/GetTextReader - NpgsqlDataReader has a _columnStream which gets set when GetStream is called, and disposed when moving to the next row.

I don't think there's a particular advantage in any of the two techniques, but we should unify and have just one mechanism for this lifecycle management.

@Emill Emill Mar 19, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that an outer NpgsqlDataReader could be referenced by multiple inner data readers, and multiple levels of inner data readers. I also want this to work as good as possible in case we recycle data readers. So not sure how the stream approach would work in this case.

@roji roji Mar 19, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right - since we need to handle multiple "dependents", the stream approach doesn't work. But then we can convert the stream to look at the new UniqueRowId mechanism.

@roji roji Mar 19, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW that's also maybe a thing to consider... NpgsqlDataReader currently doesn't support multiple streams even when in non-sequential mode (where there's no reason not too). IIRC this was intentional - I wanted users to not depend on the ability to have multiple ones, so that if they switch to sequential (which generally makes a lot of sense where column streams are used!) they have no issues.

The same logic may apply for the inner data reader, i.e. we may want to restrict to only one active at a given time, just like stream. I doubt this would be restrictive for actual users. What do you think?

@Emill Emill Mar 19, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "stream approach" wouldn't work for the multiple depth case I think, since you have multiple inner data readers then. If there is a simple way to support multiple streams, I would say, why restrict it just because we haven't seen a good use case so far? But to be consistent with sequential mode, maybe we should still restrict it... I'm not sure.

@roji roji Apr 8, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it's a bit trickier... I think we need to make sure there's no more than one nested reader open for a given reader (nested or top-level) - that should provide the right protection for sequential. BTW this protection also applies between streams and readers - you can't have both a stream and a reader open at the same time in sequential.

As to restricting in non-sequential as well... I thought about it again, and I guess it's not a problem if working code starts throwing when you switch on sequential - after all this is already what happens if you're not accessing fields in order; sequential simply does impose further constraints on how you interact with the reader. So I'd be fine with removing all restrictions for non-sequential (including the existing stream one).

@Emill Emill Apr 11, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok so can we leave "allow multiple streams to be opened for non-sequential mode on a particular row" for a separate PR?

@roji roji Apr 12, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure!


internal NpgsqlNestedDataReader? CachedFreeNestedDataReader;

@roji roji Apr 8, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: CachedNestedDataReader?

@Emill Emill Apr 11, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought it was a good idea to signify that the reader is free.

@roji roji Apr 12, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK by me


static readonly NpgsqlLogger Log = NpgsqlLogManager.CreateLogger(nameof(NpgsqlDataReader));

internal NpgsqlDataReader(NpgsqlConnector connector)
Expand Down Expand Up @@ -175,6 +183,7 @@ public override bool Read()
{
CheckClosedOrDisposed();

UniqueRowId++;

@vonzshik vonzshik Apr 14, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason why we do this only for sync Read?

@Emill Emill Apr 14, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No :/
My mistake

var fastRead = TryFastRead();
return fastRead.HasValue
? fastRead.Value
Expand Down Expand Up @@ -1188,6 +1197,50 @@ public override int GetValues(object[] values)
/// <returns>The value of the specified column.</returns>
public NpgsqlDateTime GetTimeStamp(int ordinal) => GetFieldValue<NpgsqlDateTime>(ordinal);

/// <inheritdoc />
protected override DbDataReader GetDbDataReader(int ordinal) => GetData(ordinal);

/// <summary>
/// Returns a nested data reader for the requested column.
/// The column type must be a record or a to Npgsql known composite type, or an array thereof.
/// Currently only supported in non-sequential mode.
/// </summary>
/// <param name="ordinal">The zero-based column ordinal.</param>
/// <returns>A data reader.</returns>
public new NpgsqlNestedDataReader GetData(int ordinal)
{
var field = CheckRowAndGetField(ordinal);
var type = field.PostgresType;
var isArray = type is PostgresArrayType;
var elementType = isArray ? ((PostgresArrayType)type).Element : type;
var compositeType = elementType as PostgresCompositeType;
if (elementType.InternalName != "record" && compositeType == null)
throw new InvalidCastException("GetData() not supported for type " + field.TypeDisplayName);

SeekToColumn(ordinal, false).GetAwaiter().GetResult();
if (ColumnLen == -1)
ThrowHelper.ThrowInvalidCastException_NoValue(field);

if (_isSequential)
throw new NotSupportedException("GetData() not supported in sequential mode.");

var reader = CachedFreeNestedDataReader;
if (reader != null)
{
CachedFreeNestedDataReader = null;
reader.Init(UniqueRowId, compositeType);
}
else
{
reader = new NpgsqlNestedDataReader(this, null, UniqueRowId, 1, compositeType);
}
if (isArray)
reader.InitArray();
else
reader.InitSingleRow();
return reader;
}

#endregion

#region Special binary getters
Expand Down Expand Up @@ -2080,6 +2133,8 @@ Task ConsumeRow(bool async)
{
Debug.Assert(State == ReaderState.InResult || State == ReaderState.BeforeResult);

UniqueRowId++;

if (!CanConsumeRowNonSequentially)
return ConsumeRowSequential(async);

Expand Down Expand Up @@ -2204,7 +2259,7 @@ void CheckClosedOrDisposed()
throw new InvalidOperationException("The reader is closed");
case ReaderState.Disposed:
throw new ObjectDisposedException(nameof(NpgsqlDataReader));
}
}
}

#endregion
Expand Down
Loading