Abstracting out a query to a stored procedure makes it run very slowly

Question

I have a very long stored procedure with a number of blocks of logic in it that insert to different tables. Here's one such block

I have the following table with a unique constraint on 'data'

[id] [int] IDENTITY(1,1) NOT NULL
[data] [varchar](512) NULL

This block attempts to insert a value to 'data'. if that value is unique, it is inserted. In all cases the relevant data id is returned

BEGIN TRY 
    INSERT INTO Data SELECT @data; 
END TRY 
BEGIN CATCH 
END CATCH   
SET @data_id = (SELECT id FROM Data WHERE data = @data);

When I include this block of code in my original stored procedure, it runs fine. However, for the sake of neatness I and DRY, I thought I'd abstract it out to a sub-procedure, as the same block is called in a few other SPs

ALTER PROCEDURE [dbo].[q_Data_TryInsert]

   @data nvarchar(512),
   @id INT OUTPUT

AS
BEGIN

    BEGIN TRY 
        INSERT INTO Data SELECT @data; 
    END TRY 
    BEGIN CATCH 
    END CATCH   
    SET @id = (SELECT id FROM Data WHERE data = @data);     

END

I then call this abstracted SP like so

EXEC [q_Data_TryInsert] @data, @data_id OUTPUT

The abstracted SP slows down the whole process my several orders of magnitude, even though the code is the same.

Why is this happening?

You should never have exception "catching" in your normal logic flow. (Thus why it is called an "exception"..it should be exceptional (rare). Put a exists check around your INSERT. "if not exists (select null from Data where data = @data) begin /* insert here */ END. — granadaCoder, Jun 08 '15 at 13:23
Yu might want to look at scope_identity as well, it will save you having to do that select @id — Tony Hopkinson, Jun 08 '15 at 13:25
What does the execution plan show as the slowest element in the slow query? — Tab Alleman, Jun 08 '15 at 14:14
Also, in SQL, DRY is an anti-pattern. You need much a better reason than saving keystrokes to justify those extra layers of abstraction. — Anon, Jun 08 '15 at 14:18
Your question is why, but I don't see enough detail about the original proc to determine how that data is being used subsequently. One suggestion, use the Output command on the table into a temp table then grab your single value from that small list, rather than searching the whole table for your new "data" row. Or use the Merge w/Output statement to do this in batch. — Brad D, Jun 08 '15 at 15:01
@granadaCoder as far as I know, an EXISTS check is slower than using a unique index. — roryok, Jun 09 '15 at 08:24
@TonyHopkinson `SCOPE_IDENTITY` only works if the insert was successful. It doesn't return the @id if this value already exists in the table. Although I suppose I could use it in the `try` block and move the select statement to the `catch` block — roryok, Jun 09 '15 at 08:26
As far as you know EXISTS check is slower than using a unique index? Did you test? Throwing an exception has overhead. Really think trying the insert and getting an exception is faster than avoiding both the insert and exception all together via an exists? — paparazzo, Jun 09 '15 at 10:34
@roryok. Assume competence. To get rid of the exception you need an exists. If it doesn't you insert and get scope identiity. Either way you end with Id which gets rid of the next query... — Tony Hopkinson, Jun 09 '15 at 11:28
Do NOT use Exceptions as normal-logic-flows. This is a horrible practice.... — granadaCoder, Jun 09 '15 at 13:18
But I have to use a try / catch in this case surely? I could have two ops run concurrently, both return nothing for the data_id and both try to insert. My insert statement therefore _must_ be wrapped in a try catch block. Remus Rusanu says as much in his answer to this very similar question - http://stackoverflow.com/questions/11011364/constraints-check-try-catch-vs-exists — roryok, Jun 09 '15 at 16:05
"Option - 3" is the best approach from that answer, IMHO. (from Pankaj). You can put in exception handling to handle EXCEPTIONS. So yes, 2 people checking for the same row within a few ms of each other...is EXCEPTIONAL...(very rare odds to happen)....do not use exception-handling for normal-logic-flows. — granadaCoder, Jun 09 '15 at 21:08

paparazzo · Answer 1 · 2015-06-09T10:20:40.397

1

INSERT INTO [PKvalue] ([value])
select 'Data6' as [value] 
 where not exists (select top 1 ID from [PKvalue] where [value] = 'Data6');
select top 1 ID from [PKvalue] where [value] = 'Data6';

INSERT INTO data (data)
select @dtata as [data] 
 where not exists (select top 1 ID from [data] where [data] = @data);
select top 1 ID from [data] where [data] = '@data;

Don't even need a transaction. That insert is a transaction. Even if another insert happened before the select you would still get the right answer. Only a delete or update could break the select. A transaction has overhead.

edited Jun 09 '15 at 10:20

answered Jun 08 '15 at 14:03

paparazzo

44,497
23
105
176

is this true even if I have multiple threads running this SQL statement? – roryok Aug 04 '15 at 10:38
Transaction is a transaction. – paparazzo Aug 04 '15 at 10:50

score 1 · Answer 2 · answered Jun 08 '15 at 14:41

Test for data, saving @id. Insert @data if needed. Update @id if needed.

BEGIN TRANSACTION
  DECLARE @output TABLE (id int)

  SELECT @id = id FROM #Data WHERE data = @data

  INSERT Data (data)
  OUTPUT inserted.[id] INTO @output
  SELECT @data
  WHERE @id IS NULL

  SELECT TOP 1 @id = id FROM @output
COMMIT TRANSACTION

Yuri · Answer 3 · 2015-06-09T12:36:46.947

0

please change

INSERT INTO Data SELECT @data;

to

INSERT INTO Data (data)
 VALUES (@data)

And change

SET @data_id = (SELECT id FROM Data WHERE data = @data);

to

SET @data_id = IDENT_CURRENT('Data')

EDIT: to get what you need the store procedure needs to be reworked in this way

ALTER PROCEDURE [dbo].[q_Data_TryInsert]

   @data nvarchar(512),
   @id INT OUTPUT

AS
BEGIN
  IF NOT EXISTS(SELECT id FROM Data WHERE data = @data)
   BEGIN
    INSERT INTO Data (data) Values (@data) 
    SET @data_id = IDENT_CURRENT('Data')    
   END
  ELSE
    SET @id = (SELECT id FROM Data WHERE data = @data);     

END

edited Jun 09 '15 at 12:36

answered Jun 08 '15 at 14:00

Yuri

2,820
4
28
40

If the record already exists this will not return the id. Works for inserts only. Although I could put it inside my try catch block which would speed things up in that case. So thanks. – roryok Jun 09 '15 at 08:21

Abstracting out a query to a stored procedure makes it run very slowly

3 Answers3

Linked