Bad Habits to Kick : Using SELECT or RETURN instead of OUTPUT
See the full index.
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 = sys.sp_columns @table_name = N'dbo.bar'; SELECT @rc = @@ROWCOUNT; SELECT hr = @hr; RETURN (@rc); END
The title of this post kind of gives it away, but I thought it would be fun to ask anyway. Here is what I see wrong:
- 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 assigning multiple variables, IIRC the standard does not allow SELECT without FROM. And in the 20+ SELECT examples in Books Online, not one of them references scalars.
- 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.
- 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 = sys.sp_columns @table_name = N'dbo.bar'; SET @rc = @@ROWCOUNT; IF @debug = 1 BEGIN SELECT hr = @hr, rc = @rc; END RETURN (@hr); END
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% 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 may 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.
See the full index.
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!
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.
Aaron Bertrand for President! Yey!
(seriously – a great article that answered my question thoroughly)
You talk about the benefits of using OUTPUT, but you never use OUTPUT in any of your examples…
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?"
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.
*(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).
(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.
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.
RBarryYoung, how do you propose to call a stored procedure from a function?
"…do you see anything wrong with this procedure?"
The first thing that I noticed is that it should have been a function.
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.
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…