markrendle / Simple.Data

A light-weight, dynamic data access component for C# 4.0
MIT License
1.33k stars 303 forks source link

Insert() returns null on Simple.Data.SqlServer when PK isn't an autoincrementing int #370

Open jrnail23 opened 8 years ago

jrnail23 commented 8 years ago

Here's the reproduction (w/ SQL Server 2012 Express, using Simple.Data.SqlServer):

-- my table definition:
CREATE TABLE [dbo].[Example](
    [ID] [uniqueidentifier] NOT NULL,
    [Name] [varchar](50) NOT NULL,
        CONSTRAINT [PK_Example] PRIMARY KEY CLUSTERED ([ID] ASC))
var example = db.Example.Insert(Name: "some value");

In this case, example always ends up null, unless I change the ID column (the primary key) to [ID] [int] IDENTITY(1,1)

markrendle commented 8 years ago

Hi. Yes, at present Simple.Data.SqlServer only returns inserted records when there is an IDENTITY column. v2 should address this.

As a technical note, using uniqueidentifier as a Primary Key is not recommended. Clustered indexes on GUIDs expand at a ridiculous rate. I suggest using an IDENTITY column as the Primary Key and a secondary column with a non-clustered index to hold the GUID. If you also specify newid() as the default for the GUID column, Simple.Data will return the generated value for that column as well.

skironDotNet commented 8 years ago

If I could click "like" on what markrendle said "As a technical note", I would. My hint is also that BigInt for Primary Key totally enough. I also had a chance to work with systems where PK was not set auto identity and the ID was generated by DAL using fancy algorithm. And yes I had a chance to work with GUID type PK and it was super hard to make any SQL investigations, JOINS, and findings, ORDER BY ID is hard with GUID.

If you really want to use GUID, you could disabale identity and work like this

var id = System.Guid.NewGuid() var example = db.Example.Insert(ID: id, Name: "some value");

WayneHiller commented 8 years ago

I agree with the "technical note", however there are cases when uniqueidentifier's are invaluable. For example I have built a number of databases that participate in a custom built replication system. Trying to merge records (with child records and relations) without guid primary keys is a nightmare.

The other place that they really helped is that these databases partition data by Customer. So each Customer gets their own copy of data but it is all stored within the same tables. Trying to build composite keys and links to other tables separated by Customer in a setup like this would be horrible.

Mark, how are you doing on Simple.Data 2.0? I know your busy as hell building your cloud services etc. I hope you still find some time to play on that xbox :-)

markrendle commented 8 years ago

I completely agree that uniqueidentifiers are invaluable for a range of use cases. Many of the databases I design use GUIDs as secondary ID columns, but I always use a BIGINT IDENTITY as the PK.

Simple.Data 2.0-beta1 is on course for a release next month, and I feel a blog post is in order...

jrnail23 commented 8 years ago

Point taken about GUIDs as PKs. Where we do use them, we do either newsequentialid() as default value or a CombGuid generated in the application to mitigate the index fragmentation / performance issues.

WayneHiller commented 8 years ago

Mark, so then you would use that Secondary key for foreign keys? Interesting, I never thought about it that way.

jrnail23, yes I use newsequentialid() as well. What do you mean by CombGuid?

jrnail23 commented 8 years ago

@WayneHiller, a CombGuid is an algorithm that creates a sequential GUID in application code. It's used in NHibernate as one of its built-in identity generator strategies, which AFAIK originates here: The Cost of GUIDs as Primary Keys, by Jimmy Nilson.

Also, more info here: GUIDs as fast primary keys under multiple databases

WayneHiller commented 8 years ago

Thanks @jrnail23, I have cases where I do indeed assign the GUID's from code, especially when doing large data imports, Using CombGuid's in that case would make a lot of sense. Thank you very much for the info :-)