Ruby on Rails | Screencasts | Download | Documentation | Weblog | Community | Source

Ticket #7662 (closed defect: duplicate)

Opened 2 years ago

Last modified 1 year ago

[PATCH] SQL Server adapter should use SCOPE_IDENTITY() in place of @@IDENTITY

Reported by: tomafro Assigned to: tomafro
Priority: low Milestone: 1.x
Component: ActiveRecord Version: edge
Severity: normal Keywords: sqlserver
Cc:

Description

According to Pat Wendorf:

{{{"In the insert function you're using @@IDENTITY to get the last inserted row id from the server. Since SQL 2k, SCOPE_IDENTITY() has been the right way to do this."

"You won't see this problem until you add a trigger to a table that does an insert to another table with an identity column. In this case @@IDENTITY would return the id for the new row that was inserted in the table in the trigger, not the row that you inserted into the original table."

"SCOPE_IDENTITY() is a drop in replacement for @@IDENTITY, but it will not work on SQL 7.0 or lower."}}}

While supporting databases with triggers isn't exactly a priority, it's probably best to alter the adapter to use the correct technique, maybe reverting to the older technique for older databases.

Attachments

sqlserver_use_scope_identity.diff (0.7 kB) - added by tomafro on 07/03/07 17:12:16.

Change History

(follow-up: ↓ 2 ) 04/19/07 17:21:12 changed by mattmcknight

  • severity changed from trivial to normal.

Recommend changing severity as this will cause all inserts to fail if there are triggers that insert into tables with identity fields.

Patch is pretty simple, unless you want to add a check for the SQL Server version, which would possibly require a change to the ADO driver.

(in reply to: ↑ 1 ; follow-up: ↓ 3 ) 04/19/07 17:25:16 changed by tomafro

Replying to mattmcknight:

Recommend changing severity as this will cause all inserts to fail if there are triggers that insert into tables with identity fields. Patch is pretty simple, unless you want to add a check for the SQL Server version, which would possibly require a change to the ADO driver.

Am in process of compiling bundled patch including this fix. I have written code to check for the SQL Server version as this is likely to be generally useful, but I'm not sure exactly why this might require a change to the ADO driver. Could you elaborate?

(in reply to: ↑ 2 ; follow-up: ↓ 4 ) 04/19/07 18:00:39 changed by mattmcknight

Replying to tomafro:

Replying to mattmcknight:

Patch is pretty simple, unless you want to add a check for the SQL Server version, which would possibly require a change to the ADO driver.

Am in process of compiling bundled patch including this fix. I have written code to check for the SQL Server version as this is likely to be generally useful, but I'm not sure exactly why this might require a change to the ADO driver. Could you elaborate?

That was just the easiest method I could think of to check for the version of SQL Server without breaking compatibility in the ODBC use of sql server- you're probably smarter than me on this, since I really haven't been thinking about it at all.

The reason to check for the version would be to keep compatibility with SQL Server 7 by retaining the @@identity method for those servers, I suppose. I think supporting SS7 users is probably lower priority than supporting users with triggers that do inserts, but I could be wrong.

As it is I just did a drop in of SCOPE_IDENTITY() and it appears to work with my triggers. They use DatabaseMail to send email, which actually does an insert in a log somewhere, which is why this is giving me headaches.

(in reply to: ↑ 3 ) 04/20/07 10:36:32 changed by tomafro

Replying to mattmcknight:

That was just the easiest method I could think of to check for the version of SQL Server without breaking compatibility in the ODBC use of sql server- you're probably smarter than me on this, since I really haven't been thinking about it at all.

I wouldn't bet on me being smarter! I'm just using SELECT @@VERSION version and parsing the resulting string.

The reason to check for the version would be to keep compatibility with SQL Server 7 by retaining the @@identity method for those servers, I suppose. I think supporting SS7 users is probably lower priority than supporting users with triggers that do inserts, but I could be wrong.

I think you're probably right. I'll be submitting the patch this weekend.

07/03/07 17:12:16 changed by tomafro

  • attachment sqlserver_use_scope_identity.diff added.

07/03/07 17:13:00 changed by tomafro

  • summary changed from SQL Server adapter should use SCOPE_IDENTITY() in place of @@IDENTITY to [PATCH] SQL Server adapter should use SCOPE_IDENTITY() in place of @@IDENTITY.

Added patch

07/03/07 17:47:56 changed by tomafro

  • status changed from new to closed.
  • resolution set to duplicate.

Rolled up into #8862