I have the pleasure at the moment of doing a code review on some vendor code. No names will be mentioned. I’ve seen better. I’ve seen a lot better. I’m seeing very common mistakes in the code, so, in the interests of my sanity, I’m going to go over a couple of common T-SQL mistakes in the hopes that the next batch of code I get to review doesn’t have these mistakes in…
1. Error Handling
Proper error handling is hard. SQL 2005 has made it a lot easier with the TRY…CATCH blocks, but it still means that everything that can throw an error be wrapped inside a TRY block, with an appropriate CATCH block to handle any errors.
It was a lot harder on SQL 2000 when all we had to work with was @@Error. What I think was not well understood was what statements set and reset @@Error, and how long a non-zero value persists, leading to code constructs like this
Insert into SomeTable ... Update SomeTable SET ... Delete From SomeOtherTable ... IF @@Error !=0 Print 'An error occured'
Every statement sets the value of @@Error to the error number it returned, or 0 if the statement succeeded. In the previous example, the only error that would be picked up by the IF is an error that occurs in the delete statement. If either the insert or the update threw an error, the error handling wouldn’t notice.
It’s more subtle than that. Not only do DDL and DML statements set the value of @@Error, but so does every other statement in T-SQL. Consider this.
Insert into SomeTable ... Update SomeTable SET ... Delete From SomeOtherTable ... IF @@Error !=0 RETURN @@Error ELSE RETURN 0
That will always return 0, because the IF itself will reset the value of @@Error to the error number it threw (0) clearing out the error that the previous statement set.
So, how to do this properly? Here’s one way.
DECLARE @ErrNo INT Insert into SomeTable ... SET @ErrNo = @@Error IF @ErrNo !=0 GOTO ErrHandling Update SomeTable SET ... SET @ErrNo = @@Error IF @ErrNo !=0 GOTO ErrHandling Delete From SomeOtherTable ... SET @ErrNo = @@Error IF @ErrNo !=0 GOTO ErrHandling RETURN 0 ErrHandling: RETURN @ErrNo
Another way would be to use a TRY … CATCH block.
2. Transations
Nested transactions is another thing that seems to catch people out. Consider this piece
CREATE PROCEDURE LogError @ErrString Varchar(100) AS BEGIN TRANSACTION INSERT INTO LoggingTable (Message) Values (@ErrString) COMMIT TRANSACTION GO
Nothing wrong there. Now, add this.
BEGIN TRANSACTION -- Do fancy stuff here If (some condition indicating an error) BEGIN EXEC LogError 'Something went seriously wrong' ROLLBACK TRANSACTION END ELSE COMMIT TRANSACTION
So, what’s in the logging table? Absolutely nothing. The commit within the LogError stored procedure is nested. All the commit does is decrement the @@Trancount (the count of open transactions) by 1. The transaction is still open and uncommitted. When the rollback runs, it rolls back all open transactions, including the one that logged the error message.
In my example, the fix is easy. Do the logging after the rollback. I’ve seen many real-life situations where it isn’t so simple, where logging is done in many places throughout the proc and the commit/rollback done at the end.
It’s sometimes possible to use table variables, which are not part of transactions. Otherwise it may require some redesigning of the procedure.
I’ll try and a more substantial and useful post out in a couple days, when I get a few minutes free from debugging Jadex agents and studying .Net stuff.