Top 10 T-SQL Code Smells

re: Top 10 T-SQL Code Smells

Dr Herbie — Wed, 19 Aug 2009 09:17:42 GMT

Could you elaborate more on smell #3? I don't understand your reference to 'closure' and 'shape' and the benefits they bring that make views better than stored procedures in this case.

Note, that I'm a C# developer who writes SQL and not a trained DBA, so there are a lot of concepts that I'm missing ...

Thanks

Herbie

re: Top 10 T-SQL Code Smells

Andrew Wickham — Wed, 19 Aug 2009 12:16:31 GMT

Rule #7.

I think Rule #7 can be justified from an application standpoint. Sometimes "locked" isn't so much as the row is being updated, but maybe someone actually went into that record in the application and is editing it there. Example...

Person 1 opens record 200

Person 2 opens record 200

Person 1 changed Name

Person 2 now has stale data in their form and changes Address

Person 1 saves changes

Person 2 saves changes

Person 1's changes were lost, because Person 2's changes were submitted with the stale data. There are probably better ways of taking care of this, but locking someone out of the record when someone else is in it is a relatively easy way to handle this. When Person 1 opens the record it updates the row and sets it to locked. Person 2 then tries to go into the same record and it says "Sorry, this row is already being edited."

Let me know what you guys think. Like I said, maybe there is a better way of doing that.

re: Top 10 T-SQL Code Smells

Alexander Kuznetsov — Wed, 19 Aug 2009 13:27:13 GMT

Merrill,

I loved reading most of the post, it's a great one. Yet I cannot agree with #9 "Code having no error handling flow control or transactions." Because error handling in T-SQL is a bad joke, it is full of loopholes, in many cases it makes a lot of sense to handle errors in another layer developed in a better language.

Regarding views as the alternative to stored procedures, for me inline UDFs are an even better alternative as the take parameters yet still are macros just like views.

re: Top 10 T-SQL Code Smells

Adam Machanic — Wed, 19 Aug 2009 14:47:58 GMT

Agreed with Andrew Wickham. SQL Server has no built in support for meta-transactional locks (i.e., at the business process level). This is something that you MUST roll yourself. There are better and worse ways to do it, of course. Blatant plug: You might want to refer to my chapter on the topic in "Expert SQL Server 2005 Development", which shows the methods I've come up with for solving the various problems that can crop up when you don't think through the solution carefully enough.

re: Top 10 T-SQL Code Smells

merrillaldrich — Wed, 19 Aug 2009 15:29:03 GMT

Thanks, guys, for the feedback. Andrew and Adam - with true "business side" locks I can see your point, and that'd definitely be justified. However, I just got through locating a bug in a vendor-supplied app where they were doing a standard "sequence table," and instead of using a transaction to lock the underlying row, they used a flag in a column. It was slow, and painful to watch, and later, unsurprisingly, became a concurrency bug. So, I still would call the "roll-your-own locks" notion something to watch out for (smell) but not something that should never be done. Maybe I loaded on the sarcasm too thick in the post :-).

re: Top 10 T-SQL Code Smells

merrillaldrich — Wed, 19 Aug 2009 15:54:27 GMT

Dr Herbie: what I mean about closure is

This doesn't work:

select i.item, m.bar

from dbo.items i

inner join exec dbo.myProc m on i.something = m.something

where m.somethingelse = 'FOO'

But this does:

select i.item, m.bar

from dbo.items i

inner join dbo.myView m on i.something = m.something

where m.somethingelse = 'FOO'

The reason is that a view's "output" is the same as a table's "output" -- one result set with known columns -- where a proc can spit out just about anything, or even different things depending on conditions, etc. That means views can be used in a select statement just like tables, but procs, even if they contain only a select statement, cannot.

re: Top 10 T-SQL Code Smells

merrillaldrich — Wed, 19 Aug 2009 16:03:04 GMT

Alexander - check my understanding: do you mean that transactions and error handling should be used, but that it's more convenient / safer to use a client-side library (C#, java, whatever) to actually write the code? I can definitely see that. In that case, a profiler trace (I think) still shows transaction boundaries and so on, as that client library directs the action at the server, and the error handling is present, just in the app code and not in raw T-SQL. What I was going after in terms of smell is those systems that lack error handling or transactions altogether...

I probably should have said, "if you don't see transactions or error handling in the DB, make certain your app is doing that work instead."

re: Top 10 T-SQL Code Smells

Alexander Kuznetsov — Wed, 19 Aug 2009 18:24:23 GMT

Merrill, yes I meant "it's more convenient / safer to use a client-side library (C#, java, C++) to actually write the error handling code".

re: Top 10 T-SQL Code Smells

Bob — Wed, 19 Aug 2009 19:16:38 GMT

Question about #10. What's your opinion about creating temp tables in a sproc so that you can index it and reuse it in downstream code. I frequently find that this can produce faster results than subqueries.

re: Top 10 T-SQL Code Smells

Al Tenhundfeld — Wed, 19 Aug 2009 19:53:29 GMT

11. READ UNCOMMITTED.

While can be useful in debugging, if your production code needs to read uncommitted transactions, something is probably wrong.

And re: 7.Roll-your-own locking, I'm not sure what you're advocating. A business transaction is different from a SQL transaction. For example, I might want to "lock" a support ticket while a user edits it, which may take minutes to hours. I don't want anyone else to edit that ticket, but I certainly don't want to leave a SQL transaction open that long.

re: Top 10 T-SQL Code Smells

merrillaldrich — Wed, 19 Aug 2009 21:22:08 GMT

Bob - agreed that temp tables _can_ be a performance optimization for some scenarios; to me the issue is more around use of INSERT #temp ... EXEC constantly as a default design pattern, or people who always use temp tables out of habit, where it hurts performance. #Tables usually add overhead, and only sometimes make up for that overhead, plus provide net gain in performance.

I think it's better to apply optimizations selectively, when you have a problem or when you can really predict you will have a problem. So I would look at a specific case, see if a basic query would work and scale OK, then use a temp table only if needed and really helpful. In any case, most of these Top 10 are not hard-and-fast rules, they are just symptoms to watch out for.

Al - exactly right. I worry when a SQL transaction was needed and appropriate, but was missing, which is different than the type of business-related "locking" that you and Adam and Andrew describe. It'd be handy if there were a word other than "lock" for that. "Meta-transactional locks? (Adam)

re: Top 10 T-SQL Code Smells

Bob — Thu, 20 Aug 2009 01:23:30 GMT

Well stated. I probably use them more often than needed. Often it's to simplify the logic: "Rule of Clarity", etc

The majority of my work is with data warehouse loads where 2 minute execution vs 4 minute execution is trivial.

Side note: I came to database programming (15 years ago?) from OO programing and attacked many problems with loops, I simply wasn't thinking in set operations. Some of the awful code I produced still wakes me up at night. It took about 6 months for set operations to click. Sadly, a power company in the midwest is probably still saddled with my stench!

re: Top 10 T-SQL Code Smells

Larry Leonard — Sat, 22 Aug 2009 00:06:13 GMT

Excellent! However, "I also dislike hungarian notation (dbo.tbl_MyTable, dbo.vw_trans), but if I am honest I have to admit that is really a personal stylistic preference."? Dude?

Have you ever worked in a database where all the names are common words like "Transaction", "Invoice', and my personal favorite, "[Group]". In my current world, the tables all begin with "tbl", and thank God they do - makes grepping a whole lot easier!

re: Top 10 T-SQL Code Smells

merrillaldrich — Sat, 22 Aug 2009 21:41:10 GMT

tblTransaction, tblInvoice and tblGroup? ;-)

re: Top 10 T-SQL Code Smells

Phil Factor — Sun, 23 Aug 2009 15:23:36 GMT

This is an excellent start to establishing list of SQL Code-smells. The reason I like the concept is that it is less prescriptive than the term 'best practice'. A bad smell in the fridge always needs investigation even if it turns out to be ripe Camembert cheese. I must admit that a some of my code has a bit of a whiff about it, but I always bristle a bit when I'm told it does not 'conform with Best-practice'.

re: Top 10 T-SQL Code Smells

pjones — Mon, 24 Aug 2009 09:37:52 GMT

There's a simple reason for number 3 - to keep developers within limits and remove sql select statements and injection vulnerability from their code.

Best practice for development is to use stored procs and a data access layer. Don't discourage them, a few extra stored procs are far less hassle than fixing the results of a sql injection attack!

re: Top 10 T-SQL Code Smells

AaronBertrand — Mon, 24 Aug 2009 13:24:03 GMT

Larry, I can understand why you would have a hard time grepping for references to a table named "transaction" or "group" since those words will appear in many procedures that have nothing to do with either table. But what if you used a different but common naming technique, using a collective name for the tables (Transactions, Groups)? A table is a collection of things and very rarely contains only one row. I am not saying that it is better for everyone but it certainly solves your grepping problem. Another is to ensure that you ALWAYS specify the schema prefix when referencing a table. In that case also, grepping for "dbo.Transaction" shouldn't yield any false positives - unless you also have "dbo.TransactionDetails" etc. In which case combining these practices would further ease your grepping woes: "dbo.Transactions" and "dbo.TransactionDetails"...

Seems like a poor excuse to add an otherwise utterly useless prefix to every single table. If it looks and smells like a table, you do not need to name it tblTable.

re: Top 10 T-SQL Code Smells

merrillaldrich — Mon, 24 Aug 2009 16:23:59 GMT

pjones - respectfully, I hate SQL injection as much as the next guy, but please keep in mind that it's handling parameters correctly that really prevents SQL injection. Using stored procs happens to favor the use of parameters, a fact which seems to have morphed into the idea that stored procs themselves prevent SQL injection. It's parameters that do the work and keep things safe. There are many safe ways to parameterize queries. (I don't want to get into the whole sprocs argument, as it's a discussion that's already completely played out elsewhere, and there's not much new to add.)

re: Top 10 T-SQL Code Smells

AaronBertrand — Mon, 24 Aug 2009 16:29:40 GMT

Merrill, very true. You can't just move unsafe code into a stored procedure, and it magically becomes "safe". I have often seen the following approach when people are told to get the SQL queries out of their application code and use stored procedures.

SQLstr = "SELECT * FROM bar " + WhereClause;

...will often become...

CRETAE PROCEDURE dbo.foo

@WhereClause NVARCHAR(MAX)

BEGIN

SET NOCOUNT ON;

EXEC('SELECT * FROM bar ' + @WhereClause);

END

Parameters, Perms and Procs: Are You Really Protected from Injection?

Merrill Aldrich — Mon, 24 Aug 2009 22:01:31 GMT

In my last post, Top 10 T-SQL Code Smells , I caught some flack got some feedback for including one (#3)

re: Top 10 T-SQL Code Smells

David Russ — Tue, 25 Aug 2009 16:51:20 GMT

My approach to the business workflow type of locking, I typically have a timestamp column that I read and store the byte value when the application opens the "record". Then when the employ saves, I check the record's timestamp column and then compare it to my stored byte array. If they are different, then I warn the employee that the record has been modified outside of their "screen". This gives them a heads up.

re: Top 10 T-SQL Code Smells

Bob — Tue, 16 Mar 2010 18:08:01 GMT

Optimizer hints are also bad. That's all code that needs to be rechecked every time SQL is updated. Especially pervasive is select * from x (WITH NOLOCK)

All Those Extra DR Bits, Part 1.5

Merrill Aldrich — Fri, 26 Nov 2010 22:17:23 GMT

I got some positive feedback on the last post, All Those Extra Bits We Need for DR , so I wanted to put

re: Top 10 T-SQL Code Smells

GSquared — Tue, 30 Nov 2010 14:38:07 GMT

On the "business locks" vs "transaction locks", I prefer to use the term "MUTEX" for the business locks, to avoid the confusion on "locks". A flag or timestamp of some sort on a row in a table, indicating to other resources trying to access that data that it's in-use, definitely falls under the heading of a mutex.

Understanding how they work, and what to watch out for (things like rows that never get let go because of an incomplete or failed action), are all covered under the general programming concepts for mutexes.

(Yes, this is a bit late to the game. Ended up here off of an article today on Simple-Talk.com.)

2010: It’s a Wrap

Merrill Aldrich — Sat, 01 Jan 2011 01:27:11 GMT

So, last day of the year, and I can see many people are in a reflective mood. I don’t usually deep dive

T-SQL Tuesday 24: Ode to Composable Code

Merrill Aldrich — Tue, 08 Nov 2011 06:54:09 GMT

I love the T-SQL Tuesday tradition, started by Adam Machanic and hosted this month by Brad Shulz . I