{"id":95486,"date":"2023-02-23T16:49:32","date_gmt":"2023-02-23T16:49:32","guid":{"rendered":"https:\/\/www.red-gate.com\/simple-talk\/?p=95486"},"modified":"2026-03-09T12:45:39","modified_gmt":"2026-03-09T12:45:39","slug":"40-problems-sql-server-stored-procedure","status":"publish","type":"post","link":"https:\/\/www.red-gate.com\/simple-talk\/databases\/sql-server\/t-sql-programming-sql-server\/40-problems-sql-server-stored-procedure\/","title":{"rendered":"40 Common Stored Procedure Problems in SQL Server"},"content":{"rendered":"<section><em>This article reviews a realistic SQL Server stored procedure and identifies 40 problems &#8211; from missing SET NOCOUNT ON and inconsistent naming to sargability issues, improper error handling, and inefficient query patterns. Each problem is explained with the fix and the reasoning behind it. <\/em><\/section>\n<section><\/section>\n<section><em>Whether you\u2019re writing stored procedures or reviewing someone else\u2019s code, this serves as a practical checklist for SQL Server T-SQL coding standards. The problems cover naming conventions, parameter handling, temp table usage, date functions, schema qualification, SET vs. SELECT for variables, UNION vs. UNION ALL, and execution plan analysis.<\/em><\/section>\n<section><\/section>\n<section id=\"bertrand-blog-post\">I&#8217;ve been at this for a while now, and have a very particular set of rules and coding conventions that I follow when writing and, more importantly, <em>reviewing<\/em> T-SQL code. I often perform code reviews and thought it would be fun to frame this exercise: a completely fictitious stored procedure hits my desk, I&#8217;ll reject it of course, and enumerate the reasons why. The procedure might pass review in a lot of shops, but not in mine.\n<p>We&#8217;re going to use the AdventureWorks sample database (<a href=\"https:\/\/learn.microsoft.com\/en-us\/sql\/samples\/adventureworks-install-configure\" target=\"_blank\" rel=\"noopener\">get your copy here<\/a>), where the folks in marketing requested a list of users to e-mail a new promotional campaign. The customers need to meet at least one of the following criteria:<\/p>\n<ul style=\"line-height: 1.125rem;\" type=\"square\">\n<li>last placed an order more than a year ago<\/li>\n<li>placed 3 or more orders in the past year<\/li>\n<li>have ordered from a specific category in the past two weeks<\/li>\n<\/ul>\n<p>These criteria don&#8217;t have to make sense! They just need to make the query a little bit more complex than your average <a href=\"https:\/\/www.red-gate.com\/simple-talk\/databases\/sql-server\/t-sql-programming-sql-server\/sql-server-crud-generation-from-system-views\/\" target=\"_blank\" rel=\"noopener\">CRUD operations<\/a>.<\/p>\n<p>First, we&#8217;re going to update <code style=\"font-family: consolas; background: #e4e4e4; padding: 3px 4px 1px 4px; border-radius: 3px;\">Sales.SalesOrderHeader<\/code> to modern times so that dates make sense relative to today. We only care about <code style=\"font-family: consolas; background: #e4e4e4; padding: 3px 4px 1px 4px; border-radius: 3px;\">OrderDate<\/code> here, but there are check constraints that protect a couple of other columns (as well as a trigger that <em>sometimes<\/em> fails on <a href=\"https:\/\/dbfiddle.uk\/7ka9tZni\" target=\"_blank\" rel=\"noopener\">db&lt;&gt;fiddle<\/a> but that I have no energy to troubleshoot):<\/p>\n<pre class=\"theme:tomorrow-night font:consolas font-size:14 line-height:16 decode-attributes:false tab-convert:true lang:tsql decode:true\">DISABLE TRIGGER Sales.uSalesOrderHeader ON Sales.SalesOrderHeader;\nGO\n\nDECLARE @months int;\n\nSELECT @months = DATEDIFF(MONTH, MAX(OrderDate), GETDATE()) \n    FROM Sales.SalesOrderHeader;\n\nUPDATE Sales.SalesOrderHeader SET DueDate   = DATEADD(MONTH, @months-1, DueDate);\nUPDATE Sales.SalesOrderHeader SET ShipDate  = DATEADD(MONTH, @months-1, ShipDate);\nUPDATE Sales.SalesOrderHeader SET OrderDate = DATEADD(MONTH, @months-1, OrderDate);\nGO\n\nENABLE TRIGGER Sales.uSalesOrderHeader ON Sales.SalesOrderHeader;<\/pre>\n<p>This stored procedure that someone wrote will now return data (without the update, it would be hard to write predictable queries based on, say, some offset from <code style=\"font-family: consolas; background: #e4e4e4; padding: 3px 4px 1px 4px; border-radius: 3px;\">GETDATE()<\/code>).<\/p>\n<pre class=\"theme:tomorrow-night font:consolas font-size:14 line-height:16 nums:true nums-toggle:false decode-attributes:false tab-convert:true lang:tsql decode:true \">create proc sp_find_customers(@paramIntCategoryId INT)\nas\nselect customerid, firstname, emailaddress from\n(\n  -- last placed an order more than a year ago\n\n  select distinct customerid, firstname, emailaddress\n  from sales.customer a (nolock), person.person b(nolock), \n  person.emailaddress c(nolock) where personid = b.businessentityid \n  and b.businessentityid = c.businessentityid and (select max(orderdate) \n  from sales.salesorderheader (nolock) \n  where customerid = a.customerid) &lt; dateadd(yyyy,-1,convert(DATE,getdate()))\n\nunion -- placed at least 3 orders in the past year\n\n  select distinct customerid, firstname, emailaddress \n  from person.person p (nolock) join person.emailaddress e (nolock)\n  on p.businessentityid = e.businessentityid\n  join sales.customer c (nolock) on personid = p.businessentityid\n  where customerid in (select customerid from sales.salesorderheader (nolock)\n  where orderdate between dateadd(yy, -1, convert(DATE,getdate())) and getdate()\n  group by customerid having count(*) &gt;= 3)\n\nunion -- ordered within specified category in past two weeks\n\n  select distinct customerid, firstname, emailaddress \n  from person.emailaddress em (nolock) join person.person pp with (nolock)\n  on em.businessentityid=pp.businessentityid\n  join sales.customer cu (nolock) on personid = pp.businessentityid\n  where customerid in (select top (2147483647) customerid \n  from sales.salesorderheader oh (nolock)\n  join sales.salesorderdetail od (nolock) on oh.salesorderid = od.salesorderid\n  and datediff(day, oh.orderdate, convert(DATE,getdate())) &lt;= 14\n  join production.product pr (nolock) on od.productid = pr.productid\n  inner join production.productsubcategory AS sc (nolock)\n  on pr.productsubcategoryid = sc.productsubcategoryid\n  and sc.productcategoryid = @paramIntCategoryId order by customerid)\n) \nx \norder by 1<\/pre>\n<p>You laugh. Or maybe you cry. But I do see all of this, all the time, just maybe not always all at the same time.<\/p>\n<p>While the stored procedure itself is only 40 lines, I&#8217;m sure a quick skim will reveal some things you don&#8217;t like. But do you think I can find <em>40 problems?<\/em><\/p>\n<h3>Challenge Accepted.<\/h3>\n<p>Before we start, I can&#8217;t emphasize enough that just about <em>any<\/em> convention in T-SQL is completely subjective, and you may or may not agree with any of mine. And that&#8217;s okay. Still, I&#8217;ll tell you the problems I spotted, line by line, and why I think they&#8217;re problems (with helpful links to more details, where appropriate).<\/p>\n<div style=\"width: 100px!important; text-align: center; color: white; background: #c00; border-radius: 5px; font-weight: bold; padding: 3px 6px; margin-bottom: 12px;\">Line 1<\/div>\n<pre class=\"theme:tomorrow-night font:consolas font-size:14 line-height:16 nums:true nums-toggle:false decode-attributes:false tab-convert:true lang:tsql decode:true start-line:1\">create proc sp_find_customers(@paramIntCategoryId INT)<\/pre>\n<p>On the very first line, there are multiple problems:<\/p>\n<ol class=\"bad\">\n<li><b style=\"color: #c00;\">All lower case keywords<\/b> &#8211; I&#8217;m only going to mention this once, but reading a wall of code in all lower case (or all upper case!) is hard on the eyes. Personally, I like upper casing T-SQL keywords, Pascal case for entity names, and lower case for data types (to be mentioned in a moment).<\/li>\n<li><b style=\"color: #c00;\">Shorthand<\/b> &#8211; The abbreviation <code style=\"font-family: consolas; background: #e4e4e4; padding: 3px 4px 1px 4px; border-radius: 4px;\">proc<\/code> is grating to me because people start to <em>talk<\/em> that way, too. &#8220;Can you create a prock?&#8221; It&#8217;s a procedure; just spell it out.<\/li>\n<li><b style=\"color: #c00;\">Missing schema prefix<\/b> &#8211; Leaving out the schema <a href=\"https:\/\/sqlblog.org\/2019\/09\/12\/bad-habits-to-kick-avoiding-the-schema-prefix\" target=\"_blank\" rel=\"noopener\">can be problematic<\/a> both for performance and for other users and maintainers. While the performance issue is less relevant during object <em>creation<\/em>, it&#8217;s a mindset and we should all be consistent.<\/li>\n<li><b style=\"color: #c00;\">Bad procedure name<\/b> &#8211; Not just the <code style=\"font-family: consolas; background: #e4e4e4; padding: 3px 4px 1px 4px; border-radius: 3px;\">sp_<\/code> prefix, which <a href=\"https:\/\/sqlperformance.com\/2012\/10\/t-sql-queries\/sp_prefix\" target=\"_blank\" rel=\"noopener\">should be avoided<\/a> but, also, what does &#8220;<code style=\"font-family: consolas; background: #e4e4e4; padding: 3px 4px 1px 4px; border-radius: 3px;\">find_customers<\/code>&#8221; mean? This name should be more specific to the task at hand, and also match your existing naming scheme. Something like <code style=\"font-family: consolas; background: #e4e4e4; padding: 3px 4px 1px 4px; border-radius: 3px;\">Customer_GetListForCampaignXYZ<\/code>, for example. (Generally, I prefer <code style=\"font-family: consolas; background: #e4e4e4; padding: 3px 4px 1px 4px; border-radius: 3px;\">entity_action<\/code>, like <code style=\"font-family: consolas; background: #e4e4e4; padding: 3px 4px 1px 4px; border-radius: 3px;\">Customer_Find<\/code>, over <code style=\"font-family: consolas; background: #e4e4e4; padding: 3px 4px 1px 4px; border-radius: 3px;\">action_entity<\/code>, like <code style=\"font-family: consolas; background: #e4e4e4; padding: 3px 4px 1px 4px; border-radius: 3px;\">Find_Customer<\/code>, primarily due to sorting and grouping. I&#8217;m very rarely looking for all the stored procedures that find <em>anything<\/em>.)<\/li>\n<li><b style=\"color: #c00;\">Parentheses for nothing<\/b> &#8211; It&#8217;s a minor gripe, but I don&#8217;t like adding extra control characters for the sake of it. Stored procedure parameter lists do not <em>need<\/em> to be enclosed in parentheses, so add them only if your team agrees that they actually make the procedure more readable.<\/li>\n<li><b style=\"color: #c00;\">Bad parameter name (i)<\/b> &#8211; It doesn&#8217;t need to start with <code style=\"font-family: consolas; background: #e4e4e4; padding: 3px 4px 1px 4px; border-radius: 3px;\">@param<\/code> &#8211; that is obvious from where it is and, later in the procedure, the source isn&#8217;t likely to be important anyway.<\/li>\n<li><b style=\"color: #c00;\">Bad parameter name (ii)<\/b> &#8211; It also doesn&#8217;t need to be prefixed with its data type &#8211; that is also obvious from the definition, and naming it this way locks it in (making future changes to that data type harder).<\/li>\n<li><b style=\"color: #c00;\">Bad parameter name (iii)<\/b> &#8211; In AdventureWorks, the IDs are upper case, so it should be <code style=\"font-family: consolas; background: #e4e4e4; padding: 3px 4px 1px 4px; border-radius: 3px;\">@CategoryID<\/code> just to be consistent and avoid any confusion or ambiguity.<\/li>\n<li><b style=\"color: #c00;\">Upper case data type<\/b> &#8211; I learned <a href=\"https:\/\/www.mssqltips.com\/sqlservertip\/6989\/sql-server-data-types-handling-errors\/?utm_source=AaronBertrand\" target=\"_blank\" rel=\"noopener\">the hard way<\/a> the best pattern for data types is to match the case in <code style=\"font-family: consolas; background: #e4e4e4; padding: 3px 4px 1px 4px; border-radius: 3px;\">sys.types<\/code> exactly, which is arguably more important if you ever deal with case sensitive databases or instances. While it doesn&#8217;t affect the basic native types, for consistency, this means you would use <code style=\"font-family: consolas; background: #e4e4e4; padding: 3px 4px 1px 4px; border-radius: 3px;\">int<\/code>, not <code style=\"font-family: consolas; background: #e4e4e4; padding: 3px 4px 1px 4px; border-radius: 3px;\">INT<\/code>.<\/li>\n<li><b style=\"color: #c00;\">Everything on one line<\/b> &#8211; For formatting purposes I kept the parameter list short, but I&#8217;ve seen lists of 10+ parameters scroll way off the screen to the right, and I don&#8217;t have to tell you what a headache that is during a fire drill. Strive to be more liberal with white space &#8211; the goal shouldn&#8217;t be to keep the line count short at the cost of readability.<\/li>\n<\/ol>\n<div style=\"width: 100px!important; text-align: center; color: white; background: #c00; border-radius: 5px; font-weight: bold; padding: 3px 6px; margin-bottom: 12px;\">Line 2<\/div>\n<pre class=\"theme:tomorrow-night font:consolas font-size:14 line-height:16 nums:true nums-toggle:false decode-attributes:false tab-convert:true lang:tsql decode:true start-line:2\">as<\/pre>\n<p>It&#8217;s only two characters, but it&#8217;s more about what&#8217;s missing here:<\/p>\n<ol start=\"11\">\n<li><b style=\"color: #c00;\">No BEGIN\/END<\/b> &#8211; I prefer to wrap my procedure body in clear block elements that are even more explicit about where the procedure ends than indentation and the (non-T-SQL!) <code style=\"font-family: consolas; background: #e4e4e4; padding: 3px 4px 1px 4px; border-radius: 3px;\">GO<\/code> batch separator could ever be.<\/li>\n<li><b style=\"color: #c00;\">No SET NOCOUNT<\/b> &#8211; I always add <code style=\"font-family: consolas; background: #e4e4e4; padding: 3px 4px 1px 4px; border-radius: 3px;\">SET NOCOUNT ON;<\/code> to the beginning of every stored procedure to suppress <a href=\"https:\/\/www.red-gate.com\/hub\/product-learning\/sql-prompt\/finding-code-smells-using-sql-prompt-set-nocount-problem-pe008-pe009\" target=\"_blank\" rel=\"noopener\">all of those chatty <code style=\"font-family: consolas; background: #e4e4e4; padding: 3px 4px 1px 4px; border-radius: 3px;\">DONE_IN_PROC<\/code> messages<\/a>. While in modern versions of SQL Server and various transport layers the benefits <a href=\"https:\/\/sqlperformance.com\/2016\/02\/t-sql-queries\/nocount\" target=\"_blank\" rel=\"noopener\">aren&#8217;t as pronounced<\/a> as they used to be, the extra chatter still can&#8217;t possibly help anything.<\/li>\n<\/ol>\n<div style=\"width: 100px!important; text-align: center; color: white; background: #c00; border-radius: 5px; font-weight: bold; padding: 3px 6px; margin-bottom: 12px;\">Line 3<\/div>\n<pre class=\"theme:tomorrow-night font:consolas font-size:14 line-height:16 nums:true nums-toggle:false decode-attributes:false tab-convert:true lang:tsql decode:true start-line:3\">select customerid, firstname, emailaddress from<\/pre>\n<p>Only one new problem here:<\/p>\n<ol start=\"13\">\n<li><b style=\"color: #c00;\">Lower case columns<\/b> &#8211; In the metadata, those columns are named <code style=\"font-family: consolas; background: #e4e4e4; padding: 3px 4px 1px 4px; border-radius: 3px;\">CustomerID<\/code>, <code style=\"font-family: consolas; background: #e4e4e4; padding: 3px 4px 1px 4px; border-radius: 3px;\">FirstName<\/code>, and <code style=\"font-family: consolas; background: #e4e4e4; padding: 3px 4px 1px 4px; border-radius: 3px;\">EmailAddress<\/code>. Every single reference to them should match.<\/li>\n<\/ol>\n<div style=\"width: 100px!important; text-align: center; color: white; background: #c00; border-radius: 5px; font-weight: bold; padding: 3px 6px; margin-bottom: 12px;\">Line 5<\/div>\n<pre class=\"theme:tomorrow-night font:consolas font-size:14 line-height:16 nums:true nums-toggle:false decode-attributes:false tab-convert:true lang:tsql decode:true start-line:5\">  -- last placed an order more than a year ago<\/pre>\n<ol start=\"14\">\n<li><b style=\"color: #c00;\">Single-line comments<\/b> &#8211; While it&#8217;s actually refreshing to see comments inside of queries, not all comments are the same. As Brent Ozar explains in <a href=\"https:\/\/www.brentozar.com\/archive\/2021\/04\/never-ever-ever-start-t-sql-comments-with-two-dashes\/\" target=\"_blank\" rel=\"noopener\">Never, Ever, Ever Start T-SQL Comments with Two Dashes<\/a>, single-line comments cause many problems downstream &#8211; for example, when reviewing queries from the plan cache or monitoring tools, or even returning the definition to a grid in Management Studio. I&#8217;m far from perfect, especially in quick throwaway fiddles, but I&#8217;ve been trying harder to always use <code style=\"font-family: consolas; background: #e4e4e4; padding: 3px 4px 1px 4px; border-radius: 3px;\">\/* this form *\/<\/code>. Contradicting Brent a little bit, Andy Mallon has a great productivity use case of combining the two forms <a href=\"https:\/\/github.com\/amtwo\/Presentations\/blob\/production\/Shortcuts%20From%20an%20Impatient%20DBA\/Shortcuts%20from%20an%20Impatient%20DBA.pptx\" target=\"_blank\" rel=\"noopener\">in slide 28 of this presentation<\/a>.<\/li>\n<\/ol>\n<div style=\"width: 100px!important; text-align: center; color: white; background: #c00; border-radius: 5px; font-weight: bold; padding: 3px 6px; margin-bottom: 12px;\">Line 7<\/div>\n<pre class=\"theme:tomorrow-night font:consolas font-size:14 line-height:16 nums:true nums-toggle:false decode-attributes:false tab-convert:true lang:tsql decode:true start-line:7\">  select distinct customerid, firstname, emailaddress<\/pre>\n<ol start=\"15\">\n<li><b style=\"color: #c00;\">Unnecessary DISTINCT<\/b> &#8211; This query is part of a <code style=\"font-family: consolas; background: #e4e4e4; padding: 3px 4px 1px 4px; border-radius: 3px;\">UNION<\/code> which &#8211; by default, and possibly not commonly known &#8211; has to perform a <code style=\"font-family: consolas; background: #e4e4e4; padding: 3px 4px 1px 4px; border-radius: 3px;\">DISTINCT<\/code> against the output of the concatenation anyway, so doing so inside any of the inputs to the <code style=\"font-family: consolas; background: #e4e4e4; padding: 3px 4px 1px 4px; border-radius: 3px;\">UNION<\/code> is redundant. This query is littered with these and they can <em>all<\/em> be avoided &#8211; typically they are used to suppress duplicate rows from joins that should have been implemented as <code style=\"font-family: consolas; background: #e4e4e4; padding: 3px 4px 1px 4px; border-radius: 3px;\">EXISTS<\/code> checks instead and\/or deferring the joins until later.<\/li>\n<\/ol>\n<div style=\"width: 100px!important; text-align: center; color: white; background: #c00; border-radius: 5px; font-weight: bold; padding: 3px 6px; margin-bottom: 12px;\">Line 8<\/div>\n<pre class=\"theme:tomorrow-night font:consolas font-size:14 line-height:16 nums:true nums-toggle:false decode-attributes:false tab-convert:true lang:tsql decode:true start-line:8\">  from sales.customer a (nolock), person.person b(nolock),<\/pre>\n<p>All right, now we&#8217;re getting warmed up! A few new problems here:<\/p>\n<ol start=\"16\">\n<li><b style=\"color: #c00;\">Lower case object names<\/b> &#8211; These tables are called <code style=\"font-family: consolas; background: #e4e4e4; padding: 3px 4px 1px 4px; border-radius: 3px;\">Sales.Customer<\/code> and <code style=\"font-family: consolas; background: #e4e4e4; padding: 3px 4px 1px 4px; border-radius: 3px;\">Person.Person<\/code>, and it&#8217;s important for code to match what is in the metadata since it may be destined for case sensitive environments. IntelliSense and other tools make missing this detail hard to excuse these days. If you have a case sensitive database, being cavalier about this can lead to problems, especially if production is case sensitive and your development environment is not. I&#8217;m a big advocate of <em>always<\/em> developing in case sensitive because surprises are easier to fix locally and up front than after they&#8217;ve been deployed.<\/li>\n<li><b style=\"color: #c00;\">Bad aliases<\/b> &#8211; Why is <code style=\"font-family: consolas; background: #e4e4e4; padding: 3px 4px 1px 4px; border-radius: 3px;\">customer<\/code> aliased as <code style=\"font-family: consolas; background: #e4e4e4; padding: 3px 4px 1px 4px; border-radius: 3px;\">a<\/code> and <code style=\"font-family: consolas; background: #e4e4e4; padding: 3px 4px 1px 4px; border-radius: 3px;\">person<\/code> as <code style=\"font-family: consolas; background: #e4e4e4; padding: 3px 4px 1px 4px; border-radius: 3px;\">b<\/code>? In longer and more complex queries, reverse engineering this bizarre (but common!) mapping pattern is a <a href=\"https:\/\/sqlblog.org\/2009\/10\/08\/bad-habits-to-kick-using-table-aliases-like-a-b-c-or-t1-t2-t3\" target=\"_blank\" rel=\"noopener\">recipe for migraines<\/a>. As an aside, I detest aliases that are added in this form without using the <code style=\"font-family: consolas; background: #e4e4e4; padding: 3px 4px 1px 4px; border-radius: 3px;\">AS<\/code> keyword &#8211; while optional, I think it gives an important visual cue.<\/li>\n<li><b style=\"color: #c00;\">Littering of NOLOCK<\/b> &#8211; This hint is not a magic go-faster switch; it is just bad news in general. Placing it next to every table reference throughout all queries is even worse. If you have some reason you can&#8217;t use <code style=\"font-family: consolas; background: #e4e4e4; padding: 3px 4px 1px 4px; border-radius: 3px;\">READ COMMITTED SNAPSHOT<\/code> (RCSI), at the very least set the isolation level once at the top of the procedure (and maybe even add a comment explaining why this is necessary). Then, when you come to your senses, it&#8217;s just one line to fix. (I collected a treasure trove of resources detailing the many perils of <code style=\"font-family: consolas; background: #e4e4e4; padding: 3px 4px 1px 4px; border-radius: 3px;\">NOLOCK<\/code> <a href=\"https:\/\/sqlblog.org\/nolock\" target=\"_blank\" rel=\"noopener\">here<\/a>.)<\/li>\n<li><b style=\"color: #c00;\">Old-style joins<\/b> &#8211; I still see these all the time, even though <a href=\"https:\/\/sqlblog.org\/2009\/10\/08\/bad-habits-to-kick-using-old-style-joins\" target=\"_blank\" rel=\"noopener\">better syntax replaced them<\/a> more than 30 years ago! <code style=\"font-family: consolas; background: #e4e4e4; padding: 3px 4px 1px 4px; border-radius: 3px;\">FROM a, b<\/code> is less desirable than explicit joins for a couple of reasons. One is that it forces you to group join criteria and filter predicates together (and an argument there that is only partially true is that it can invite unintended Cartesian products). The other is that it forces inconsistency because in SQL Server there is no longer a way to perform deterministic <em>outer<\/em> joins without switching to explicit <code style=\"font-family: consolas; background: #e4e4e4; padding: 3px 4px 1px 4px; border-radius: 3px;\">OUTER JOIN<\/code> syntax.<\/li>\n<\/ol>\n<div style=\"width: 100px!important; text-align: center; color: white; background: #c00; border-radius: 5px; font-weight: bold; padding: 3px 6px; margin-bottom: 12px;\">Line 9<\/div>\n<pre class=\"theme:tomorrow-night font:consolas font-size:14 line-height:16 nums:true nums-toggle:false decode-attributes:false tab-convert:true lang:tsql decode:true start-line:9\">  person.emailaddress c(nolock) where personid = b.businessentityid<\/pre>\n<ol start=\"20\">\n<li><b style=\"color: #c00;\">Missing alias<\/b> &#8211; Why is there no alias reference associated with <code style=\"font-family: consolas; background: #e4e4e4; padding: 3px 4px 1px 4px; border-radius: 3px;\">personid<\/code>? When writing the initial query, this might not be a problem, because you&#8217;d get an ambiguous column name error if it were. But what about when someone comes back later and adds another table to the query? Now they have to go fix <em>all<\/em> unreferenced columns and possibly go look up all the tables to unravel. I talked about how we should always reference all columns for <a href=\"https:\/\/sqlblog.org\/2022\/11\/08\/t-sql-tuesday-156-production-code\" target=\"_blank\" rel=\"noopener\">a recent T-SQL Tuesday<\/a>.<\/li>\n<\/ol>\n<div style=\"width: 100px!important; text-align: center; color: white; background: #c00; border-radius: 5px; font-weight: bold; padding: 3px 6px; margin-bottom: 12px;\">Line 10<\/div>\n<pre class=\"theme:tomorrow-night font:consolas font-size:14 line-height:16 nums:true nums-toggle:false decode-attributes:false tab-convert:true lang:tsql decode:true start-line:10\">  and b.businessentityid = c.businessentityid and (select max(orderdate) <\/pre>\n<p>There&#8217;s definitely a code smell starting to waft from this line:<\/p>\n<ol start=\"21\">\n<li><b style=\"color: #c00;\">Correlated subquery in a predicate<\/b> &#8211; Running an aggregate against another table as part of a <code style=\"font-family: consolas; background: #e4e4e4; padding: 3px 4px 1px 4px; border-radius: 3px;\">WHERE<\/code> clause has the potential to scan that other table completely <em>for every row<\/em> in the current query (which could be a cross product for all we know).<\/li>\n<li><b style=\"color: #c00;\">Subquery indentation<\/b> &#8211; Any time you introduce an <code style=\"font-family: consolas; background: #e4e4e4; padding: 3px 4px 1px 4px; border-radius: 3px;\">IN<\/code> or <code style=\"font-family: consolas; background: #e4e4e4; padding: 3px 4px 1px 4px; border-radius: 3px;\">EXISTS<\/code> or other type of correlated anything, you should use white space to make it clear something radical is happening here &#8211; start it on the next line, make the left margin wider, do <em>something<\/em> to make it obvious and easier to follow.<\/li>\n<\/ol>\n<div style=\"width: 100px!important; text-align: center; color: white; background: #c00; border-radius: 5px; font-weight: bold; padding: 3px 6px; margin-bottom: 12px;\">Line 12<\/div>\n<pre class=\"theme:tomorrow-night font:consolas font-size:14 line-height:16 nums:true nums-toggle:false decode-attributes:false tab-convert:true lang:tsql decode:true start-line:12\">  where customerid = a.customerid) &lt; dateadd(yyyy,-1,convert(DATE,getdate()))<\/pre>\n<ol start=\"23\">\n<li><b style=\"color: #c00;\">DATEPART shorthand<\/b> &#8211; I don&#8217;t even know if you could call this &#8220;shorthand,&#8221; as I certainly see no advantage to typing four letters (<code style=\"font-family: consolas; background: #e4e4e4; padding: 3px 4px 1px 4px; border-radius: 3px;\">yyyy<\/code>) instead of four <em>different<\/em> letters that form a self-documenting word (<code style=\"font-family: consolas; background: #e4e4e4; padding: 3px 4px 1px 4px; border-radius: 3px;\">year<\/code>). A shorthand habit here can lead to problems, e.g. play with date parts like <code style=\"font-family: consolas; background: #e4e4e4; padding: 3px 4px 1px 4px; border-radius: 3px;\">W<\/code> and <code style=\"font-family: consolas; background: #e4e4e4; padding: 3px 4px 1px 4px; border-radius: 3px;\">Y<\/code>. I talk about abusing shorthand and several other date-related topics in <a href=\"https:\/\/sqlblog.org\/dates\" target=\"_blank\" rel=\"noopener\">Dating Responsibly<\/a>.<\/li>\n<li><b style=\"color: #c00;\">Repeated non-deterministic variable<\/b> &#8211; A minor thing but if <code style=\"font-family: consolas; background: #e4e4e4; padding: 3px 4px 1px 4px; border-radius: 3px;\">getdate()<\/code> is evaluated multiple times in the query, it could give a different answer, especially if the query is run right around midnight. Much better to pull these calculations out into a variable &#8211; not necessarily for better estimates, but to make sure all the predicates are speaking the same language.<\/li>\n<\/ol>\n<div style=\"width: 100px!important; text-align: center; color: white; background: #c00; border-radius: 5px; font-weight: bold; padding: 3px 6px; margin-bottom: 12px;\">Line 21<\/div>\n<pre class=\"theme:tomorrow-night font:consolas font-size:14 line-height:16 nums:true nums-toggle:false decode-attributes:false tab-convert:true lang:tsql decode:true start-line:21\">  where orderdate between dateadd(yy, -1, convert(DATE,getdate())) and getdate()<\/pre>\n<p>No new problems until we hit line 21:<\/p>\n<ol start=\"25\">\n<li><b style=\"color: #c00;\">DATEPART shorthand<\/b> &#8211; I&#8217;m mentioning this again not because of the shorthand itself but because it&#8217;s inconsistent. If you&#8217;re going to use <code style=\"font-family: consolas; background: #e4e4e4; padding: 3px 4px 1px 4px; border-radius: 3px;\">yyyy<\/code> then <em>always<\/em> use <code style=\"font-family: consolas; background: #e4e4e4; padding: 3px 4px 1px 4px; border-radius: 3px;\">yyyy<\/code>. Don&#8217;t get even lazier about it <em>sometimes<\/em>.<\/li>\n<li><b style=\"color: #c00;\">BETWEEN {some time} and &#8230; {now}?<\/b> &#8211; I&#8217;m already not a big fan of <code style=\"font-family: consolas; background: #e4e4e4; padding: 3px 4px 1px 4px; border-radius: 3px;\">BETWEEN<\/code> for date range queries (<a href=\"https:\/\/sqlblog.org\/2011\/10\/19\/what-do-between-and-the-devil-have-in-common\" target=\"_blank\" rel=\"noopener\">it can lead to lots of ambiguity<\/a>), but I certainly see no reason to include the current time as the upper bound, rather than the whole clause just being <code style=\"font-family: consolas; background: #e4e4e4; padding: 3px 4px 1px 4px; border-radius: 3px;\">&gt;= {some time}<\/code>. Orders generally aren&#8217;t placed in the future and, if they are, <em>all<\/em> the queries in this procedure should use that same upper bound. And if there is some reason the query should filter out future dates, there should be a comment here explaining why.<\/li>\n<\/ol>\n<div style=\"width: 100px!important; text-align: center; color: white; background: #c00; border-radius: 5px; font-weight: bold; padding: 3px 6px; margin-bottom: 12px;\">Line 27<\/div>\n<pre class=\"theme:tomorrow-night font:consolas font-size:14 line-height:16 nums:true nums-toggle:false decode-attributes:false tab-convert:true lang:tsql decode:true start-line:27\">  from person.emailaddress em (nolock) join person.person pp with (nolock)<\/pre>\n<ol start=\"27\">\n<li><b style=\"color: #c00;\">Inconsistent join ordering<\/b> &#8211; in the previous section, the tables were listed <code style=\"font-family: consolas; background: #e4e4e4; padding: 3px 4px 1px 4px; border-radius: 3px;\">person<\/code> then <code style=\"font-family: consolas; background: #e4e4e4; padding: 3px 4px 1px 4px; border-radius: 3px;\">emailaddress<\/code>. Now the order is swapped, and maybe there&#8217;s a reason, but it causes the reader to pause and wonder what it is.<\/li>\n<li><b style=\"color: #c00;\">Inconsistent aliasing<\/b> &#8211; In the previous section of the query, <code style=\"font-family: consolas; background: #e4e4e4; padding: 3px 4px 1px 4px; border-radius: 3px;\">person<\/code> was aliased as <code style=\"font-family: consolas; background: #e4e4e4; padding: 3px 4px 1px 4px; border-radius: 3px;\">p<\/code>, and <code style=\"font-family: consolas; background: #e4e4e4; padding: 3px 4px 1px 4px; border-radius: 3px;\">emailaddress<\/code> as <code style=\"font-family: consolas; background: #e4e4e4; padding: 3px 4px 1px 4px; border-radius: 3px;\">e<\/code>. Now they&#8217;ve changed to <code style=\"font-family: consolas; background: #e4e4e4; padding: 3px 4px 1px 4px; border-radius: 3px;\">pp<\/code> and <code style=\"font-family: consolas; background: #e4e4e4; padding: 3px 4px 1px 4px; border-radius: 3px;\">em<\/code> &#8211; while they are technically different queries, it is much better for future maintainers to adopt a consistent aliasing strategy (including what to do when a single query is aliased multiple times in the same query), so that there is less reverse engineering every time a query is reviewed.<\/li>\n<li><b style=\"color: #c00;\">Inconsistent hinting<\/b> &#8211; All the other <code style=\"font-family: consolas; background: #e4e4e4; padding: 3px 4px 1px 4px; border-radius: 3px;\">NOLOCK<\/code> hints in the query are just added to the table alias. This one is done <em>more correctly<\/em>, with the proper <code style=\"font-family: consolas; background: #e4e4e4; padding: 3px 4px 1px 4px; border-radius: 3px;\">WITH<\/code> keyword, but it&#8217;s a variation that can be disruptive to a reviewer.<\/li>\n<\/ol>\n<div style=\"width: 100px!important; text-align: center; color: white; background: #c00; border-radius: 5px; font-weight: bold; padding: 3px 6px; margin-bottom: 12px;\">Line 28<\/div>\n<pre class=\"theme:tomorrow-night font:consolas font-size:14 line-height:16 nums:true nums-toggle:false decode-attributes:false tab-convert:true lang:tsql decode:true start-line:28\">  on pp.businessentityid=em.businessentityid\n<\/pre>\n<ol start=\"30\">\n<li><b style=\"color: #c00;\">Inconsistent clause ordering<\/b> &#8211; Getting nit-picky here perhaps, but in the previous section, the clauses were listed in the same order the tables were introduced to the join; this time, they&#8217;re swapped. If there&#8217;s a reason the order has changed, there should be a comment to help a future maintainer understand.<\/li>\n<li><b style=\"color: #c00;\">Inconsistent spacing<\/b> &#8211; I like white space around my keywords and operators, so it really stands out to me when there is a change in pattern &#8211; in this case, there is no helpful white space around the <code style=\"font-family: consolas; background: #e4e4e4; padding: 3px 4px 1px 4px; border-radius: 3px;\">=<\/code> operator.<\/li>\n<\/ol>\n<div style=\"width: 100px!important; text-align: center; color: white; background: #c00; border-radius: 5px; font-weight: bold; padding: 3px 6px; margin-bottom: 12px;\">Line 30<\/div>\n<pre class=\"theme:tomorrow-night font:consolas font-size:14 line-height:16 nums:true nums-toggle:false decode-attributes:false tab-convert:true lang:tsql decode:true start-line:30\">  where customerid in (select top (2147483647) customerid<\/pre>\n<ol start=\"32\">\n<li><b style=\"color: #c00;\">TOP (2 billion)<\/b> &#8211; This seems a holdover from the SQL Server 2000 days, where you could fool the engine into returning data from a view in a specific order <em>without having to say so<\/em>, simply by putting <code style=\"font-family: consolas; background: #e4e4e4; padding: 3px 4px 1px 4px; border-radius: 3px;\">TOP 100 PERCENT ... ORDER BY<\/code> into the view definition. That hasn&#8217;t worked in 20 years, but I see this technique sometimes used to try to defeat row goals or coerce a specific index. Most likely it was just copied and pasted from somewhere else, but it has no place here.<\/li>\n<\/ol>\n<div style=\"width: 100px!important; text-align: center; color: white; background: #c00; border-radius: 5px; font-weight: bold; padding: 3px 6px; margin-bottom: 12px;\">Line 33<\/div>\n<pre class=\"theme:tomorrow-night font:consolas font-size:14 line-height:16 nums:true nums-toggle:false decode-attributes:false tab-convert:true lang:tsql decode:true start-line:33\">  and datediff(day, oh.orderdate, convert(DATE,getdate())) &lt;= 14<\/pre>\n<ol start=\"33\">\n<li><b style=\"color: #c00;\">Expression against a column<\/b> &#8211; SQL Server won&#8217;t be able to use an index on <code style=\"font-family: consolas; background: #e4e4e4; padding: 3px 4px 1px 4px; border-radius: 3px;\">OrderDate<\/code> because it will have to evaluate the expression <em>for every row<\/em>. This is a pattern I see quite a bit, and it can be hard to explain to newer users because, rightly so, they expect SQL Server to be able to rewrite the expression so that it <em>can<\/em> use an index. Always question when a <code style=\"font-family: consolas; background: #e4e4e4; padding: 3px 4px 1px 4px; border-radius: 3px;\">WHERE<\/code> or <code style=\"font-family: consolas; background: #e4e4e4; padding: 3px 4px 1px 4px; border-radius: 3px;\">ON<\/code> clause has to <em>change<\/em> what&#8217;s in the column in order to perform a comparison. In this case, much better (and more logical) to say <code style=\"font-family: consolas; background: #e4e4e4; padding: 3px 4px 1px 4px; border-radius: 3px;\">WHERE oh.OrderDate &gt;= @TwoWeeksAgo<\/code>.<\/li>\n<\/ol>\n<div style=\"width: 100px!important; text-align: center; color: white; background: #c00; border-radius: 5px; font-weight: bold; padding: 3px 6px; margin-bottom: 12px;\">Line 35<\/div>\n<pre class=\"theme:tomorrow-night font:consolas font-size:14 line-height:16 nums:true nums-toggle:false decode-attributes:false tab-convert:true lang:tsql decode:true start-line:35\">  inner join production.productsubcategory AS sc (nolock)<\/pre>\n<ol start=\"34\">\n<li><b style=\"color: #c00;\">Inconsistent join syntax<\/b> &#8211; This one wasn&#8217;t really a problem for me until it became inconsistent &#8211; don&#8217;t say <code style=\"font-family: consolas; background: #e4e4e4; padding: 3px 4px 1px 4px; border-radius: 3px;\">join<\/code> sometimes, and <code style=\"font-family: consolas; background: #e4e4e4; padding: 3px 4px 1px 4px; border-radius: 3px;\">inner join<\/code> other times. Personally, I prefer being more explicit about the type of join, but pick one or the other and always use that form.<\/li>\n<li><b style=\"color: #c00;\">Another table alias issue<\/b> &#8211; Suddenly we get an <code style=\"font-family: consolas; background: #e4e4e4; padding: 3px 4px 1px 4px; border-radius: 3px;\">AS<\/code> here, seemingly out of nowhere. I think this is another case where inconsistency is borne from queries being copied &amp; pasted from other sources or multiple authors contributing to a query. It may seem trite but take the time to step through the query and make sure all of the conventions match your established style (and if you don&#8217;t have one, <a href=\"https:\/\/www.red-gate.com\/simple-talk\/databases\/sql-server\/t-sql-programming-sql-server\/basics-good-t-sql-coding-style\/\" target=\"_blank\" rel=\"noopener\">create one<\/a>).<\/li>\n<\/ol>\n<div style=\"width: 100px!important; text-align: center; color: white; background: #c00; border-radius: 5px; font-weight: bold; padding: 3px 6px; margin-bottom: 12px;\">Line 37<\/div>\n<pre class=\"theme:tomorrow-night font:consolas font-size:14 line-height:16 nums:true nums-toggle:false decode-attributes:false tab-convert:true lang:tsql decode:true start-line:37\">  and sc.productcategoryid = @paramIntCategoryId order by customerid)<\/pre>\n<ol start=\"36\">\n<li><b style=\"color: #c00;\">Filter as a join clause<\/b> &#8211; Technically, that <code style=\"font-family: consolas; background: #e4e4e4; padding: 3px 4px 1px 4px; border-radius: 3px;\">and<\/code> should be a <code style=\"font-family: consolas; background: #e4e4e4; padding: 3px 4px 1px 4px; border-radius: 3px;\">WHERE<\/code>, since that is not a part of the join criteria. When we&#8217;re talking about <code style=\"font-family: consolas; background: #e4e4e4; padding: 3px 4px 1px 4px; border-radius: 3px;\">OUTER<\/code> joins, there is more nuance depending on which table the predicate applies to, but for an <code style=\"font-family: consolas; background: #e4e4e4; padding: 3px 4px 1px 4px; border-radius: 3px;\">INNER<\/code> join, the join and filter criteria shouldn&#8217;t mix.<\/li>\n<li><b style=\"color: #c00;\">Misplaced ORDER BY<\/b> &#8211; Merely mentioning this because when the <code style=\"font-family: consolas; background: #e4e4e4; padding: 3px 4px 1px 4px; border-radius: 3px;\">TOP<\/code> is removed this pointless clause will actually cause a compilation error.<\/li>\n<\/ol>\n<div style=\"width: 100px!important; text-align: center; color: white; background: #c00; border-radius: 5px; font-weight: bold; padding: 3px 6px; margin-bottom: 12px;\">Line 39<\/div>\n<pre class=\"theme:tomorrow-night font:consolas font-size:14 line-height:16 nums:true nums-toggle:false decode-attributes:false tab-convert:true lang:tsql decode:true start-line:39\">x<\/pre>\n<ol start=\"38\">\n<li><b style=\"color: #c00;\">Meaningless alias name<\/b> &#8211; This alias should be more descriptive about the subquery it references. And trust me, I&#8217;m guilty of this too; you&#8217;ll see <code style=\"font-family: consolas; background: #e4e4e4; padding: 3px 4px 1px 4px; border-radius: 3px;\">x<\/code> as an alias or CTE name in many of my solutions on Stack Overflow (but never in production code). I never said some of the problems I&#8217;d point out here aren&#8217;t my own problems too!<\/li>\n<\/ol>\n<div style=\"width: 100px!important; text-align: center; color: white; background: #c00; border-radius: 5px; font-weight: bold; padding: 3px 6px; margin-bottom: 12px;\">Line 40<\/div>\n<pre class=\"theme:tomorrow-night font:consolas font-size:14 line-height:16 nums:true nums-toggle:false decode-attributes:false tab-convert:true lang:tsql decode:true start-line:40\">order by 1<\/pre>\n<ol start=\"39\">\n<li><b style=\"color: #c00;\">ORDER BY ordinal<\/b> &#8211; Using ordinal numbers to define sorting is convenient, sure, but it&#8217;s <a href=\"https:\/\/sqlblog.org\/2009\/10\/06\/bad-habits-to-kick-order-by-ordinal\" target=\"_blank\" rel=\"noopener\">very brittle<\/a> and not self-documenting. Anyone reading the code now has to go parse the <code style=\"font-family: consolas; background: #e4e4e4; padding: 3px 4px 1px 4px; border-radius: 3px;\">SELECT<\/code> list to work it out, and anyone modifying the <code style=\"font-family: consolas; background: #e4e4e4; padding: 3px 4px 1px 4px; border-radius: 3px;\">SELECT<\/code> list might not realize that adding or rearranging columns just introduced a bug in the application.<\/li>\n<li><b style=\"color: #c00;\">No statement terminator<\/b> &#8211; This is one of the things <a href=\"https:\/\/sqlblog.org\/?s=semi-colon\" target=\"_blank\" rel=\"noopener\">I&#8217;ve been harping on<\/a> throughout a decent portion of my career &#8211; we should always be terminating statements with semi-colons. Though, personally, I don&#8217;t like adding them to block wrappers, like <code style=\"font-family: consolas; background: #e4e4e4; padding: 3px 4px 1px 4px; border-radius: 3px;\">BEGIN\/END<\/code>, because to me those aren&#8217;t independent statements (and try adding a semi-colon to <code style=\"font-family: consolas; background: #e4e4e4; padding: 3px 4px 1px 4px; border-radius: 3px;\">BEGIN\/END TRY<\/code> and <code style=\"font-family: consolas; background: #e4e4e4; padding: 3px 4px 1px 4px; border-radius: 3px;\">BEGIN\/END CATCH<\/code> &#8211; one of these things is definitely <em>not<\/em> like the others.<\/li>\n<\/ol>\n<\/section>\n<p><strong>Read also:<\/strong><br \/><a href=\"https:\/\/www.red-gate.com\/simple-talk\/databases\/sql-server\/t-sql-programming-sql-server\/temporary-tables-in-sql-server\/\" target=\"_blank\" rel=\"noopener\">Temporary tables in SQL Server<\/a><br \/><a href=\"https:\/\/www.red-gate.com\/simple-talk\/databases\/sql-server\/t-sql-programming-sql-server\/using-a-subquery-in-a-select-statement\/\" target=\"_blank\" rel=\"noopener\">Subqueries vs. derived tables<\/a><br \/><a href=\"https:\/\/www.red-gate.com\/simple-talk\/databases\/sql-server\/t-sql-programming-sql-server\/dependencies-and-references-in-sql-server\/\" target=\"_blank\" rel=\"noopener\">Dependencies and references in SQL Server<\/a><br \/><a href=\"https:\/\/www.red-gate.com\/simple-talk\/databases\/sql-server\/t-sql-programming-sql-server\/sql-server-choose-function\/\" target=\"_blank\" rel=\"noopener\">CHOOSE function for conditional logic<\/a><\/p>\n<section id=\"bertrand-blog-post\">\n<h3>See, That Wasn&#8217;t So Hard.<\/h3>\n<p>I looked at a 40-line stored procedure and identified 40 problems (not counting the ones that occurred multiple times). That&#8217;s a problem per line! And it wasn&#8217;t even that hard to come up with this &#8220;crazy&#8221; stored procedure &#8211; it&#8217;s actually quite believable that something similar could have crossed <em>your<\/em> desk as part of a pull request.<\/p>\n<h3>But How Would You Write It, Aaron?<\/h3>\n<p>Well, that&#8217;s a good question.<\/p>\n<p>I would certainly start by stepping back to understand what the procedure is actually trying to accomplish. We have three separate queries that all do similar things: they join order and customer tables and include or exclude rows based on dates and counts and other criteria. Doing this independently and then combining them with multiple distinct sorts to remove duplicates is wasteful and could lead to unacceptable performance at scale.<\/p>\n<p>So, I would attack the problem with the goal of touching each table only once &#8211; and of course cleaning up all the other things I noticed (most of which, admittedly, are aesthetic and have no bearing on performance). Taking a quick swing at it, and keeping in mind there are a dozen ways to write this, I came up with the following:<\/p>\n<pre class=\"theme:tomorrow-night font:consolas font-size:14 line-height:16 nums:true nums-toggle:false decode-attributes:false tab-convert:true lang:tsql decode:true \">CREATE PROCEDURE dbo.Customer_GetListForCampaignXYZ\n  @CategoryID int\nAS\nBEGIN\n  SET NOCOUNT ON;\n \n  DECLARE @AYearAgo    date = DATEADD(YEAR, -1, GETDATE()),\n          @TwoWeeksAgo date = DATEADD(WEEK, -2, GETDATE());\n \n  WITH CandidateCustomers AS\n  (\n    SELECT CustomerID, \n      IsAnOrderOlderThanAYearAgo     = MAX  ([old]),\n      OrdersInPastYear               = COUNT([new]),\n      RecentOrderFromDefinedCategory = MAX  ([cat])\n    FROM\n    (\n      SELECT CustomerID,\n         [old] = CASE WHEN OrderDate &lt;  @AYearAgo THEN 1 END,\n         [new] = CASE WHEN OrderDate &gt;= @AYearAgo THEN 1 END,\n         [cat] = CASE WHEN OrderDate &gt;= @TwoWeeksAgo \n             AND EXISTS \n             (\n               SELECT 1 FROM Sales.SalesOrderDetail AS d\n               WHERE d.SalesOrderID = h.SalesOrderID \n               AND EXISTS\n               (\n                 SELECT 1 FROM production.product AS p\n                 INNER JOIN Production.ProductSubcategory AS sc \n                         ON p.ProductSubcategoryID = sc.ProductSubcategoryID\n                 WHERE sc.ProductCategoryID = @CategoryID \n                   AND p.ProductID = d.ProductID\n               )\n             ) THEN 1 END\n      FROM Sales.SalesOrderHeader AS h\n    ) AS agg GROUP BY CustomerID\n  )\n  SELECT cc.CustomerID, pp.FirstName, em.EmailAddress \n    FROM CandidateCustomers AS cc\n    INNER JOIN Sales.Customer AS cu\n      ON cc.CustomerID = cu.CustomerID\n    INNER JOIN Person.Person AS pp\n      ON cu.PersonID = pp.BusinessEntityID\n    INNER JOIN Person.EmailAddress AS em\n      ON pp.BusinessEntityID = em.BusinessEntityID\n  WHERE (cc.IsAnOrderOlderThanAYearAgo = 1 AND cc.OrdersInPastYear = 0) \n     OR (cc.OrdersInPastYear &gt;= 3) \n     OR (cc.RecentOrderFromDefinedCategory = 1)\n  ORDER BY cc.CustomerID;\nEND<\/pre>\n<p>Total cost (in terms of line count): 10 additional lines (20%). Character count is actually slightly <em>lower<\/em>: 1,845 -&gt; 1,793. But the improvement to readability, maintainability, and grokability: priceless.<\/p>\n<h3>What About Performance?<\/h3>\n<p>After confirming that they return the same results, I compared execution times and a few plan characteristics. Here are the raw metrics observed at runtime (<a href=\"\/simple-talk\/wp-content\/uploads\/2023\/02\/ab-40problems-runtime.png\" target=\"_blank\" rel=\"noopener\">click to enlarge<\/a>):<\/p>\n<p><a href=\"\/simple-talk\/wp-content\/uploads\/2023\/02\/ab-40problems-runtime.png\" target=\"_blank\" rel=\"noopener\"><img decoding=\"async\" style=\"width: 70%; border: 1px solid black;\" src=\"\/simple-talk\/wp-content\/uploads\/2023\/02\/ab-40problems-runtime.png\" alt=\"Runtime metrics comparison\" \/><\/a><\/p>\n<p>On my system, which has modern NVMe drives, the original query is a lot faster than I thought (even with a cold cache). But notice the much higher number of reads, which could be an issue on systems with slower or more contentious I\/O, or not enough memory to always have the data in the buffer pool. Let&#8217;s look at the execution plans a little closer.<\/p>\n<p>For the old query, here is the plan (<a href=\"\/simple-talk\/wp-content\/uploads\/2023\/02\/ab-40problems-originalPlan.png\" target=\"_blank\" rel=\"noopener\">click to enlarge<\/a>):<\/p>\n<p><a href=\"\/simple-talk\/wp-content\/uploads\/2023\/02\/ab-40problems-originalPlan.png\" target=\"_blank\" rel=\"noopener\"><img decoding=\"async\" style=\"width: 70%; border: 1px solid black;\" src=\"\/simple-talk\/wp-content\/uploads\/2023\/02\/ab-40problems-originalPlan.png\" alt=\"Original, busy plan\" \/><\/a><\/p>\n<p>And here is the new plan, which is far simpler (<a href=\"\/simple-talk\/wp-content\/uploads\/2023\/02\/ab-40problems-newPlan.png\" target=\"_blank\" rel=\"noopener\">click to enlarge<\/a>):<\/p>\n<p><a href=\"\/simple-talk\/wp-content\/uploads\/2023\/02\/ab-40problems-newPlan.png\" target=\"_blank\" rel=\"noopener\"><img decoding=\"async\" style=\"width: 70%; border: 1px solid black;\" src=\"\/simple-talk\/wp-content\/uploads\/2023\/02\/ab-40problems-newPlan.png\" alt=\"New, simpler plan\" \/><\/a><\/p>\n<p>You can get a <em>general<\/em> sense for the differences by looking at the plans casually, but we can also take a quantitative comparison of the operators and other information the plans provide (with the worse figure highlighted in <span style=\"padding: 1px 3px; background: #fecfcc!important;\">this salmon-y color<\/span>):<\/p>\n<table style=\"border-collapse: collapse; font-size: 0.875rem; line-height: 1.25rem; width: 100%; height: 363px;\">\n<tbody>\n<tr style=\"background: #f4f4f4;\">\n<th style=\"border: 1px solid #000000; vertical-align: middle; padding: 6px 8px !important; height: 33px;\">Metric<\/th>\n<th style=\"border: 1px solid #000000; vertical-align: middle; text-align: center; padding: 6px 8px !important; height: 33px;\">Original<\/th>\n<th style=\"border: 1px solid #000000; vertical-align: middle; text-align: center; padding: 6px 8px !important; height: 33px;\">New<\/th>\n<th style=\"border: 1px solid #000000; vertical-align: middle; padding: 6px 8px !important; height: 33px;\">Notes<\/th>\n<\/tr>\n<tr style=\"height: 43px;\">\n<th style=\"border: 1px solid #000000; background: #f4f4f4; vertical-align: middle; height: 43px;\">Estimated Cost %<\/th>\n<td style=\"border: 1px solid #666666; text-align: center; vertical-align: middle; height: 43px;\">40.9%<\/td>\n<th style=\"border: 1px solid #666666; background: #fecfcc; text-align: center; vertical-align: middle; height: 43px;\">59.1%<\/th>\n<td style=\"border: 1px solid #666666; vertical-align: middle; height: 43px;\">Given everything else below, this demonstrates why estimated cost % can be misleading (a.k.a. garbage).<\/td>\n<\/tr>\n<tr style=\"height: 44px;\">\n<th style=\"border: 1px solid #000000; background: #f4f4f4; vertical-align: middle; height: 44px;\">Estimated Rows<br \/><span style=\"font-size: 0.75rem; font-weight: 400!important;\">(Actual: 2,313)<\/span><\/th>\n<th style=\"border: 1px solid #666666; background: #fecfcc; text-align: center; vertical-align: middle; height: 44px;\">25,700<br \/><span style=\"font-size: 0.75rem; font-weight: 400!important;\">1,111%<\/span><\/th>\n<td style=\"border: 1px solid #666666; text-align: center; vertical-align: middle; height: 44px;\">6,920<br \/><span style=\"font-size: 0.75rem; color: #aaa!important;\">299%<\/span><\/td>\n<td style=\"border: 1px solid #666666; vertical-align: middle; height: 44px;\">Neither estimate was a bullseye, but the <code style=\"font-family: consolas; background: #e4e4e4; padding: 3px 4px 1px 4px; border-radius: 3px;\">UNION<\/code> query was off by more than an order of magnitude.<\/td>\n<\/tr>\n<tr style=\"height: 43px;\">\n<th style=\"border: 1px solid #000000; background: #f4f4f4; vertical-align: middle; height: 43px;\">Reads<\/th>\n<th style=\"border: 1px solid #666666; background: #fecfcc; text-align: center; vertical-align: middle; height: 43px;\">9,626<\/th>\n<td style=\"border: 1px solid #666666; text-align: center; vertical-align: middle; height: 43px;\">1,902<\/td>\n<td style=\"border: 1px solid #666666; vertical-align: middle; height: 43px;\">This can be a good warning for systems that are I\/O-bound and\/or memory-bound.<\/td>\n<\/tr>\n<tr style=\"height: 24px;\">\n<th style=\"border: 1px solid #000000; background: #f4f4f4; vertical-align: middle; min-width: 170px !important; height: 24px;\">\n<div>Clustered Index Scans<\/div>\n<\/th>\n<th style=\"border: 1px solid #666666; background: #fecfcc; text-align: center; vertical-align: middle; height: 24px;\">7<\/th>\n<td style=\"border: 1px solid #666666; text-align: center; vertical-align: middle; height: 24px;\">4<\/td>\n<td style=\"border: 1px solid #666666; vertical-align: middle; height: 24px;\">Scans against <code style=\"font-family: consolas; background: #e4e4e4; padding: 3px 4px 1px 4px; border-radius: 3px;\">SalesOrderHeader<\/code>: 3 vs. 1<\/td>\n<\/tr>\n<tr style=\"height: 23px;\">\n<th style=\"border: 1px solid #000000; background: #f4f4f4; vertical-align: middle; height: 23px;\">Hash Matches<\/th>\n<th style=\"border: 1px solid #666666; background: #fecfcc; text-align: center; vertical-align: middle; height: 23px;\">8<\/th>\n<td style=\"border: 1px solid #666666; text-align: center; vertical-align: middle; height: 23px;\">5<\/td>\n<td style=\"border: 1px solid #666666; vertical-align: middle; height: 23px;\">This can be a good warning for systems that are memory-bound (<a href=\"https:\/\/sqlserverfast.com\/epr\/hash-match\/#memory\" target=\"_blank\" rel=\"noopener\">more info<\/a>).<\/td>\n<\/tr>\n<tr style=\"height: 24px;\">\n<th style=\"border: 1px solid #000000; background: #f4f4f4; vertical-align: middle; height: 24px;\">Sorts<\/th>\n<th style=\"border: 1px solid #666666; background: #fecfcc; text-align: center; vertical-align: middle; height: 24px;\">5<\/th>\n<td style=\"border: 1px solid #666666; text-align: center; vertical-align: middle; height: 24px;\">1<\/td>\n<td style=\"border: 1px solid #666666; vertical-align: middle; height: 24px;\">And one sort in each plan (the final <code style=\"font-family: consolas; background: #e4e4e4; padding: 3px 4px 1px 4px; border-radius: 3px;\">ORDER BY<\/code>) isn&#8217;t necessary.<\/td>\n<\/tr>\n<tr style=\"height: 43px;\">\n<th style=\"border: 1px solid #000000; background: #f4f4f4; vertical-align: middle; height: 43px;\">Spills<\/th>\n<th style=\"border: 1px solid #666666; background: #fecfcc; text-align: center; vertical-align: middle; height: 43px;\">1<\/th>\n<td style=\"border: 1px solid #666666; text-align: center; vertical-align: middle; height: 43px;\">0<\/td>\n<td style=\"border: 1px solid #666666; vertical-align: middle; height: 43px;\">There was one level 2 spill in the original plan, nothing disastrous; likely a result of bad estimates.<\/td>\n<\/tr>\n<tr style=\"height: 43px;\">\n<th style=\"border: 1px solid #000000; background: #f4f4f4; vertical-align: middle; height: 43px;\">Spools<\/th>\n<td style=\"border: 1px solid #666666; text-align: center; vertical-align: middle; height: 43px;\">0<\/td>\n<th style=\"border: 1px solid #666666; background: #fecfcc; text-align: center; vertical-align: middle; height: 43px;\">1<\/th>\n<td style=\"border: 1px solid #666666; vertical-align: middle; height: 43px;\">The spool in the new plan is the only operator that is concerning &#8211; it only handles ~7K rows, but represents 13% of overall CPU.<\/td>\n<\/tr>\n<tr style=\"height: 43px;\">\n<th style=\"border: 1px solid #000000; background: #f4f4f4; vertical-align: middle; height: 43px;\">Residual I\/O Warnings<\/th>\n<th style=\"border: 1px solid #666666; background: #fecfcc; text-align: center; vertical-align: middle; height: 43px;\">2<\/th>\n<td style=\"border: 1px solid #666666; text-align: center; vertical-align: middle; height: 43px;\">0<\/td>\n<td style=\"border: 1px solid #666666; vertical-align: middle; height: 43px;\">These are two bright yellow signs that the original query is reading a lot more data than it should.<\/td>\n<\/tr>\n<\/tbody>\n<\/table>\n<p>Given all that, there&#8217;s very little redeeming about the original approach to union three separate queries &#8211; and I would still feel that way even without the unnecessary distinct sorts. The code review created a much more readable procedure <em>and<\/em> made the query perform better in nearly every measurable aspect. And it could potentially be tuned even further, even though a one-off lead generation procedure isn&#8217;t likely to be performance critical. The important thing is that making these conventions and processes commonplace will make it more likely that the <em>next<\/em> procedure won&#8217;t face such a rigorous review.<\/p>\n<h3>What If The Problems Aren&#8217;t Obvious?<\/h3>\n<p>As I stated at the beginning, I&#8217;ve developed a sense of code smells and best practices through experience over my entire career, and I&#8217;m still adjusting my techniques. So nobody expects you to be able to spot all of these problems out of the gate. One way to get a head start is to try out an add-in like <a href=\"https:\/\/www.red-gate.com\/products\/sql-development\/sql-prompt\/\" target=\"_blank\" rel=\"noopener\">SQL Prompt<\/a>, which certainly did <a href=\"\/simple-talk\/wp-content\/uploads\/2023\/02\/40-problems-sql-prompt.png\" target=\"_blank\" rel=\"noopener\">identify several of the above issues<\/a>.<\/p>\n<p><strong>Read also: <\/strong><a href=\"https:\/\/www.red-gate.com\/simple-talk\/blogs\/sql-naming-conventions\/\" target=\"_blank\" rel=\"noopener\">SQL naming conventions<\/a><\/p>\n<h3>Wrapping Up<\/h3>\n<p>Now, would I torture a co-worker with a code review this thorough and pedantic? Of course not. But if the code raises my left eyebrow this many times, now I have a blog post I can point them at. \ud83d\ude42<\/p>\n<\/section>\n\n\n<section id=\"my-first-block-block_1ef913783b71b919f6eece39a69ffdc1\" class=\"my-first-block alignwide\">\n    <div class=\"bg-brand-600 text-base-white py-5xl px-4xl rounded-sm bg-gradient-to-r from-brand-600 to-brand-500 red\">\n        <div class=\"gap-4xl items-start md:items-center flex flex-col md:flex-row justify-between\">\n            <div class=\"flex-1 col-span-10 lg:col-span-7\">\n                <h3 class=\"mt-0 font-display mb-2 text-display-sm\">Fast, reliable and consistent SQL Server development&#8230;<\/h3>\n                <div class=\"child:last-of-type:mb-0\">\n                                            &#8230;with SQL Toolbelt Essentials. 10 ingeniously simple tools for accelerating development, reducing risk, and standardizing workflows.                                    <\/div>\n            <\/div>\n                                            <a href=\"https:\/\/www.red-gate.com\/products\/sql-toolbelt-essentials\/\" class=\"btn btn--secondary btn--lg\" aria-label=\"Learn more &amp; try for free: Fast, reliable and consistent SQL Server development...\">Learn more &amp; try for free<\/a>\n                    <\/div>\n    <\/div>\n<\/section>\n\n\n<section id=\"faq\" class=\"faq-block my-5xl\">\n    <h2>FAQs: Common SQL Server stored procedure problems<\/h2>\n\n                        <h3 class=\"mt-4xl\">1. What are the most common stored procedure problems in SQL Server?<\/h3>\n            <div class=\"faq-answer\">\n                <p>The most frequent issues include: missing SET NOCOUNT ON (causes extra result sets), no error handling (TRY\/CATCH), inconsistent naming conventions (sp_ prefix, mixed case), non-sargable WHERE clauses (wrapping columns in functions), using SELECT instead of SET for variable assignment, missing schema qualification (dbo.), using SELECT * instead of named columns, and not specifying varchar\/decimal precision. These are coding standards issues &#8211; the procedure runs but is fragile and hard to maintain.<\/p>\n            <\/div>\n                    <h3 class=\"mt-4xl\">2. How do you do a stored procedure code review in SQL Server?<\/h3>\n            <div class=\"faq-answer\">\n                <p>Review line by line checking: (1) Does it SET NOCOUNT ON? (2) Is there TRY\/CATCH error handling? (3) Are all objects schema-qualified? (4) Are naming conventions consistent? (5) Are WHERE clauses sargable? (6) Is UNION ALL used instead of UNION where duplicates aren\u2019t possible? (7) Are temp tables properly indexed and cleaned up? (8) Does the execution plan show unexpected scans or sorts? Tools like SQL Prompt can automate detection of many of these issues.<\/p>\n            <\/div>\n                    <h3 class=\"mt-4xl\">3. Should stored procedures use SET or SELECT for variable assignment in SQL Server?<\/h3>\n            <div class=\"faq-answer\">\n                <p>Use SET. SET is the ANSI standard, assigns one variable at a time, and returns NULL if the query returns no rows (making errors detectable). SELECT can assign multiple variables in one statement but silently keeps the previous value if no rows are returned, which can cause subtle bugs. The only exception: if you need to assign multiple variables from the same row for performance, SELECT is slightly more efficient &#8211; but document why.<\/p>\n            <\/div>\n            <\/section>\n","protected":false},"excerpt":{"rendered":"<p>Review 40 real stored procedure problems in SQL Server: naming conventions, error handling, NOCOUNT, temp table usage, sargability, schema binding, SET vs SELECT, and query plan issues.&hellip;<\/p>\n","protected":false},"author":341115,"featured_media":0,"comment_status":"open","ping_status":"closed","sticky":false,"template":"","format":"standard","meta":{"_acf_changed":false,"footnotes":""},"categories":[53,143531],"tags":[4745],"coauthors":[158980],"class_list":["post-95486","post","type-post","status-publish","format-standard","hentry","category-featured","category-t-sql-programming-sql-server","tag-transact-sql"],"acf":[],"_links":{"self":[{"href":"https:\/\/www.red-gate.com\/simple-talk\/wp-json\/wp\/v2\/posts\/95486","targetHints":{"allow":["GET"]}}],"collection":[{"href":"https:\/\/www.red-gate.com\/simple-talk\/wp-json\/wp\/v2\/posts"}],"about":[{"href":"https:\/\/www.red-gate.com\/simple-talk\/wp-json\/wp\/v2\/types\/post"}],"author":[{"embeddable":true,"href":"https:\/\/www.red-gate.com\/simple-talk\/wp-json\/wp\/v2\/users\/341115"}],"replies":[{"embeddable":true,"href":"https:\/\/www.red-gate.com\/simple-talk\/wp-json\/wp\/v2\/comments?post=95486"}],"version-history":[{"count":230,"href":"https:\/\/www.red-gate.com\/simple-talk\/wp-json\/wp\/v2\/posts\/95486\/revisions"}],"predecessor-version":[{"id":109053,"href":"https:\/\/www.red-gate.com\/simple-talk\/wp-json\/wp\/v2\/posts\/95486\/revisions\/109053"}],"wp:attachment":[{"href":"https:\/\/www.red-gate.com\/simple-talk\/wp-json\/wp\/v2\/media?parent=95486"}],"wp:term":[{"taxonomy":"category","embeddable":true,"href":"https:\/\/www.red-gate.com\/simple-talk\/wp-json\/wp\/v2\/categories?post=95486"},{"taxonomy":"post_tag","embeddable":true,"href":"https:\/\/www.red-gate.com\/simple-talk\/wp-json\/wp\/v2\/tags?post=95486"},{"taxonomy":"author","embeddable":true,"href":"https:\/\/www.red-gate.com\/simple-talk\/wp-json\/wp\/v2\/coauthors?post=95486"}],"curies":[{"name":"wp","href":"https:\/\/api.w.org\/{rel}","templated":true}]}}