SqlServerDatabaseProvider should override HasNativeGuidSupport => true

[strangepholker]

I'm using the SqlServerDatabaseProvider with Sql Azure and querying large tables based on various Guid and uniqueidentifier properties and I believe the performance of the query is being negatively impacted because the parameter type in the sql statement is nvarchar(40) rather than uniqueidentifier.

Here's some sample code for how we're constructing the query.

var patientId = Guid.NewGuid();
var query = Sql.Builder;
query
	.Select("*")
	.From($"[medical].[{nameof(DocumentVersion)}] {TableAliases.Version} ")
	.Where($"{TableAliases.Version}.[{nameof(DocumentVersion.PatientId)}] = @0 ", patientId)

This is producing a query where @0 is an nvarchar(40) rather than a uniqueidentifier.

Does anyone see a reason I shouldn't throw together a PR to implement an override for HasNativeGuidSupport in the SqlServerDatabaseProvider?

[asherber]

It looks like Guid support has been in SQL Server since 2008, which is older than PetaPoco. I'm guessing it may have been an oversight that the SqlServerDatabaseProvider doesn't override this to return true. Fixing that now would potentially be a breaking change, however.

[strangepholker]

Given that Guid support has been present in SQL Server for longer than PetaPoco has existed it seems like a fairly safe switch to make. It's also one that's pretty straight forward to validate.

Having said that, if you feel the concern about introducing a breaking change here outweighs the benefits of SQL performing implicit data type conversions then I would propose introducing a new DatabaseProvider (perhaps SqlServer2008DataBaseProvider?) and marking the existing SqlServerDatabaseProvider as obsolete with an appropriate message.

[asherber]

I think the questions to consider are:

• Is it possible/likely that users of PetaPoco have been relying on Guids being treated as strings for Sql Server, or that if they were suddenly treated as native Guids there would be unexpected failures in user code? If so, then I don't think we can just change the current behavior. (You're right that it's easy to test/validate the happy path, but I'm not sure that we can anticipate all the ways that PP is being used in the wild.)
• If we introduce a new DatabaseProvider, does it become the default? Note that because of the several ways that Database can be instantiated, making the new provider the default for Sql Server would have the same effect as just changing the behavior of HasNativeGuidSupport for many users.
• If we don't make the new provider the default, then users would have to opt in to what is really the "correct" behavior, which is unfortunate.

[asherber]

Note that for your own uses, a short term solution is for you to create your own SqlServerDatabaseProvider descendant, changing HasNativeGuidSupport, and then use the Database constructor that takes an IProvider instance. Or you could package this provider in a separate assembly as a (custom provider)[https://github.com/CollaboratingPlatypus/PetaPoco/wiki/Custom-DB-Providers], if you want to be able to use it in multiple solutions.

A custom provider could also be made available on NuGet, which might give users an easier way of opting into this behavior.

[iadaz]

I am ashamed to say that some software I wrote ten years ago (when I was young and stupid) stores GUIDs in a varchar field. Of course, this predates Petapoco, but I think it's possible there are other crazies around.

I don't think you should have to cater for idiots like me at the expense of good practice, but it shouldn't be dropped in to the current version. It ought to target the next major version and have an accompanying 'breaking changes' document associated with it. Anyone who decides to update their package through a major version should be expecting to have some work to do.

The provider approach would seem sensible for @strangepholker's case.

[strangepholker]

Thanks Aaron, I think the CustomDbProvider is the approach that our team will take in the short to medium term.

We're still in the early stages of converting our queries from EF to PetaPoco and we've only done so in a couple places so I've already addressed our issue in prod by updating the queries we've written to new up SqlParameters and specify the SqlDbType property as SqlDbType.UniqueIdentifier.

Regarding your previous post about the questions to consider for PetaPoco adoption. I think it's a pretty reasonable assumption that most people who are using Guids with Sql Server are neither relying on nor benefiting from the implicit conversions between Guids and strings. [Edit: Open mouth, insert foot. Sorry @iadaz! It is very much worth considering schemas that have migrated from pre-SqlServer 2008 as well without having their tables altered as well!]

I certainly understand the reluctance to introduce a potentially breaking changes. Again, anyone using Guid data types in their application code and using Sql Server 2005 or earlier would experience this as a breaking change. For that reason, I don't think you would want any new database provider would become the default. I think I would lean towards supporting the legacy behavior by default, but marking the existing provider as obsolete with a message referring people to the new provider and a one line sentence about native Guid support. That would at least aid in the discoverability of the new IProvider.

Personally, I don't like the idea of releasing a Nuget package with a custom IProvider for a well known DBMS that has native support in PetaPoco. As a consumer, of PetaPoco I would find the discovery of that to be much more challenging than a new IProvider within PetaPoco and I would tend to be less trusting of something that is an extension of the project rather than a part of the core project, but maybe that's just my personal bias.

Thanks again, I'm curious to hear what you feel about it.

[asherber]

My thought about the NuGet package was that it was easier from a code perspective. If PetaPoco adds a new SqlServer provider but leaves the old one as the default, then users would have to use either the fluent configuration syntax or the Database constructor that takes an IProvider instance. With a NuGet package, you could register the "custom" provider as the default for Sql Server connection strings with one line of code at startup, and then you could use whatever constructor you want.

But it occurs to me that even if a "new" provider is available within PetaPoco itself, you could still use RegisterCustomProvider() to register that provider as the default for Sql Server; custom registrations are evaluated before the built-in PetaPoco logic when choosing a provider. I haven't tested this yet, but it should work. In this case, I agree that there's no advantage to a NuGet package.