Comments posted to this topic are about the item A Simple Introduction to Dynamic SQL
You quite rightly say
When placing your Dynamic SQL code into production (typically in stored procedures), be careful about concatenating alphanumeric parameters directly because of SQL injection.
but a few lines previously, you've done exactly that, ending up with a
Shouldn't you be recommending the use of sp_executesql from the outset?
Hi Julian, I was trying to focus more on the basic principals of how sql statements are created dynamically.
But you make a very good point, I should probably have nudged the readers towards using sp_executesql from the start.
For those of you reading this, here is a link to what Julian is referring to:
Instead of escaping the single tick, I find it more straight forward to use QUOTENAME:
SELECT @Result = 'Total of ' + QUOTENAME(CAST(@NoOfColumns AS VARCHAR), CHAR(39)) + ' column(s)';
or use a quote;
SELECT @Result = 'Total of "' + CAST(@NoOfColumns AS VARCHAR) + '" column(s)';
julian.fletcher - Monday, February 12, 2018 2:16 AM
Also, there's nothing about sp_executesql that makes it immune to SQL injection. sp_executesql allows for dynamic SQL to be parameterised, but not everything can be parameterised.
Specifically database name can't.
SELECT @SQL ='SELECT COUNT(1) ' +'FROM [' + @DBName + '].[dbo].[CommonTable] ' +'WHERE [InsertDate] = ''' + CAST(@Date AS VARCHAR) + ''''
Hence, without some whitelisting, that will still be vulnerable, even if run with sp_executesql.
Microsoft Certified Master: SQL Server, MVP, M.Sc (Comp Sci)
SQL In The Wild: Discussions on DB performance with occasional diversions into recoverability
I would have used the concat function SQL Server 2012 or better, eliminate the cast by letting concat handle the char(10) cast and use sysdatetime() instead of Getdate().
declare @DBName as varchar(20);
set @DBName = 'Branch1';
declare @Date as date;
set @Date = sysdatetime() --not Getdate();
declare @SQL as varchar(2000);
= 'SELECT COUNT(1) ' + 'FROM [' + @DBName + '].[dbo].[CommonTable] ' + 'WHERE [InsertDate] = '''
+ cast(@Date as char(10)) + '''';
= concat('SELECT COUNT(1) FROM [', @DBName, '].[dbo].[CommonTable] WHERE [InsertDate] = ', quotename(@Date, ''''));
Nice article. Dynamic SQL is one of those things you don't use very often, and a simple article like this is useful for refreshing your memory on the basic principle.
Peter Heller - Monday, February 12, 2018 5:42 AM
SysDateTime() returns more bytes, which isn't necessary here because you're dumping the result to a variable with the DATE datatype.
As a bit of a sidebar and , SysDateTime() is relatively crippled because it returns a DATETIME2() datatype compared to GETDATE() which uses the powerful DATETIME datatype. DATETIME supports incredibly easy to use direct date/time math for period calculations (which is in the ISO standards) where DATETIME2() does not (at least not in SQL Server because they screwed it up). If you use it to support supposed "portable code", true portability is a myth and can't actually be accomplished to any great degree.
Change is inevitable... Change for the better is not.
In our shop we have both professional SQL programmers and non professional SQL users. The rampant use of dynamic SQL by the non-professionals is one of the most common sources of poor performance. We handle this (but not always well) by isolating the non-professionals to their own server.
I second Julian's recommendation, as someone who wants to learn, I don't just want to know that something is possible if it's a bad idea, I want to know that "some people recommend this <code></code> but that is a bad idea because of XYZ, and the right way to do it is ZYX"
The point of learning from sites like this is standing on the shoulders of those who have come before, so I can get farther faster.
Please follow Best Practices For Posting On Forums to receive quicker and higher quality responses
Thanks everyone for the feedback and constructive comments.
DECLARE @crlf CHAR(4)
DECLARE @v_SQL varchar(MAX) = ''
SET @crlf = CHAR(10) + CHAR(13)
SELECT @v_SQL = @v_SQL + 'DELETE FROM dbo.Some_Table' + ' ' + @crlf --can add carriage return line feed for real complex sql, makes it easier to debug
SELECT @v_SQL = @v_SQL + 'WHERE Person = ' + CHAR(39) + 'JONES' + CHAR(39) --use char(39) instead of ''', easier to read especially if you are doint this alot
Thanks for the article. My only suggestion would be to move the SQL injection warning into a prominent box at the top of the article. Many readers might jump right into using dynamic SQL without taking into account the security implications.
And maybe also add a link to "The Curse and Blessings of Dynamic SQL," by Erland Sommarskog:
A SQL query walks into a bar and sees two tables. He walks up to them and asks, "Can I join you?"
webrunner - Monday, February 12, 2018 12:40 PM
I agree with this sentiment. This is a good introduction to the existence of dynamic SQL, and there's not really a great, gentle way to properly and succinctly introduce Dynamic SQL. DSQL is like a dangerous weapon; one you only want the most skilled people handling. But if you don't have those skilled people, where do you start them? Certainly you've got to start somewhere. But I'm of the opinion that it's use should come with a big sign saying "BEWARE OF THE LEOPARD".
Chris Hurlbut - Monday, February 12, 2018 12:10 PM
DECLARE @v_SQL Nvarchar(MAX) -- Should be Nvarchar
DECLARE @QT Nchar(1) = NCHAR(39) -- declare the quote character as a variable rather than using the function later.
SET @v_SQL = CONCAT( -- Using CONCAT can make it easier to write and read, especially for casts
N'DELETE FROM dbo.Some_Table', @crlf,
N'WHERE Person = ' , @QT, N'JONES', @QT,
Viewing 15 posts - 1 through 15 (of 16 total)