August 11, 2010 at 5:17 am
I posted this on the microsoft forum and it turned into a discussion about stored procedures: http://social.msdn.microsoft.com/Forums/en-US/transactsql/thread/a4e87b4b-480c-4aeb-a4bb-a2da817a1392
I don't want to use stored procedures. I have my reasons.
So here it is again. Please see if you can offer some sql text.
I don't write SQL enough to get good at it. In this case I was forced to use a few loops. This is very slow and needs to be much faster.
I need to find customers that are active but have never been serviced.
First I look for records with no history at all and this works well:
private string getNewCustomers()
{
newcustomers = "";
string select = "SELECT DISTINCT tblLocation.ID FROM tblLocation ";
select += "LEFT JOIN tblManifest ON tblManifest.LocationID = tblLocation.ID ";
select += "LEFT JOIN tblManifestDetail ON tblManifestDetail.ManifestID = tblManifest.ID ";
select += "WHERE tblManifest.Manifest IS NULL ";
select += "AND tbllocation.Inactive = '0' ";
select += getRoute();
fillTableWithData(select);
DataRow dr = null;
for (int i = 0; i < ds.Tables[0].Rows.Count; ++i)
{
dr = ds.Tables[0].Rows;
newcustomers += "OR tblLocation.ID = '" + dr[0].ToString() + "' ";
}
newcustomers += getLocations(); // next function below
return (newcustomers);
}
But I also need to find customers that have been invoiced for setup charges but have never been serviced. This loop is very slow because I'm looking at one row at a time.
private string getLocations()
{
ConnectionStringSettings cS = ConfigurationManager.ConnectionStrings[1];
string connString = cS.ConnectionString;
SqlConnection conn = new SqlConnection(connString);
conn.Open();
DataTable temp = new DataTable();
string select = "SELECT DISTINCT tblLocation.ID FROM tblLocation ";
if (RouteradioButton.Checked) {
select += "WHERE tblLocation.Route IS NOT NULL ";
select += "AND tblLocation.Route <> '0-0' ";will call
}
else {
select += "WHERE tblLocation.VanRoute IS NOT NULL ";
select += "AND tblLocation.VanRoute <> '0-0' "; customer will call
}
select += "AND tbllocation.Inactive = '0' ";
select += getRoute();
SqlDataAdapter adapter = new SqlDataAdapter(select, conn);
try { adapter.Fill(temp); }
catch { }
string newcustomers = "";
for (int i = 0; i < temp.Rows.Count; ++i)
{
DataRow dr = temp.Rows;
if (!getLastManifests(Convert.ToInt32(dr[0]))) // next function below
{
dr = temp.Rows;
string s = "OR tblLocation.ID = '" + dr[0].ToString() + "' ";
if (!newcustomers.Contains(s)) newcustomers += s;
}
}
adapter.Dispose();
conn.Close();
conn.Dispose();
temp.Dispose();
return (newcustomers);
}
private bool getLastManifests(int LocationID) // find customers that have never been picked-up.
{
ConnectionStringSettings cS = ConfigurationManager.ConnectionStrings[1];
string connString = cS.ConnectionString;
SqlConnection conn = new SqlConnection(connString);
conn.Open();
DataTable temp = new DataTable();
string startdate = dateTimePicker2.Value.Date.ToString("MM/dd/yyy");
string select = "SELECT DISTINCT tblManifest.BillToName FROM tblManifest ";
select += "LEFT JOIN tblManifestDetail ON tblManifestDetail.ManifestID = tblManifest.ID ";
select += "WHERE (SELECT MAX(t1.ManifestDate) FROM tblManifest t1 WHERE t1.ID = tblManifestDetail.ManifestID) >= '" + startdate + "' ";
select += "AND tblManifest.LocationID = " + LocationID.ToString() + " ";
if (RouteradioButton.Checked)
{
select += "AND (tblManifestDetail.MatCode = '1AGA' OR tblManifestDetail.MatCode = '3AGA' OR tblManifestDetail.MatCode = '3EGA') ";
}
else
{
select += "AND (((tblManifest.ManifestType) = 'V') AND ((tblManifestDetail.CapacityCode) = '01' OR (tblManifestDetail.CapacityCode) = '03' ";
select += "OR (tblManifestDetail.CapacityCode) = '42' OR (tblManifestDetail.CapacityCode) = '55' OR (tblManifestDetail.CapacityCode) = '85' ";
select += "OR (tblManifestDetail.CapacityCode) = '95')) ";
}
SqlDataAdapter adapter = new SqlDataAdapter(select, conn);
bool found = false;
try { adapter.Fill(temp); }
catch { }
if (temp.Rows.Count > 0) found = true; // only interested in customers where found == 0
adapter.Dispose();
conn.Close();
conn.Dispose();
temp.Dispose();
return (found);
}
Here is sample SQL text for the three functions:
SELECT DISTINCT tblLocation.ID
FROM tblLocation
LEFT JOIN tblManifest ON tblManifest.LocationID = tblLocation.ID
LEFT JOIN tblManifestDetail ON tblManifestDetail.ManifestID = tblManifest.ID
WHERE tblManifest.Manifest IS NULL
AND tbllocation.Inactive = '0'
AND (tblLocation.Route LIKE '1-%')
SELECT DISTINCT tblLocation.ID
FROM tblLocation
WHERE tblLocation.Route IS NOT NULL
AND tblLocation.Route <> '0-0'
AND tbllocation.Inactive = '0'
AND (tblLocation.Route LIKE '1-%')
SELECT DISTINCT tblManifest.BillToName
FROM tblManifest
LEFT JOIN tblManifestDetail ON tblManifestDetail.ManifestID = tblManifest.ID
WHERE (SELECT MAX(t1.ManifestDate) FROM tblManifest t1 WHERE t1.ID = tblManifestDetail.ManifestID) >= '08/11/2008'
AND tblManifest.LocationID = 1765 AND (tblManifestDetail.MatCode = '1AGA' OR tblManifestDetail.MatCode = '3AGA' OR tblManifestDetail.MatCode = '3EGA')
I'm hoping someone can combine these (or at least the last two) for me which should speed it up a lot.
I know this will take some time and thank you for you time,
Dennis
August 11, 2010 at 5:22 am
You have come this far, why not go one step further and post some DDL and DMLs for your request . Please go through the following article on how to present in a neat format so that people will jump on and lend their help.
CLICK HERE FOR FORUM POSTING ETIQUETTES - BY JEFF MODEN[/url]
We are volunteers , Dennis, so some help from your side will attract large help from ours 🙂
August 11, 2010 at 6:08 am
dennisv (8/11/2010)
...I'm hoping someone can combine these (or at least the last two) for me which should speed it up a lot.
I know this will take some time and thank you for you time,
Dennis
I'm sure it's possible but it will take quite a lot more explanation than that given.
The value for LocationID in the third query - is this the value returned by the second query?
For fast, accurate and documented assistance in answering your questions, please read this article.
Understanding and using APPLY, (I) and (II) Paul White
Hidden RBAR: Triangular Joins / The "Numbers" or "Tally" Table: What it is and how it replaces a loop Jeff Moden
August 11, 2010 at 6:52 am
dennisv (8/11/2010)
...I don't write SQL enough to get good at it...
...
No offence, but do you seriously believe that writing "enough" in C# makes you good in it? 😉
Hornestly, before advising you on SQL, anyone would need to understand what you're trying to achieve and it cannot be done from the end queries you gave. Reading your c# code is not easy. But I can asure you, if you building your SQL as:
for (int i = 0; i < ds.Tables[0].Rows.Count; ++i)
{
dr = ds.Tables[0].Rows;
newcustomers += "OR tblLocation.ID = '" + dr[0].ToString() + "' ";
}
it is no going to perform good.
I have read through your discussion on MS forum. Your reasons for not using stored proc is quite pathetic. I'm afraid that failing to get right person to write proper SQL will not be rectified by having, even very good C# programmer who has no knowledge in SQL (and, hornestly, have kind of strange experience in designing applications working with relational databases). Does your system architect is happy about replacing stored procs with in-line SQL?
August 11, 2010 at 7:35 am
Eugene Elutin (8/11/2010)
dennisv (8/11/2010)
...I don't write SQL enough to get good at it...
...
No offence, but do you seriously believe that writing "enough" in C# makes you good in it? 😉
Hornestly, before advising you on SQL, anyone would need to understand what you're trying to achieve and it cannot be done from the end queries you gave. Reading your c# code is not easy. But I can asure you, if you building your SQL as:
for (int i = 0; i < ds.Tables[0].Rows.Count; ++i)
{
dr = ds.Tables[0].Rows;
newcustomers += "OR tblLocation.ID = '" + dr[0].ToString() + "' ";
}
it is no going to perform good.
I have read through your discussion on MS forum. Your reasons for not using stored proc is quite pathetic. I'm afraid that failing to get right person to write proper SQL will not be rectified by having, even very good C# programmer who has no knowledge in SQL (and, hornestly, have kind of strange experience in designing applications working with relational databases). Does your system architect is happy about replacing stored procs with in-line SQL?
Sorry I asked here.
Please eveyone just forget about it.
But yes, we are all happy to do away with 1000+ stored procedures and 400+ views and 350+ temporary tables.
August 11, 2010 at 8:13 am
Sorry mate, no one said that your solution need 1000+ stored procedures and 400+ views and 350+ temporary tables. Actuallty, no one here in position to advise about these numbers without knowing what your system/application is about.
However you should understand that there is nothing wrong with having even larger number of procs, views and temp tables. Its depend on the application/system.
Also, I have not said that you MUST use stored proc, I have only said that the reason you've used against using them are not sustainable.
At the end, if you provide more details (and clear requirements) of what you're trying to achieve together with DDL and some test data as per link in my signature, you will find a lot of people here who will help you with writing right SQL for the task you presented.
August 11, 2010 at 9:05 am
dennisv (8/11/2010)
Sorry I asked here.Please eveyone just forget about it.
But yes, we are all happy to do away with 1000+ stored procedures and 400+ views and 350+ temporary tables.
So let me get this straight. You've asked two of the top SQL forums for advice. One pointedly points you to a solution you don't like. You go to the second because you didn't like that and are told that your reasons on the first doesn't hold water, so you're leaving it. I guess that if you just don't like hearing sound advice, and will have to keep looking for someplace that will just give you what you want.
There are all kinds of reasons FOR using stored procedures. The biggest ones (IMO) are minimizing the risk of sql injection; that you can start utilizing cache plan reuse and a simple sql code change doesn't require an application build. The way the code is being written, where instead of using parameters you are coding the values directly into the code to be executed, you will never take advantage of cache plan reuse. This will cause your system to have to compile each and every query your application make. (Just know that if the query (not including the parameters) is not an exact match, it will be recompiled.) Not a good idea - minimizing these will greatly help a busy database server.
You might have some valid points on the use of views and temp tables... it all depends on how they are used. But I will tell you that if you seem to be having a problem with them, then it is because they aren't being used properly.
Since
I don't write SQL enough to get good at it.
, you might want to seriously listen to those that do.
Wayne
Microsoft Certified Master: SQL Server 2008
Author - SQL Server T-SQL Recipes
August 11, 2010 at 9:16 am
It's not rocket science, converting a query into a stored procedure or vice versa. So why not let us assume that you are writing a stored procedure? Then if you must, you can plug it into your client code when it's a wrap. This would give everyone a level playing field and remove a massive layer of complexity which frankly we can do without. Say someone comes up with a single query which actually does exactly what you want, and you plug it into your client code. It fails, not because there's something wrong with the query, but because there's an error somewhere in the client. Will it happen? Of course it will.
For fast, accurate and documented assistance in answering your questions, please read this article.
Understanding and using APPLY, (I) and (II) Paul White
Hidden RBAR: Triangular Joins / The "Numbers" or "Tally" Table: What it is and how it replaces a loop Jeff Moden
August 11, 2010 at 9:31 am
I won't enter the debate about stored procedures or not.
You have some problems in your SQL code. First, why are you using DISTINCT for all the queries? This is an aggregation function that will impact code and should only be used when needed. If you need it everywhere, you may have some structural problems with your tables and the data you're storing may be incorrect.
All the OR clauses can be causing scans. You might trying breaking those up into a series of UNION ALL queries.
Also, the MAX statement, another aggregation. You might see better performance using a TOP with an ORDER By.
Finally, as far as combining them goes... I suspect you can do that through LocationID. As an intitial trial, simply make the first query a derived table and join it to the second query, through the LocationId. Take a look at the resulting execution plan and figure out if there are other steps you can take to tune the query from there.
----------------------------------------------------The credit belongs to the man who is actually in the arena, whose face is marred by dust and sweat and blood... Theodore RooseveltThe Scary DBAAuthor of: SQL Server 2017 Query Performance Tuning, 5th Edition and SQL Server Execution Plans, 3rd EditionProduct Evangelist for Red Gate Software
August 11, 2010 at 10:10 am
cached query plans are not always good
if they are not reused they waste resources
and if they are dependent on parameters they are worse than compiling fresh
but if you are going to write dynamic sql statements like that
you could try a more robust approach:
StringBuilder sql = new StringBuilder();
sql.AppendLine("SELECT [Field]");
sql.AppendLine("FROM [Table]");
is more efficient, more readable and less prone to forgetting that trailing space you are using
"SQL Injection" is just a silly name for "not encoding literals properly"
create some helper functions:
public static string SqlName(string name) {
return "[" + name.Replace("]", "]]") + "]";
}//method
public static string SqlString(string name) {
return "'" + name.Replace("'", "''") + "'";
}//method
and then build on those until your SQL strings are constructed mostly from functions
less chance of mistakes and "sql injection" that way
check out sp_executesql (with parameter) as it handles encoding types for you - including dates which can easily catch you out
I will bet you in a year from now you will read this post and do a big face palm
Take care
August 11, 2010 at 10:19 am
Eugene Elutin (8/11/2010)
Sorry mate, no one said that your solution need 1000+ stored procedures and 400+ views and 350+ temporary tables. Actuallty, no one here in position to advise about these numbers without knowing what your system/application is about.However you should understand that there is nothing wrong with having even larger number of procs, views and temp tables. Its depend on the application/system.
Also, I have not said that you MUST use stored proc, I have only said that the reason you've used against using them are not sustainable.
At the end, if you provide more details (and clear requirements) of what you're trying to achieve together with DDL and some test data as per link in my signature, you will find a lot of people here who will help you with writing right SQL for the task you presented.
The project I'm replacing has 1000+ stored procedures and 400+ views and 350+ temporary tables.
Because of all the pain we've endured trying to dig through all these, I'm just not willing to add to that mess. We're looking forward to the day when we can delete all of it.
This is a small company with less than 50 tables that actually store live data.
I've replaced 60% of the old project and everything runs 4 times faster.
Most of the SQL has been straight forward. But as I said, I don't need to do anything fancy very often.
August 11, 2010 at 10:43 am
What I have so far was not meant to be the final product. I know the loops are poor.
I do what most of you do, I get it working any way possible, then I try and make it more efficient (why I asked here).
I'm trying to get a list of all the customers on a route (easy to do).
Then the customers that do not meet the third query conditions are added to the main query.
I may have to look at 100 customers, but only one or two will get added to the main query.
I just need a way to combine the 2nd and 3rd queries.
August 11, 2010 at 10:52 am
Have you looked at my response?
----------------------------------------------------The credit belongs to the man who is actually in the arena, whose face is marred by dust and sweat and blood... Theodore RooseveltThe Scary DBAAuthor of: SQL Server 2017 Query Performance Tuning, 5th Edition and SQL Server Execution Plans, 3rd EditionProduct Evangelist for Red Gate Software
August 11, 2010 at 11:04 am
Grant Fritchey (8/11/2010)
I won't enter the debate about stored procedures or not.
Thank you.
You have some problems in your SQL code. First, why are you using DISTINCT for all the queries? This is an aggregation function that will impact code and should only be used when needed. If you need it everywhere, you may have some structural problems with your tables and the data you're storing may be incorrect.
Sorry, left over from speed trials.
All the OR clauses can be causing scans. You might trying breaking those up into a series of UNION ALL queries.
Will do.
Also, the MAX statement, another aggregation. You might see better performance using a TOP with an ORDER By.
I tried a TOP but it was a lot slower, but I did not use an ORDER BY. How would that make it faster?
Finally, as far as combining them goes... I suspect you can do that through LocationID. As an intitial trial, simply make the first query a derived table and join it to the second query, through the LocationId. Take a look at the resulting execution plan and figure out if there are other steps you can take to tune the query from there.
Thank you.
August 11, 2010 at 11:33 am
dennisv (8/11/2010)
Sorry, left over from speed trials.
I'd drop them then. They're going to hurt you, guaranteed.
I tried a TOP but it was a lot slower, but I did not use an ORDER BY. How would that make it faster?
If there's an index that the query can use, TOP (1) with the ORDER BY clause can take advantage of it, usually better than a MAX statement, again, because MAX is an aggregate function. Funny thing is, if you looked at the execution plan, you might find that the MAX is actually using TOP on it's own. The optimizer picks up on stuff like that fairly frequently. You'll still usually be better off with the TOP though.
----------------------------------------------------The credit belongs to the man who is actually in the arena, whose face is marred by dust and sweat and blood... Theodore RooseveltThe Scary DBAAuthor of: SQL Server 2017 Query Performance Tuning, 5th Edition and SQL Server Execution Plans, 3rd EditionProduct Evangelist for Red Gate Software
Viewing 15 posts - 1 through 15 (of 39 total)
You must be logged in to reply to this topic. Login to reply