April 10, 2012 at 6:17 am
Dear SQLCentral.com members and administrators,
Hello guys, a greeting of peace.
It is my first time here so I want to say sorry if in case this thread of mine is not in a proper discussion thread, because to be honest I am new to SQL Server and some technicalities are still broad to me, but I am trying my best to study more about Microsoft SQL server.
I created a table named [font="Courier New"]Student[/font] and has four (4) fields: [font="Courier New"]id[/font], [font="Courier New"]fn[/font], [font="Courier New"]mi[/font], and [font="Courier New"]ln[/font] respectively (all are [font="Courier New"]NVARCHAR(20)[/font] type).
I entered two (2) dummy data on my table, and here are the data:
id fn mi ln
-------------------------------
001 PETER P PARKER
002 ANTHONY A STARK
I created a stored procedure named [font="Courier New"]SearchRecords[/font], this will be used in my C# program that will search for a data depending if he/she want to search by [font="Courier New"]id[/font], or by [font="Courier New"]fn[/font], or by [font="Courier New"]mi[/font], or by [font="Courier New"]ln[/font] . Here is the code of my stored procedure:
CREATE PROCEDURE SearchRecords
@searchQuery AS NVARCHAR(100),
@col AS VARCHAR(100)
AS
BEGIN
SET NOCOUNT ON;
DECLARE @sql NVARCHAR(1000)
DECLARE @value NVARCHAR(20)
SET @value = @searchQuery
IF (@col = 'PERSON_ID')
BEGIN
SET @sql = 'SELECT * FROM [Student] WHERE [id] = ' + @value
END
ELSE IF (@col = 'FIRST_NAME')
BEGIN
SET @sql = 'SELECT * FROM [Student] WHERE [fn] = ' + @value
END
EXEC (@SQL)
END
I tried to execute my stored procedure to check if it's working before I use it in my code in C#.
I execute this in a stored procedure using a new query:
EXEC SearchRecords '001','id'
I get this result (which is good):
id fn mi ln
----------------------------
001 PETER P PARKER
But when I try to change my seary query, like I want to search for a first name 'ANTHONY':
EXEC SearchRecords 'ANTHONY','fn'
I get this result:
[font="Courier New"]Invalid column name 'ANTHONY'.[/font]
I tried to change the data types, arrangement of variables, but still the same.
I know it is very poorly constructed stored procedure, that is why I am asking some help what revision/s should I make in order to this stored procedure much flexible, becasue I do not want to create four (4) stored procedures for searching on each field.
Thank you in advance and more power.
Warm regards,
Mark Squall 🙂
________________________________
"Listen to advice and accept instruction, and in the end you will be wise." -Proverbs 19:20
April 10, 2012 at 6:39 am
There is absolutely no need for dynamic SQL here (and the problem was that there were no quotes around the concatenated string in the dynamic SQL.
CREATE PROCEDURE SearchRecords
@searchQuery AS NVARCHAR(100),
@col AS VARCHAR(100)
AS
BEGIN
SET NOCOUNT ON;
IF (@col = 'PERSON_ID')
SELECT * FROM [Student] WHERE [PERSON_ID] = @searchQuery
IF (@col = 'FIRST_NAME')
SELECT * FROM [Student] WHERE [FIRST_NAME] = @searchQuery
END
Edit: fixed parameters.
Gail Shaw
Microsoft Certified Master: SQL Server, MVP, M.Sc (Comp Sci)
SQL In The Wild: Discussions on DB performance with occasional diversions into recoverability
April 10, 2012 at 6:40 am
The first query works (when you correct the columns to match the parameters) because you're getting implicit type conversions to numbers. '001' becomes 1 on both sides of the equals statement.
The second query errors because you can't convert ANTHONY to an integer.
This stored procedure is rife with injection possibilities.
Read either or both of these articles on a better way to do this.
http://www.sommarskog.se/dyn-search-2005.html
http://sqlinthewild.co.za/index.php/2009/03/19/catch-all-queries/
The methodologies done here are applicable to this kind of query.
April 10, 2012 at 6:42 am
GilaMonster (4/10/2012)
There is absolutely no need for dynamic SQL here (and the problem was that there were no quotes around the concatenated string in the dynamic SQL.
Really? Wow... I recall a time when if you had two queries like this nested inside of IF statements
(granted this is a really simple one) that the compiler would lose its mind.
Then again I was probably working with more complicated queries at the time... 🙂
April 10, 2012 at 6:43 am
That said, this is a bad approach for a number of reasons and you should consider multiple procedures.
This is just one reason:
http://sqlinthewild.co.za/index.php/2009/09/15/multiple-execution-paths/
Another reason has to do with the 'single responsibility' principal. You wouldn't write a C# class that could do one of several different things depending on a parameter passed to the constructor, similarly you shouldn't do that in T-SQL.
Gail Shaw
Microsoft Certified Master: SQL Server, MVP, M.Sc (Comp Sci)
SQL In The Wild: Discussions on DB performance with occasional diversions into recoverability
April 10, 2012 at 6:46 am
GilaMonster (4/10/2012)
That said, this is a bad approach for a number of reasons and you should consider multiple procedures.This is just one reason:
http://sqlinthewild.co.za/index.php/2009/09/15/multiple-execution-paths/
Another reason has to do with the 'single responsibility' principal. You wouldn't write a C# class that could do one of several different things depending on a parameter passed to the constructor, similarly you shouldn't do that in T-SQL.
Well technically isn't this just a varianet on the catch-all?
My statement about losing its mind was actually in reference to your article above (something I knew about but you do such a better job writing about).
April 10, 2012 at 6:46 am
mtassin (4/10/2012)
Really? Wow... I recall a time when if you had two queries like this nested inside of IF statements(granted this is a really simple one) that the compiler would lose its mind.
See the link I just posted. The solution however is not dynamic SQL usually. It's multiple procedures.
p.s. The reason the OP's code broke was not implicit conversions, the first_name column is nvarchar. It broke because the dynamic SQL would have been this at the point the query was executed
SELECT * FROM [Student] WHERE [FIRST_NAME] = ANTHONY
No quotes for the string value means that what was intended as a string literal would have been interpreted as a column name.
Gail Shaw
Microsoft Certified Master: SQL Server, MVP, M.Sc (Comp Sci)
SQL In The Wild: Discussions on DB performance with occasional diversions into recoverability
April 10, 2012 at 6:52 am
GilaMonster (4/10/2012)
mtassin (4/10/2012)
Really? Wow... I recall a time when if you had two queries like this nested inside of IF statements(granted this is a really simple one) that the compiler would lose its mind.
See the link I just posted. The solution however is not dynamic SQL usually. It's multiple procedures.
p.s. The reason the OP's code broke was not implicit conversions, the first_name column is nvarchar. It broke because the dynamic SQL would have been this at the point the query was executed
SELECT * FROM [Student] WHERE [FIRST_NAME] = ANTHONYNo quotes for the string value means that what was intended as a string literal would have been interpreted as a column name.
Right... though again it's lack of caffeine here...
The other one turned into
SELECT * FROM [Student] WHERE [ID] = 001
Which would get converted because he went dynamic... no?
The second one couldn't do that... I should have been more clear... for the reason you stated above.
April 10, 2012 at 7:12 am
mtassin (4/10/2012)
The other one turned into
SELECT * FROM [Student] WHERE [ID] = 001Which would get converted because he went dynamic... no?
That would be an implicit conversion whether the query is dynamic or not. 001 is numeric (no quotes), so the column gets converted to a matching numeric type (integer in this case)
Gail Shaw
Microsoft Certified Master: SQL Server, MVP, M.Sc (Comp Sci)
SQL In The Wild: Discussions on DB performance with occasional diversions into recoverability
April 10, 2012 at 7:20 am
Right... but if it hadn't been dynamic it would likely have been
SELECT * FROM [Student] WHERE [PERSON_ID] = @value
Which wouldn't have gone implicit. Because he chose to use dynamic SQL it went implicit because the appending of the variable onto the end of the query made it so.
Which is what I was trying to say when I explained why execution 1 worked, and why execution 2 didn't..
1 worked because 001 was implicitly converted to a number and so the column was also implicitly converted into a number.
2 didn't work because the word ANTHONY couldn't be converted into anything and instead was treated as if it was a column.
April 10, 2012 at 7:21 am
Dear Ms. Gail Shaw and Sir Mark Tassin,
Hello, thank you for the information and the links you have given to me. I will read them for another added knowledge on my part. Thank you because now I know I am creating a dynamic SQL (I am really sorry I did not knew what it is called). 😉
To Ms. Gail Shaw:
Actually Mam, that is my first plan, to create four (4) stored procedures for each search categories(i.e. search by [font="Courier New"]id[/font], search by [font="Courier New"]fn[/font], etc). Your link was useful. I just thought having four (4) different stored procedures that do the same thing (searching that is) will be considered as a bad practice.
To Ms. Gail Shaw and Sir Mark Tassin:
Does using sub stored procedure than dynamic SQL makes me less of an SQL programmer? Or is it the way around? Or any strategies will do as long as I achieve my goal? I am asking this because I do not know what would be the best practice and what are the standards used in the industry regarding my problem.
Thank you, thank you for all the members that keeps on giving me new ideas. And more power.
Warm regards,
MarkSquall
________________________________
"Listen to advice and accept instruction, and in the end you will be wise." -Proverbs 19:20
April 10, 2012 at 7:24 am
Dynamic SQL should be used when there's no other way of solving the problem. It's not the first thing you should think of. Also, a 'good SQL programmer' (whatever that means) should use the simplest, easiest solutions, not look for 'clever' or complex ways.
p.s. I am not a Sir.
Gail Shaw
Microsoft Certified Master: SQL Server, MVP, M.Sc (Comp Sci)
SQL In The Wild: Discussions on DB performance with occasional diversions into recoverability
April 10, 2012 at 7:29 am
The correct answer, as always is... it depends.
If you choose to use Dynamic SQL, then you need to be warey of injection.
Creating a dynamic SQL string by appending text type variables onto it, especially those that come from parameters opens you up to injection.
Using EXEC(@variable) instead of EXEC sp_executesql @varaible is also poor form
Personally I have nothing against Dynamic SQL... So long as it's used responsibly.
for instance, based on your stored proc.... what if I passed
;DELETE Student
in as the value for @searchQuery ?
Your query would turn into
SELECT * FROM [Student] WHERE [ID] = ;DELETE Student
OR
SELECT * FROM [Student] WHERE [FN] = ;DELETE Student
Which would probably not be what you wanted.
April 10, 2012 at 7:59 am
mtassin (4/10/2012)
The correct answer, as always is... it depends.If you choose to use Dynamic SQL, then you need to be warey of injection.
Creating a dynamic SQL string by appending text type variables onto it, especially those that come from parameters opens you up to injection.
Using EXEC(@variable) instead of EXEC sp_executesql @varaible is also poor form
Personally I have nothing against Dynamic SQL... So long as it's used responsibly.
for instance, based on your stored proc.... what if I passed
;DELETE Student
in as the value for @searchQuery ?
Your query would turn into
SELECT * FROM [Student] WHERE [ID] = ;DELETE Student
OR
SELECT * FROM [Student] WHERE [FN] = ;DELETE Student
Which would probably not be what you wanted.
Problems with dynamic SQL are not only limited to SQL Injection (although that is a big one). You also are looking at possibly filling your plan cache with junk if you have 30/300/3000 variations on a query based on changing values, parameters, etc. You're also hitting the CPU, requiring more compiles. You're getting less plan reuse. You have more opportunities for poorly written queries. You have a tougher time identifying and tuning queries.
I'm sure I can come up with more reasons if I thought about it some more. I agree, there's nothing wrong with dynamic SQL, used in the appropriate places at appropriate times as long as it's well written, preferably using sp_executesql and paramters to help avoid SQL Injection and promote more, and better, plan reuse. But just a blanket statement saying "Yeah, it's fine, have fun, avoid injection" is a little dangerous to communicate, especially to someone just getting started.
"The credit belongs to the man who is actually in the arena, whose face is marred by dust and sweat and blood"
- Theodore Roosevelt
Author of:
SQL Server Execution Plans
SQL Server Query Performance Tuning
April 10, 2012 at 8:11 am
Ok Grant, so now I can learn...
If he wrote it as
ALTER PROCEDURE [dbo].[SearchRecords]
@searchQuery AS NVARCHAR(100),
@col AS VARCHAR(100)
AS
BEGIN
SET NOCOUNT ON;
DECLARE @sql NVARCHAR(1000)
DECLARE @value NVARCHAR(20)
SET @value = @searchQuery
IF (@col = 'PERSON_ID')
BEGIN
SET @sql = 'SELECT * FROM [Student] WHERE [ID] = @value'
END
ELSE IF (@col = 'FIRST_NAME')
BEGIN
SET @sql = 'SELECT * FROM [Student] WHERE [FN] = @value'
END
EXEC sp_executesql @sql,N'@value nvarchar(20)',@value=@value
END
Yes there's a CPU penalty to pay, but wouldn't it wind up with just three plans in the cache? The original one that builds the query plus one for branch one, one for branch two and the parameter passed in would just be peachy?
I don't understand how we get that many more plans into the cache by doing this or by creating two separate procedures and then executing them both. Seems I get two plans in the cache by creating two procs, and 3 if we go dynamic (one for the proc that builds the dynamic code, and one for each path of the code itself).
I'm sure I'm missing something, which is why I tend to be more quiet around here and just read what you, Gail and Jeff tell me, but I thought I understood this one (more or less, though I did flub my terminology).
Viewing 15 posts - 1 through 15 (of 25 total)
You must be logged in to reply to this topic. Login to reply