What do you guys look for when you are doing a code review from performance prospective. When i do it I look for the following things
1) Use NOLOCK when doing select so that table is not locked.
2) Prefix dbo. before a table name.
3) Use table variables instead of temp tables when dealing with less data
4) Avoid cursors, use while loop instead.
5) Joins are optimal or not.
6) Use of ISNULL is case field has been defined to accept null values.
Please share some more knowledge on this subject.
We have portfolios with holdings. One thing i always look for are
SELECT *
FROM Holdings
WHERE (@PortfolioID IS NULL OR @PortfolioID = PortfolioID)
I rather replace this with the use of a var table with primary key
DECLARE @PortfolioIDs TABLE(
PortfolioID INT PRIMARY KEY
)
IF (@PortfolioID IS NULL)
INSERT INTO @PortfolioIDs SELECT ActivePortfolioID FROM ActivePortfolios
ELSE
INSERT INTO @PortfolioIDs SELECT @PortfolioID
SELECT *
FROM Holdings h INNER JOIN
@PortfolioIDs p ON h.PortfolioID = p.PortfolioID
Also avoid using unnecessary subselects in the main select, or in the where clause.
There's no definitive list. i.e. one example of this is NOLOCK is not suitable for use in all occassions
Just reviewing the code, you can't always see optimisations to be made - in order to do that, involves comparing different approaches, checking execution plans, checking the execution stats.
You should be able to spot obvious logic flaws (especially check whether ANDs and ORs are used correctly, bracketed correctly etc) and obvious performance issues (using a cursor when it could be done as a set-based approach).
Other things I like to check: - is it maintainable/readable? i.e. if another dev picked it up at a later date, is it clear what's going on or is it spaghetti. - Use of GOTO statements - if used, are they really necessary?
The only hard and fast rule above is "Prefix dbo. before a table name". Even then, it should be prefix the schema.
For example:
ISNULL or COALESCE?
A JOIN can not be optimal by itself. It's part of a query (DISTINCT/JOIN vs EXISTS vs IN, indexes, function on column, datatype precedence, SELECT * etc)
Other that that, do you have a template or standard? With your SET NOCOUNT, SET XACT_ABORT, TRY/CATCH, transaction handling etc
The question itself is too broad, sorry
1) Use NOLOCK when doing select so that table is not locked.
Usually a bad idea. Don't do it until you have hit concurrency issues.
2) Prefix dbo. before a table name.
Good idea, but not worth throwing a war over.
3) Use table variables instead of temp tables when dealing with less data
Could be a good idea.
4) Avoid cursors, use while loop instead.
What do you mean? You use cursors in a while loop.
5) Joins are optimal or not.
What do you mean? A join is either correct or incorrect, then it's up to indexing to make it optimal.
6) Use of ISNULL is case field has been defined to accept null values.
Yes, that (or some similar construct eg. coalesce) is required to handle NULL values.
Things I would normally look for - they might all seem basic but I regularly see people not using them:
SET NOCOUNT ON to suppress the usual 'x rows returned' message.
SELECTing a named set of columns rather than just SELECT * avoiding a lookup to get the list of available columns in the table. Also avoids returning more data than necessary to the client.
Stored procs not prefixed with sp_ - sprocs with this prefix are always looked up in the master db first, regardless of which database you are connected to.
Last year I wrote a blog post on my personal "best practices" checklist for writing stored procedures. A lot of these things are subjective, of course:
I also have written a series on "bad habits to kick" which can give you some ideas of things to watch out for. The first post is here, and at the bottom of each post (I think 16 in total) there is a link to the next one:
As for your points:
1) NOLOCK should be used sparingly. If tempdb is not already a bottleneck, look at SNAPSHOT ISOLATION in 2005+.
4) there is essentially no difference between a cursor and a while loop - the underlying mechanics are essentially the same. Usually a set-based query works better than a row-by-row operation, but there are always exceptions.
First and most important, does it actually return the correct results! Also are all the changes requested actually put into the proc.
Replace any cursors with set-based logic not while loops!
Replace any dynamic sql in a stored proc if possible. This should be a last resort, not a first resort. If you must use dynamic sql, make sure to have a debug parameter which will show the code generated for the input values rather than running it. You will save no end of time when you need to adjust this or find out what went wrong with a particular set of values.
Never use select * especially if there are joins as at least one field will be sent twice.
If a table variable is used and a large data set is returned, suggest replacing with a temp table as it is likely to perform faster. Do not actually replace without performacne testing of course.
Replace any correlated subqueries with joins.
Use aliases and make sure that you put the alias on all the fields in the select. It is much easier to maintain if you don't have to figure out later which of the twelve tables in the join had that field.
Look for hard-coded values that can be parametized or pulled from an existing table. Especially if some of them will change with time (such as where mydate >'20090101'); these tend to cause bugs later.
If using Union, consider if Union All will work. It will be faster if so.
Check to see if left joins should really be inner joins and vice versa. Check to see if there is a left join with a value inthe where clause filtering on that table. Except for "Where myid is null", this will change a left join to an inner join. LIkely you have the worng results or you didn't need a left join. If you do need a left join, move this condition to the join and out of the where clause.
If you are doing more than one insert/update/delete make sure a transaction is used. Also make sure that any transactions have a way to rollback if one of the steps fails.
I'm sure there is a lot more, but this is a start.