26

I have a table where I created an INSTEAD OF trigger to enforce some business rules.

The issue is that when I insert data into this table, SCOPE_IDENTITY() returns a NULL value, rather than the actual inserted identity.

Insert + Scope code

INSERT INTO [dbo].[Payment]([DateFrom], [DateTo], [CustomerId], [AdminId])
VALUES ('2009-01-20', '2009-01-31', 6, 1)

SELECT SCOPE_IDENTITY()

Trigger:

CREATE TRIGGER [dbo].[TR_Payments_Insert]
   ON  [dbo].[Payment]
   INSTEAD OF INSERT
AS 
BEGIN
-- SET NOCOUNT ON added to prevent extra result sets from
    -- interfering with SELECT statements.
    SET NOCOUNT ON;

    IF NOT EXISTS(SELECT 1 FROM dbo.Payment p
              INNER JOIN Inserted i ON p.CustomerId = i.CustomerId
              WHERE (i.DateFrom >= p.DateFrom AND i.DateFrom <= p.DateTo) OR (i.DateTo >= p.DateFrom AND i.DateTo <= p.DateTo)
              ) AND NOT EXISTS (SELECT 1 FROM Inserted p
              INNER JOIN Inserted i ON p.CustomerId = i.CustomerId
              WHERE  (i.DateFrom <> p.DateFrom AND i.DateTo <> p.DateTo) AND 
              ((i.DateFrom >= p.DateFrom AND i.DateFrom <= p.DateTo) OR (i.DateTo >= p.DateFrom AND i.DateTo <= p.DateTo))
              )

    BEGIN
        INSERT INTO dbo.Payment (DateFrom, DateTo, CustomerId, AdminId)
        SELECT DateFrom, DateTo, CustomerId, AdminId
        FROM Inserted
    END
    ELSE
    BEGIN
            ROLLBACK TRANSACTION
    END


END

The code worked before the creation of this trigger. I am using LINQ to SQL in C#. I don't see a way of changing SCOPE_IDENTITY to @@IDENTITY. How do I make this work?

George Stocker
  • 57,289
  • 29
  • 176
  • 237
kastermester
  • 3,058
  • 6
  • 28
  • 44
  • Why not use a BEFORE INSERT trigger that produces an error if the rules aren't satisfied, rather than an INSTEAD OF? – araqnid May 25 '09 at 23:04
  • 1
    @araqnid - because as far as I have been able to see SQL Server does not have a such thing - although I gotta admit that I didn't directly try one, but based that off a Google search, it may be of course that I found invalid or outdated resources - is there in fact a such thing? – kastermester May 25 '09 at 23:42
  • We've used such triggers in SQL Server 2000 - that seems to be the default mode, just declaring the trigger as "create trigger TG_xxx on dbo.tablename for insert, update as ....". We have logic to test conditions and call "raiserror()" then do "rollback transaction" if the validation fails. Having said that, we use an SP to generate IDs rather than identity --- this might be why, the guy who insisted we do it this way was a bit vague as to the reasoning. But then he'd developed the habit before scope_identity() was implemented. – araqnid May 26 '09 at 13:39
  • Please consider Aaron Alton's suggestion to use output to get the indetity values. – HLGEM May 26 '09 at 17:15
  • araqnid, your method is far more dangerous to data integrity than using identity fields. You can create real problems with concurrent inserts if you aren't very careful. – HLGEM May 26 '09 at 17:17
  • Yes, the ID-generation SP does some extra work to exclusively lock the table holding the next ID. Which itself causes contention problems. I'm not a fan of it, either (and I definitely didn't write it and don't recommend it). – araqnid May 27 '09 at 00:43
  • I take it this table was working before you inserted the INSTEAD OF statement? Have you checked your Primary Key field to make sure it has an Identity specification? – Robert Harvey May 25 '09 at 22:30
  • Yes and yes it does indeed (you can see the code now - it does insert a row and it does get an automatic identity when inserted). – kastermester May 25 '09 at 22:35

6 Answers6

21

Use @@identity instead of scope_identity().

While scope_identity() returns the last created id in the current scope, @@identity returns the last created id in the current session.

The scope_identity() function is normally recommended over the @@identity field, as you usually don't want triggers to interfer with the id, but in this case you do.

Guffa
  • 687,336
  • 108
  • 737
  • 1,005
  • 3
    After I added some more content to my post you can see that I use LINQ to SQL in my .Net app, so I really do not have much of a choice here as far as I can see, except possibly using an sproc to insert the data. – kastermester May 25 '09 at 22:36
  • I have serious hesitations about using @@identity, but I guess in this narrow case it's okay since the other workarounds aren't really much better, and at least you know it will give the right value if the trigger doesn't insert to another table in the meantime. – ErikE Mar 06 '10 at 19:01
  • 3
    @Emtucifor: In most cases you want what scope_identity() returns, but not in this case. This time it's @@identity that is the correct choise. The reason that both exist is that sometimes you actually want the unusual result. – Guffa Mar 06 '10 at 19:21
  • 3
    Guffa, you only want @@identity because scope_identity() is not performing the function we all would expect it to: you insert to a table that has an identity column, and then want THAT identity column value back. Scope_Identity() is supposed to protect you from needing to search for and then scrutinize any triggers on the table to be sure they aren't inserting to another table with an identity column. But the unrestricted recommendation you made to use @@identity will break the moment there's another after-trigger on the table, or the instead-of-trigger inserts to a second table. – ErikE Mar 06 '10 at 23:47
  • See the answer I provided for more on this. – ErikE Mar 06 '10 at 23:49
  • 3
    I realize that you can't assume that, when inserting to a table with an INSTEAD OF trigger on it, that the rows will even be added to the table at all. They could be put in another table, or many other tables, or not inserted anywhere at all. However, the calling script doesn't know this and shouldn't need to know this. The point of an INSTEAD OF trigger is to make the messy guts of the underlying data operation transparent to the client that is doing the insert. In my opinion, there should be some way to explicitly set scope_identity in an INSTEAD OF trigger. – ErikE Mar 07 '10 at 00:32
13

Since you're on SQL 2008, I would highly recommend using the OUTPUT clause instead of one of the custom identity functions. SCOPE_IDENTITY currently has some issues with parallel queries that cause me to recommend against it entirely. @@Identity does not, but it's still not as explicit, and as flexible, as OUTPUT. Plus OUTPUT handles multi-row inserts. Have a look at the BOL article which has some great examples.

ErikE
  • 48,881
  • 23
  • 151
  • 196
Aaron Alton
  • 22,728
  • 6
  • 34
  • 32
  • what issues with parallel queries? they are in a different scope, so I don't see where they would be an issue. ident_current has issues with parallel inserts and @@identity has issues with inserts from triggers which is why scope_identity was the preferrered method before output was invented. However the advice to use Output is very good. – HLGEM May 26 '09 at 17:14
  • 1
    Sorry, I should have posted the connect link: https://connect.microsoft.com/SQLServer/feedback/ViewFeedback.aspx?FeedbackID=328811&wa=wsignin1.0 – Aaron Alton May 27 '09 at 02:07
  • The point about parallel queries is good. But @@identity handles multi-row inserts just fine: `INSERT TheTable ... | SELECT @LastID = Scope_Identity(), @Rows = @@RowCount | SELECT FROM TheTable WHERE ID BETWEEN @LastID - @Rows + 1 AND @LastID` – ErikE Mar 07 '10 at 00:35
  • 4
    Also, the OUTPUT clause will always return 0 for identity columns if an INSTEAD OF INSERT trigger exists on the table. – Nathan Dec 20 '12 at 16:50
  • For anyone who has never heard of or used OUTPUT clause, here is a thread that explains how to use it: http://stackoverflow.com/questions/10999396/how-do-i-use-an-insert-statements-output-clause-to-get-the-identity-value – Parth Shah Jul 31 '14 at 09:02
9

I was having serious reservations about using @@identity, because it can return the wrong answer.

But there is a workaround to force @@identity to have the scope_identity() value.

Just for completeness, first I'll list a couple of other workarounds for this problem I've seen on the web:

  1. Make the trigger return a rowset. Then, in a wrapper SP that performs the insert, do INSERT Table1 EXEC sp_ExecuteSQL ... to yet another table. Then scope_identity() will work. This is messy because it requires dynamic SQL which is a pain. Also, be aware that dynamic SQL runs under the permissions of the user calling the SP rather than the permissions of the owner of the SP. If the original client could insert to the table, he should still have that permission, just know that you could run into problems if you deny permission to insert directly to the table.

  2. If there is another candidate key, get the identity of the inserted row(s) using those keys. For example, if Name has a unique index on it, then you can insert, then select the (max for multiple rows) ID from the table you just inserted to using Name. While this may have concurrency problems if another session deletes the row you just inserted, it's no worse than in the original situation if someone deleted your row before the application could use it.

Now, here's how to definitively make your trigger safe for @@Identity to return the correct value, even if your SP or another trigger inserts to an identity-bearing table after the main insert.

Also, please put comments in your code about what you are doing and why so that future visitors to the trigger don't break things or waste time trying to figure it out.

CREATE TRIGGER TR_MyTable_I ON MyTable INSTEAD OF INSERT
AS
SET NOCOUNT ON

DECLARE @MyTableID int
INSERT MyTable (Name, SystemUser)
SELECT I.Name, System_User
FROM Inserted

SET @MyTableID = Scope_Identity()

INSERT AuditTable (SystemUser, Notes)
SELECT SystemUser, 'Added Name ' + I.Name
FROM Inserted

-- The following statement MUST be last in this trigger. It resets @@Identity
-- to be the same as the earlier Scope_Identity() value.
SELECT MyTableID INTO #Trash FROM MyTable WHERE MyTableID = @MyTableID

Normally, the extra insert to the audit table would break everything, because since it has an identity column, then @@Identity will return that value instead of the one from the insertion to MyTable. However, the final select creates a new @@Identity value that is the correct one, based on the Scope_Identity() that we saved from earlier. This also proofs it against any possible additional AFTER trigger on the MyTable table.

Update:

I just noticed that an INSTEAD OF trigger isn't necessary here. This does everything you were looking for:

CREATE TRIGGER dbo.TR_Payments_Insert ON dbo.Payment FOR INSERT
AS 
SET NOCOUNT ON;
IF EXISTS (
   SELECT *
   FROM
      Inserted I
      INNER JOIN dbo.Payment P ON I.CustomerID = P.CustomerID
   WHERE
      I.DateFrom < P.DateTo
      AND P.DateFrom < I.DateTo
) ROLLBACK TRAN;

This of course allows scope_identity() to keep working. The only drawback is that a rolled-back insert on an identity table does consume the identity values used (the identity value is still incremented by the number of rows in the insert attempt).

I've been staring at this for a few minutes and don't have absolute certainty right now, but I think this preserves the meaning of an inclusive start time and an exclusive end time. If the end time was inclusive (which would be odd to me) then the comparisons would need to use <= instead of <.

ErikE
  • 48,881
  • 23
  • 151
  • 196
  • This is really brilliant - although I no longer need a solution for this - for others and possibly myself in the future, this is just pure awesomeness :) – kastermester Mar 07 '10 at 14:10
  • Thanks, guys! I figured this out a few years ago after agonizing over problems in an Access Data Project (ADP) which uses @@Identity instead of Scope_Identity. I desperately wanted to use an INSTEAD OF UPDATE trigger so I could bind forms to a view and make it updatable. I finally got it working! (For completeness, please note that this requires WITH VIEW_METADATA to prevent Access from querying the underlying tables itself.) – ErikE Mar 09 '10 at 01:05
  • It's possible there could be a performance improvement by not hitting the original table again at the end, through creating #Trash, then setting identity_insert on and doing the insert with @MyTableID. – ErikE Mar 09 '10 at 01:28
  • Still doesn't affect `scope_identity()` usages in the caller (outside the trigger). While the `@@identity` might be correct, `scope_identity()` still returns null - this means the INSTEAD OF trigger is **still** a breaking change when added. (And an INSTEAD OF trigger is required to update through a view with computed columns..) – user2864740 Jan 02 '21 at 05:28
  • @user2864740 Interesting. What version of SQL Server? And can the code that broke be changed to use `COALESCE(scope_identity(), @@identity)`? – ErikE Jan 04 '21 at 18:51
  • SQL Server 2014. Unfortunately, my particular case also has merge replication which is known to negative affect @@identity. The current pending change is to switch to using the base table for the insert. I'm mostly a bit grumpy TSQL doesn't provide a method to 'reset' the caller's scope_identity when an instead-of trigger is in play, nor can OUTPUT be used to obtain the identity when there is an instead-of trigger.. the better change in the code I'm dealing with would probably be to move to an insert SP and not worry about the view trigger. Time and timing. Always issues on old crufty code. – user2864740 Jan 04 '21 at 20:48
4

Main Problem : Trigger and Entity framework both work in diffrent scope. The problem is, that if you generate new PK value in trigger, it is different scope. Thus this command returns zero rows and EF will throw exception.

The solution is to add the following SELECT statement at the end of your Trigger:

SELECT * FROM deleted UNION ALL
SELECT * FROM inserted;

in place of * you can mention all the column name including

SELECT IDENT_CURRENT(‘tablename’) AS <IdentityColumnname>
Ashish Mishra
  • 667
  • 7
  • 14
2

A little late to the party, but I was looking into this issue myself. A workaround is to create a temp table in the calling procedure where the insert is being performed, insert the scope identity into that temp table from inside the instead of trigger, and then read the identity value out of the temp table once the insertion is complete.

In procedure:

CREATE table #temp ( id int )

... insert statement ...

select id from #temp
-- (you can add sorting and top 1 selection for extra safety)

drop table #temp

In instead of trigger:

-- this check covers you for any inserts that don't want an identity value returned (and therefore don't provide a temp table)
IF OBJECT_ID('tempdb..#temp') is not null
begin
    insert into #temp(id)
    values
    (SCOPE_IDENTITY())
end

You probably want to call it something other than #temp for safety sake (something long and random enough that no one else would be using it: #temp1234235234563785635).

Ryan
  • 3,127
  • 6
  • 32
  • 48
2

Like araqnid commented, the trigger seems to rollback the transaction when a condition is met. You can do that easier with an AFTER INSTERT trigger:

CREATE TRIGGER [dbo].[TR_Payments_Insert]
   ON  [dbo].[Payment]
   AFTER INSERT
AS 
BEGIN
    SET NOCOUNT ON;

    IF <Condition>
    BEGIN
        ROLLBACK TRANSACTION
    END
END

Then you can use SCOPE_IDENTITY() again, because the INSERT is no longer done in the trigger.

The condition itself seems to let two identical rows past, if they're in the same insert. With the AFTER INSERT trigger, you can rewrite the condition like:

IF EXISTS(
    SELECT *
    FROM dbo.Payment a
    LEFT JOIN dbo.Payment b
        ON a.Id <> b.Id
        AND a.CustomerId = b.CustomerId
        AND (a.DateFrom BETWEEN b.DateFrom AND b.DateTo
        OR a.DateTo BETWEEN b.DateFrom AND b.DateTo)
    WHERE b.Id is NOT NULL)

And it will catch duplicate rows, because now it can differentiate them based on Id. It also works if you delete a row and replace it with another row in the same statement.

Anyway, if you want my advice, move away from triggers altogether. As you can see even for this example they are very complex. Do the insert through a stored procedure. They are simpler and faster than triggers:

create procedure dbo.InsertPayment
    @DateFrom datetime, @DateTo datetime, @CustomerId int, @AdminId int
as
BEGIN TRANSACTION

IF NOT EXISTS (
    SELECT *
    FROM dbo.Payment
    WHERE CustomerId = @CustomerId
    AND (@DateFrom BETWEEN DateFrom AND DateTo
    OR @DateTo BETWEEN DateFrom AND DateTo))
    BEGIN

    INSERT into dbo.Payment 
    (DateFrom, DateTo, CustomerId, AdminId)
    VALUES (@DateFrom, @DateTo, @CustomerId, @AdminId)

    END
COMMIT TRANSACTION
Andomar
  • 232,371
  • 49
  • 380
  • 404
  • Thanks for the examples and advices, I have taken a look at it, and I will probably use some of your ideas - however not all of them are applicable in this case. Also, yes I do no checks on the Id column, I let the DB handle that for me (along with a couple of other constraints) however I will probably move my trigger to after insert - i completely failed to see the logical solution when I created my trigger :) – kastermester May 26 '09 at 10:11
  • 1
    @Andomar, using BETWEEN means the server has to check four conditions. You can get by with half that many. See http://stackoverflow.com/questions/325933/determine-whether-two-date-ranges-overlap/325964#325964 for more details. – ErikE Mar 09 '10 at 01:24
  • Also, could we get some references for how stored procedures are both SIMPLER and FASTER than triggers? A trigger is often simpler because it can absolutely enforce data integrity rules, but just having a procedure doesn't guarantee it always gets used (don't give me that the app only uses the SP, some day someone inserts some rows somewhere through the back end). And I'm skeptical about the faster bit. Can you prove this? – ErikE Mar 09 '10 at 01:26
  • @Emtucifor: The link you give assumes EndA > StartA, which I would not rely on if it's not enforced by check constraints. A trigger that enforces integrety rules should be replaced by a check constraint (optionally with a UDF.) Triggers are evil and should be eliminated. – Andomar Mar 09 '10 at 11:46
  • @Andomar: that's incorrect. There is no assumption that EndA > StartA. Please see http://silentmatt.com/intersection.html for help visualizing how this works for ALL ranges. Second, Could you provide a source or some scholarly work on how triggers are always evil and should always be eliminated? I agree they are often overused, misapplied, or badly written, and if a check or foreign key constraint can do the job they are inappropriate. However, triggers are MORE reliable than SPs. So many times people say "the app only uses the SP" but some day, someone inserts data directly. – ErikE Mar 10 '10 at 20:55
  • @Emtucifor: For example StartA = 2006, EndA = 2004, StartB = 2005, EndB = 2007 would not match, but it does overlap. Scholarly work is too remote from commercial reality to provide meaningful insights. The problem of triggers is not reliability but complexity. No app will succeed in inserting data directly into tables I maintain. – Andomar Mar 10 '10 at 21:39
  • @Andomar: My apologies. I missed that both were A in your comparisons. However, I now see another issue with your query: if DateFrom and DateTo are entirely within @DateFrom and @DateTo, your query will fail! As for @DateFrom and DateTo being in reverse order, fix that with some SET statements first (so I still think your query needs some updating). About the other point, I said twice that it wasn't the app that would insert directly to the tables, but a *person*. As to the claim that scholarly work is unable to provide meaningful insights... um, ok. I'll stop casting pearls now. :) – ErikE Mar 11 '10 at 05:11
  • Just to be clear: Do some assignment statements to make @DateFrom < @DateTo and then you can use two conditions instead of 4 in your query which should be a huge performance boost, especially with an index on one of the dates (and ALSO get the case of one range fully enclosing the other which your query is missing right now). – ErikE Mar 11 '10 at 05:43
  • @Emtucifor: Update queries are just a quick fix; they won't prevent new entries. As to your comment about the range being entirely within the range, I suggest you read the question before commenting on answers ;) – Andomar Mar 11 '10 at 09:31
  • Update queries? I didn't say anything about them. I meant SET statements solely on the variables @DateFrom and @DateTo. If you mean that the *columns* DateFrom and DateTo can be in reverse chronological order, then your query also fails. Last, do you mean that since the original poster didn't check for entirely enclosed ranges, that is the true requirement? I wonder if that is an error. – ErikE Mar 11 '10 at 17:47