I feel like I should preface this with a disclaimer. I added “potentially-” to the title, because there are many queries that might seem bad but can actually perform just fine. There are queries that on the surface can be great, but are nasty without a particular index, and there are queries that make me cringe a little when looking at them, but are actually okay. Brent Ozar is asking about signs of bad code for this month’s T-SQL Tuesday (the 200th – and I have a response for all 200 if you look back through my history of posts), and he wants us to write this for 2004 Brent, rather than 2026 Brent.
2004 was a different time for T-SQL. For example, we could still write joins using commas, putting the predicates in the WHERE clause. This still works today, but back then if we wanted to do an outer join, we would join using a special operator, such as “WHERE s.ProductID *= p.ProductID”. Code like this was deprecated in SQL Server 2005, and stopped working completely in SQL Server 2012.
So let’s start with that one. Hey Brent04, stop doing joins that way. Embrace the ON clause. It won’t make your code work any better, but it will stop it from breaking in a few years’ time. Plus, you can do more with it, having control about which join predicates are used for joining which tables. If you do find yourself wanting to use a comma, write out CROSS JOIN instead. I know it’s more typing, but you’ll make better choices.
But now that we’re talking about joins – start using aliases, and don’t just refer to the first table as ‘a’, the second as ‘b’, and so on. It doesn’t change the performance, but I will judge you. If you’re only using aliases when you refer to a table twice in a query, then I’m going to think you write your queries using Query Builder, and that’s something you should move away from (Query Builder will limit you, and can even make mistakes, so don’t use it – take the time to learn to write queries without it). Without aliases, at some point you’ll look back at your code, and when you see something like: “WHERE a.Col1 > 3 AND Val < 7”, you won’t know which table the “Val” column is in, and you’ll need to keep looking back at the FROM clause to see which table is ‘a’. I’ve seen code like “FROM dbo.Bank as a JOIN dbo.Account as b ON a.BankID = b.BankID”, and that’s going to trip up anyone.
Notice how I used schemas there? Yeah, you should specify the schemas. Don’t be lazy. We all do it from time to time, especially in databases where everything is in dbo – but at some point you’ll regret it. Plus, it’s slightly more work for the database engine if you don’t tell it the schema, as it has to check what your default schema is, and see if that object is in there. But more importantly, when your code runs as someone who has a different default schema, it might do something different.
Another pattern that will really make me cringe, Brent04, is when a procedure contains lots of UPDATE statements. I’m sure you know what I mean – this kind of thing:
UPDATE #WorkingTable SET Col2 = Col2 + 1 WHERE Col3 BETWEEN 5 and 10;
UPDATE #WorkingTable SET Col3 = 11 WHERE Col2 > 10;
UPDATE #WorkingTable SET Col2 = Col2 * 2 WHERE Col3 > 10;
–followed by dozens of similar lines
This code tends to have been created by people who have a process they need to follow. A recipe. And they can get it working, but it becomes huge and error-prone, and no one wants to debug it. When someone works out the logic isn’t right – they tack more statements onto the end.
Now, I have my ways of handling this. I start by using a lot of CROSS APPLY clauses to create each new version of the table, with CASE statements to replace the WHERE clauses. It probably looks even more ugly than the list of UPDATE statements, but hopefully does everything in one fell swoop, and after testing the results and tweaking until it’s correct, I can look at the expressions that get used in the plan to figure out what the finished result should be. Worst case, I leave it as the CASE-statement version, which is almost certainly quicker than the series of UPDATE statements, which might look something like this:
UPDATE v0 SET Col1 = v3.Col1, Col2 = v3.Col2, Col3 = v3.Col3
FROM #WorkingTable v0
CROSS APPLY (SELECT v0.Col1, CASE WHEN v0.Col3 BETWEEN 5 AND 10 THEN v0.Col2 + 1 ELSE v0.Col2 END as Col2, v0.Col3) v1
CROSS APPLY (SELECT v1.Col1, v1.Col2, CASE WHEN v1.Col2 > 10 THEN 11 ELSE v1.Col3 END as Col3) v2
CROSS APPLY (SELECT v2.Col1, CASE WHEN v2.Col3 > 10 THEN v2.Col2 * 2 ELSE v2.Col2 END as Col2, v2.Col3) v3
;
If you do this, make sure each CROSS APPLY clause can return exactly one row for each row in the base table (for example, use an aggregate if there’s a FROM clause, or OUTER APPLY and TOP (1), or don’t use a FROM clause and have no filter like in my example).
So, Brent04… you know I could keep writing for a while here. I’m going to suggest that you don’t beat yourself up from all the advice you’ll get from this T-SQL Tuesday event. Bad code exists, and it takes time to get rid of bad habits. Do what works for you. Understand that correctness is more important than performance, and that you need to be coding within your means. If someone (even me) has recommending a method you don’t understand, seek to understand it before you start using it. I see a lot of people who have tried to use patterns they’ve been told are good, but do it wrong and end up with an even bigger mess.
Good luck Brent04 – I guess I’ll see you in about 5 years. I think it was 2009 when we met in person.
@robfarley.com@bluesky (previously @rob_farley@twitter)
