Sync SqlDataRecord

[Wraith2]

This is a small one. Identify similar parts of the two SqlDataRecord classes from netcore and netfx and sync those differences so the functional differences can be seen.

Observations:

1. There are only two constructors for SqlDataRecord and both of them set the _recordBuffer to a non-null value. This means that the frequent calls to EnsureSubclassOverride which checks _recordBuffer is non-null or throws are pointless and I have seen them in profile traces in the past when looking at UDT performance. I propose to remove EnsureSubclassOverride entirely.
2. All the private fields can be made readonly
3. ThrowIfInvalidOrdinal(ordinal) is used only once in IsDBNull, either it should be used in all functions that take an ordinal parameter or none of them. If it is not used ordinal will often be used without further checks in called methods which could result in out of range or key not found exceptions.
4. The functional differences between these two implementations seem to fall into two categories, in-proc hosting and old sql versions. The in-proc code could be kept in case it is needed in future and the old sql version checks could be dropped after checking version compatibility for this library.

Any of those changes would be a separate PR.

Oh. and the docs state that "Represents a single row of data and its metadata. This class cannot be inherited." when the class is not sealed and can be inherited from. Either the docs are wrong or the class should be sealed.

[cheenamalhotra]

Oh. and the docs state that "Represents a single row of data and its metadata. This class cannot be inherited." when the class is not sealed and can be inherited from. Either the docs are wrong or the class should be sealed.

The original class in Microsoft.SqlServer.Server namespace also is not "sealed" so it does raise confusions. I would leave it as-is for now.

There are only two constructors for SqlDataRecord and both of them set the _recordBuffer to a non-null value. This means that the frequent calls to EnsureSubclassOverride which checks _recordBuffer is non-null or throws are pointless and I have seen them in profile traces in the past when looking at UDT performance. I propose to remove EnsureSubclassOverride entirely.

Agreed. Also the other constructor (internal) does not seem to be used and is dead code - can be removed.

All the private fields can be made readonly

Go ahead, except "_fieldNameLookup" I see others are initialized in constructor so it's possible.

ThrowIfInvalidOrdinal(ordinal) is used only once in IsDBNull, either it should be used in all functions that take an ordinal parameter or none of them. If it is not used ordinal will often be used without further checks in called methods which could result in out of range or key not found exceptions.

I agree it should be checked for in all functions fetching for ordinal.

The functional differences between these two implementations seem to fall into two categories, in-proc hosting and old sql versions. The in-proc code could be kept in case it is needed in future and the old sql version checks could be dropped after checking version compatibility for this library.

SQL Server 2012 is the minimum official supported version as per Support Lifecycle. But we have customers still using SQL 2008 so we try not to break them. But checks to support below SQL 2005 can be considered for removal.

[Wraith2]

Ok, as stated I'll make those changes in a follow-up unless you strongly prefer I do them here.

[DavoudEshtehari]

Oh. and the docs state that "Represents a single row of data and its metadata. This class cannot be inherited." when the class is not sealed and can be inherited from. Either the docs are wrong or the class should be sealed.

According to documents, nobody's allowed to inherit this class; but the design doesn't prevent it. So, there is a chance of inheriting from this class by customers!
@David-Engel Could you specify your preference here, please?

Edited
Cheena agrees with the current design as she mentioned.
Shouldn't docs be updated?

[David-Engel]

Oh. and the docs state that "Represents a single row of data and its metadata. This class cannot be inherited." when the class is not sealed and can be inherited from. Either the docs are wrong or the class should be sealed.

According to documents, nobody's allowed to inherit this class; but the design doesn't prevent it. So, there is a chance of inheriting from this class by customers!
@David-Engel David Engel (Simba Technologies Inc) Vendor Could you specify your preference here, please?

Docs should be updated to reflect reality. That doesn't need to be a part of this PR, though.

[cheenamalhotra]

@Wraith2

Docs should be updated to reflect reality.

Could you please update docs as well in this PR, to close on this topic and address one last comment from Davoud?
It'll be better to get all changes addressed together IMO.

[Wraith2]

Docs updated to remove "This class cannot be inherited."
Everything else will get looked at in another pass over these files.