April 2, 2014 at 2:18 pm
I like the recent series of questions with some backstory. Not the strongest stories, but enough to add a nice touch to the QotD.
I do not, however, like this question. It builds on assumptions that are simply too far-fetched and unrealistic. And parts of the "correct" answer foster bad practice. Let's go over the answer options to see where I believe Andy went wrong.
The table does not have a primary key - obvious, too obvious. No idea why Andy added this optoin to the question. The 11% who (currently) did not select this option were probably either overlooking the "choose 5" remark, or just clicking some random options because they just want to read the answer and discussion and don't care about the points.
There are 2 rows in the table - again, very obvious. No INSERT trigger, no constraints, and no errors in the INSERT statements.
The query plan for the update shows a table scan - well, duh! The table is an unindexed heap; table scan is the only available option, period. I am not sure if this is supposed to refer to the update in the query window, the update in the trigger, or both, but it's true anyway. And not, as the answer suggests, because "there only two rows in the table and the table has no indexes" - but just because the table has no indexes. Add twenty billion rows, and you'll still get a scan for this query.
The trigger does not contain a logic error - this is where it gets tricky. The UPDATE ... FROM used in the trigger is a highly dangerous construction that, if I had my way, would have been deprecated a long time ago already. It is only reliable if the updated table is joined to the other table(s) on a condition that is guaranteed never to return more than a single row. In this case, that guarantee can only be given by a primary key constraint, unique constraint, or unique index on the QuestionID column. There is no such guarantee in this case, so it is possible that a single row will be updated multiple times in the trigger, if it joins to multiple rows in inserted.
However, in this specific case I will not call it a logic error - the SET clause only uses a system function, not a column from inserted, so no matter which order the optimizer picks, the result will still be the same. Just in a highly inefficient way. Any junior developer presenting this trigger code to me would be sent back to his desk immediately with the task of rewriting this code to use WHERE EXISTS - which does not have any of these issues and is hence far more efficient.
A resultset is being returned when the update runs - now this is where I really got angry. So I am supposed to assume that I would start my query tuning by just enabling the actual execution plan option? Even before I add the first index? Please don't tell me that there are people who tune this way. Good tuning does NOT start with the execution plan. It starts with using yoour brains to think about indexes. The next step is to do actual performance measurements, not by looking at execution plans but by using either SET STATISTICS IO and SET STATISTICS TIME, or by looking at actual elapsed time on a high enough umber of executions.
You only look at execution plans when you have a very complex query and are worried that you have made the optimzers' task too hard, or when you have seen a demonstrable performance issue with a query and need to figure out what the heck is causing it.
But one assumption is not enough - no, to arrive at the "right" answer, I had to make a second assumption as well: "The default for that setting is false, but it in this case it was enabled". Why would I have changed that setting? Why would anyone ever change that setting? Oh, I see - because, apparently, "it's never a good idea to return results from a trigger". Never? Really? I'd say that it's never a good idea to use the word never in any statement about SQL Server. And definitely not in this case. What if a query becomes a performance problem and you need to see the execution plan of a query in the trigger? (Sure, there are ways - but they are much harder, so why not follow the easy route?) What if a trigger doesn't yield the expected results and you cannot find the root cause? The best (or rather: least sucky) way to debug a trigger is to insert a few PRINT or SELECT statements in it and then trigger its execution in a query window. And whay if you run into one of the rare but real business cases where you actually need some results to be returned from a trigger?
If you don't want resultsets from your trigger, then simply don't use SELECT or PRINT statements in it. But don't disable a completely fine option.
With these assumptions, I could have explained the "total of three rows" statement. But there are other, less far-fetched assumptions that can also explain this - like maybe my default schema is not dbo, so the trigger is referencing another table; or a coworker played a prank on me while I was away from my keyboard; or one of the other people at the meeting was so excited that he immediately added his question to the table. Or (the one I went with, as being the least far-fetched) Andy made a mistake while typing the question and intended to write "four" - because both the update in the query window and the update in the trigger will return a "2 row(s) returned" message. (Well, unless of course you have set your SSMS to default to use SET NOCOUNT ON in every query window, like I normally do).
The update can be fixed by adding SET NOCOUNT ON - that depends on what you perceive to be the problem. Assuming that Andy meant "four" instead of "three", this will definitely fix that "problem". Plus, it is a well-documented best practice to start every trigger with SET NOCOUNT ON - those extra "nn row(s) returned" messages place unneeded extra burden on the network, slow down the performance, and worst of all can cause errors in some front-ends.
Out of the five options I picked, four matched Andy's. Three of them with no debate at all. One rather questionable - I believe that the "logic error" is a position that can be defended and I would never accept this trigger in my production system for that reason.
On the one where my answer differes from Andy's - after reading his explanation, I am even more convinced then before that my choice is less wrong than his. Not correct, as it does not exactly fir the question description, but at least it requires assumptions that are not so extremely far-fetched as the assumptions Andy implies.
I understand that, after running into this, Andy wanted to submit this as a QotD. But he somehow forgot to check exectly how unrealistic the assumptions are that someone seeing only this question would have to make.
Andy, I have always respected you a lot. Seeing the enormous amount of high-quality questions you contributed recently, and the extra effort you went through to add a light back-story has raised that respect even more. This question was an unfortunate slip. No worries, I am already looking forward to your next question, and I am 100% sure that it will be a good one again.
April 2, 2014 at 2:32 pm
Andy Warren (4/2/2014)
I appreciate the comments, good and bad, and any ideas you have for making it better without making it easier!
If the intention of the question is to educate people on the possibility of getting an error because of the combination of the results from tirggers options and including actual execution plans, then I'd suggest something along these lines: (not sure if it's harder or easier, but I like to think it's more fair)
Q: You are tuning some code, and expect the root cause of the performance problem to be a trigger that executes a rather complex query. To verify your idea, you enable the actual execution plan and rerun the code. Instead of the expected execution plan output, SQL Server throws an error. What can be the cause?
A1: The error is forced by a server-level option that has been changed from its default setting (correct)
A2: The execution plan is too complex and cannot be displayed by Management Studio (not true - I don't think SSMS has such limitations, and even if it does it would cause SSMS to throw an error, not SQL Server)
A3: Using actual execution plans on a trigger is not supported, you either have to use an estimated execution plan, or catch the plan through Extended Events or Profiler (not true, though close and some of the alternative options would work)
A4: This is a known bug in SQL Server 2008R2 and SQL Server 2012; it is fixed in CU12 for SQL Server 2008R2 and in CU2 for SQL Server 2012 (pure bluff, but the addition of the CUs where this is fixed might make it sound just realistic enough that people fall for it)
April 2, 2014 at 2:59 pm
Hugo Kornelis (4/2/2014)
I like the recent series of questions with some backstory. Not the strongest stories, but enough to add a nice touch to the QotD.A resultset is being returned when the update runs - now this is where I really got angry. So I am supposed to assume that I would start my query tuning by just enabling the actual execution plan option? Even before I add the first index? Please don't tell me that there are people who tune this way. Good tuning does NOT start with the execution plan. It starts with using yoour brains to think about indexes. The next step is to do actual performance measurements, not by looking at execution plans but by using either SET STATISTICS IO and SET STATISTICS TIME, or by looking at actual elapsed time on a high enough umber of executions.
You only look at execution plans when you have a very complex query and are worried that you have made the optimzers' task too hard, or when you have seen a demonstrable performance issue with a query and need to figure out what the heck is causing it.
Questions based on assumptions is generally a bad idea
/Håkan Winther
MCITP:Database Developer 2008
MCTS: SQL Server 2008, Implementation and Maintenance
MCSE: Data Platform
April 2, 2014 at 3:12 pm
Carlo Romagnano (4/2/2014)
In sqlserver 2008 r2 the update doesn't return any resultset and no error message.(2 row(s) affected)
(2 row(s) affected)
What I'm missing?
Same here.
--Jeff Moden
Change is inevitable... Change for the better is not.
April 2, 2014 at 3:16 pm
Hugo Kornelis (4/2/2014)
The UPDATE ... FROM used in the trigger is a highly dangerous construction that, if I had my way, would have been deprecated a long time ago already.
I'm REALLY happy that you're not likely to get your way on that one, Hugo. 😉
--Jeff Moden
Change is inevitable... Change for the better is not.
April 2, 2014 at 10:14 pm
Got 4/5 right, but am slightly disturbed by the question, primarily for the following reason:
The "disallow results from triggers" Server Configuration option has always been 0 by default in all the SQL Server installations that I have worked with till date. So, it was impossible for me to make the assumption that the option was turned ON (i.e. set to 1).
Maybe it would have been nice to say something like "all recommendations from Microsoft have been followed" or similar (per Books On Line, the recommendation from Microsoft is to set this option to 1 manually).
In any case, although I got it wrong, it was an interesting question. Thank-you, Andy and looking forward to your next question 🙂
Thanks & Regards,
Nakul Vachhrajani.
http://nakulvachhrajani.com
Follow me on
Twitter: @sqltwins
April 3, 2014 at 1:28 am
On logic error.
Having no PK and joining just by a single column to match the updated row may be considered as a logic error as well.
April 3, 2014 at 3:10 am
Nice question, thanks.
Need an answer? No, you need a question
My blog at https://sqlkover.com.
MCSE Business Intelligence - Microsoft Data Platform MVP
April 3, 2014 at 9:13 am
I think the trigger still contains a logic error...
Currently it updates the datechanged even if the values remain the same. Maybe if the field was called DateUpdated a change wouldn't be required 😉
alter TRIGGER updateQuestions ON dbo.Questions
FOR UPDATE
AS
begin
set nocount on
UPDATE Q
SET Q.datechanged = GETUTCDATE()
FROM
dbo.Questions Q
left join
deleted d
on
d.QuestionID = q.QuestionID
where
IsNull(Q.QuestionTitle,'') <> IsNull(d.QuestionTitle,'') or
IsNull(Q.IsApproved, 0) <> IsNull(d.IsApproved, 0)
end
April 3, 2014 at 9:22 am
auke.teeninga (4/3/2014)
I think the trigger still contains a logic error...Currently it updates the datechanged even if the values remain the same. Maybe if the field was called DateUpdated a change wouldn't be required 😉
ALTER TRIGGER updateQuestions ON dbo.Questions
FOR UPDATE
AS
SET NOCOUNT ON
UPDATE Q
SET Q.datechanged = GETUTCDATE()
FROM inserted i
INNER JOIN dbo.Questions Q
ON I.QuestionID = q.QuestionID
where
IsNull(Q.QuestionTitle,'') <> IsNull(i.QuestionTitle,'') or
IsNull(Q.IsApproved, -1) <> IsNull(i.IsApproved, -1)
While that does meet some esoteric requirements this will perform very poorly because this is nonSARGable.
The bigger is that you have introduced a logic error by wrapping a bit column with IsNull. This changes NULL to 1. Don't believe on that one? Try casting -1 as bit and see what you get.
select CAST(-1 as bit)
_______________________________________________________________
Need help? Help us help you.
Read the article at http://www.sqlservercentral.com/articles/Best+Practices/61537/ for best practices on asking questions.
Need to split a string? Try Jeff Modens splitter http://www.sqlservercentral.com/articles/Tally+Table/72993/.
Cross Tabs and Pivots, Part 1 – Converting Rows to Columns - http://www.sqlservercentral.com/articles/T-SQL/63681/
Cross Tabs and Pivots, Part 2 - Dynamic Cross Tabs - http://www.sqlservercentral.com/articles/Crosstab/65048/
Understanding and Using APPLY (Part 1) - http://www.sqlservercentral.com/articles/APPLY/69953/
Understanding and Using APPLY (Part 2) - http://www.sqlservercentral.com/articles/APPLY/69954/
April 3, 2014 at 9:39 am
Sean Lange (4/3/2014)
While that does meet some esoteric requirements this will perform very poorly because this is nonSARGable.
Good call on the bit field. I try to avoid them myself, so I had not encountered that behavior before.
I think even with the nonSARGablility of the where clause I think it might outperform the original tigger, due to the fact that no update (cq no lock) is needed when there's no change.
April 3, 2014 at 10:23 am
auke.teeninga (4/3/2014)
Sean Lange (4/3/2014)
While that does meet some esoteric requirements this will perform very poorly because this is nonSARGable.
Good call on the bit field. I try to avoid them myself, so I had not encountered that behavior before.
I think even with the nonSARGablility of the where clause I think it might outperform the original tigger, due to the fact that no update (cq no lock) is needed when there's no change.
When it comes to performance going with your gut instinct is not good enough. It should always be tested. That being said I can speak that from my experience there is no chance that adding a nonSARGable predicate that will cause an index scan will outperform an index seek, which is what we see from the original. The big difference is that what you posted will only update the date column when the value of the data is changed.
_______________________________________________________________
Need help? Help us help you.
Read the article at http://www.sqlservercentral.com/articles/Best+Practices/61537/ for best practices on asking questions.
Need to split a string? Try Jeff Modens splitter http://www.sqlservercentral.com/articles/Tally+Table/72993/.
Cross Tabs and Pivots, Part 1 – Converting Rows to Columns - http://www.sqlservercentral.com/articles/T-SQL/63681/
Cross Tabs and Pivots, Part 2 - Dynamic Cross Tabs - http://www.sqlservercentral.com/articles/Crosstab/65048/
Understanding and Using APPLY (Part 1) - http://www.sqlservercentral.com/articles/APPLY/69953/
Understanding and Using APPLY (Part 2) - http://www.sqlservercentral.com/articles/APPLY/69954/
April 3, 2014 at 10:58 am
So many options to choose from... 😉
Thanks, Andy!
April 3, 2014 at 12:00 pm
Nakul, I like the bit "all recommendations from Microsoft have been followed". I'll keep that in mind.
April 3, 2014 at 12:05 pm
Hugo, thanks for taking the time to write that up. Can't say I disagree with your conclusions! I'm definitely learning a lot, the trick is whether I can successfully apply those lessons when I write the next one(s). I have two ideas queued, so anything you see after today "should" be better than the previous ones, but I'm still experimenting. I want to do some simpler questions, I want to work more on the story ones, and I've even got a vague idea about a mystery/treasure hunt that might span multiple questions.I'm also trying to capture at least some of the lessons so I can maybe teach how to do it/make it approachable for someone who wants to do a single question and for that going back to re-read these comments is hugely valuable.
Viewing 15 posts - 31 through 45 (of 58 total)
You must be logged in to reply to this topic. Login to reply