THE SQL Server Blog Spot on the Web

Welcome to SQLblog.com - The SQL Server blog spot on the web Sign in | |
in Search

Aaron Bertrand

Aaron is a Product Evangelist for SentryOne, makers of Plan Explorer and a performance monitoring and event management platform for the Microsoft Data Platform and VMware. He has been blogging here at sqlblog.com since 2006, focusing on manageability, performance, and new features, and also blogs at blogs.sentryone.com and SQLPerformance.com; has been a Microsoft MVP since 1997; tweets as @AaronBertrand; and speaks frequently at major conferences, user group meetings, and SQL Saturday events worldwide.

Bad habits to kick : using SELECT or RETURN instead of OUTPUT

In my last post in this series, I covered the use of "bad" characters in entity names, such as spaces or dashes.  In this post I will talk about using RETURN and OUTPUT inappropriately.

Jamie Thomson touched on part of this pet peeve in response to one of the other posts in this series.  So let me ask, do you see anything wrong with this procedure?

CREATE PROCEDURE dbo.foo
AS
BEGIN
    SET NOCOUNT ON
;

    
DECLARE 
@hr 
INT,
@rc INT;

    
EXEC @hr = dbo.sp_columns 'dbo.bar';

    
SELECT @rc = @@ROWCOUNT;

    
SELECT hr = @hr;

    
RETURN (@rc);
END
GO

The title of this post kind of gives it away, but I thought it would fun to ask anyway.  Here is what I see wrong:

  1. SELECT should be used for returning resultsets, not scalars.  This procedure uses a SELECT statement to return a single value to the client.  This is inefficient because most applications will have to prepare additional objects (typically referred to as "recordsets") and other support in order to consume the result.  While it is certainly valid syntax to use SELECT to return scalar values, this does not need to be common in production code.  This is the kind of thing that can make it slightly harder for high-end applications to scale.

    While not definitive proof that this is bad, and while I use SELECT for multiple variable assignment, IIRC the standard does not allow SELECT without FROM.  And in the 26 SELECT examples in Books Online, not one of them references scalars, at least at the time of writing.

  2. RETURN is for exiting the procedure, and is intended for returning error / status codes, not data.  Do not use this to send back to the caller the latest IDENTITY value generated or the row count from an operation.  Note that you are restricted to using integer-based data anyway, since you cannot change the data type of the value passed back by RETURN().  For more information, see the topic "Returning Data by Using a Return Code" in Books Online.

  3. OUTPUT is designed for returning scalar values (oh, and cursors, but who does that?).  In the example above, we are sending two scalar values back to the caller (though, in fact, one should be a RETURN value), yet we are not using an OUTPUT parameter at all!  For more information, see the topic "Returning Data by Using OUTPUT Parameters" in Books Online.  As an aside, I suggest always using the full word OUTPUT; you may be tempted to just type OUT but personally I find this lazy shorthand that could prove troublesome later - for example, if you have to search your codebase for all uses of OUTPUT parameters.

Yes, using OUTPUT makes it a little more complex to develop and debug stored procedures, since you have to declare your output parameters up front.  I am not against using SELECT for scalars while developing.  But there is no reason to deploy them that way.  You can even use a @debug parameter to switch the methodology depending on the scenario, e.g.:

CREATE PROCEDURE dbo.foo
  
@debug BIT = 0,
  
@rc  INT = NULL OUTPUT
AS
BEGIN
  SET NOCOUNT
ON;

  
DECLARE @hr INT;

  
EXEC @hr = dbo.sp_columns 'dbo.bar';

  
SET @rc = @@ROWCOUNT;

  
IF @debug = 1
BEGIN
SELECT 
hr = @hrrc = @rc;
 
END

RETURN 
(@hr);
END
GO

Now you can call this stored procedure with or without specifying the OUTPUT parameter up front.  (I use this @debug technique for a lot of debugging elements, including cases where using the flag can change the plan - in the normal case this doesn't really hurt anything because the production plan is the one that is in the cache 99.99% of the time.)

-- debug mode:

EXEC 
dbo.foo @debug = 1;


-- normal operation:

DECLARE 
@rc 
INT,
@hr INT;

EXEC @hr = dbo.foo @rc = @rc OUTPUT;

PRINT 
@hr;
PRINT @rc;
You'll notice that I laced the above commentary with several links to Books Online topics.  I highly recommend becoming very familiar with when and where you should use RETURN, OUTPUT and SELECT to return data from a stored procedure.

I am working on a series of "Bad habits to kick" articles, in an effort to motivate people to drop some of the things that I hate to see when I inherit code.  Up next: using SELECT * / omitting the column list. 

Published Friday, October 09, 2009 5:01 PM by AaronBertrand

Comments

 

Jason Short said:

You know if you used a CLR Proc then you could actually use things like Debug.Assert, or #if DEBUG and have even cleaner debug / release procedures.  But I know, most people don't use them...

October 10, 2009 12:17 PM
 

AaronBertrand said:

I think you'll have a hard time convincing people to use CLR for that benefit.  In my experience, most stored procedures are still used for data retrieval / modification operations, and these aren't exactly CLR's cup of tea.  CLR certainly has its place, and I'm not against it, but I don't think there should be any broad push to move all stored procedures to CLR even though there are some minor benefits to doing so.

October 10, 2009 1:57 PM
 

RBarryYoung said:

"...do you see anything wrong with this procedure?"

The first thing that I noticed is that it should have been a function.

October 11, 2009 10:38 AM
 

AaronBertrand said:

RBarryYoung, how do you propose to call a stored procedure from a function?

October 11, 2009 10:51 AM
 

Matija Lah said:

RBarryYoung, regardless of what you say in response to Aaron, I would personally like to know *why* would you prefer to use a function in this case.

October 13, 2009 4:34 AM
 

RBArryYoung said:

(Hmm, sorry for the delay, I wasn't signed in before, so I didn't get notification...)

Aaron: You don't need to call the stored procedure dbo.sp_columns 'dbo.foo'.  You could accomplish the same thing by querying either sys.columns or INFORMATION_SCHEMA.COLUMNS, and that means that the whole thing could have been a function instead (or even a view, since there are no parameters).

Matijah: Because functions are far more general and can be used in far more places than sProcs.  For instance, functions and views can call other functions and views, but they cannot call sProcs*.  So if you unnecessarily make something a sProc instead of a function it just increases the chance that you won't be able to re-use in the future, or worse (IMHO) that you will have to use Cursors on a sProc when you could have used Cross Apply on a function instead.

October 23, 2009 7:24 PM
 

RBArryYoung said:

*(and yes, I know that there is an obscure trick that allows a function to call a stored procedure, but it has whole list of problems of its own).

October 23, 2009 7:25 PM
 

AaronBertrand said:

RBarryYoung, let's forget that the procedure I used in my example only returns a resultset.  Imagine that it does something that a function can't do (like manipulate data).  I regret using a simple data retrieval procedure for this post because my point was about how people use procedures, not to get into a war about whether it should have been a function instead.

October 23, 2009 7:46 PM
 

RBArryYoung said:

Sure, Aaron, no problem, I agree with what your article says.  I was just pointing out that that was my first reaction to the article's question "do you see anything wrong with this procedure?"

October 25, 2009 10:35 AM
 

paul said:

You talk about the benefits of using OUTPUT, but you never use OUTPUT in any of your examples...

March 14, 2013 1:37 PM
 

Dave said:

Aaron Bertrand for President! Yey!

(seriously - a great article that answered my question thoroughly)

August 13, 2014 8:46 AM
 

Barry said:

Thanks AaronBertrand, this solidified my understanding to a problem earlier.

There are always arguments of which is the most optimal, lean, fit for purpose solution when it comes to 99% of the problems we come across, but sometimes we live in a dirty world and in the trenches the job just has to be done. I'm by no way advocating messy, obfuscated or irresponsible code. I'm just saying there are times when we can only follow the boy scout rule and leave things a little nicer than they were for the next guy. This article was a great help.

October 27, 2014 5:03 PM
 

Travis Truax said:

Best practices are great when you have the opportunity to use them, but in the real world I often find myself having to deal with some less-than-optimal situations. Last year I was working on some integration with a labeling system that had such simple db functionality that it automatically expected a resultset no matter what, and refused to work properly with output parameters, or return values, so all of the output had to be via a select. I know that's not the norm though. Just saying sometimes you have to do whatever crap you can get to work!

July 11, 2016 9:37 AM
New Comments to this post are disabled

About AaronBertrand

...about me...

This Blog

Syndication

Archives

Privacy Statement