Stored Procedure speed versus Select Statement

Question

Stored Procedure speed versus Select Statement

Viewing 13 posts - 16 through 27 (of 27 total)

You must be logged in to reply to this topic. Login to reply

Jeff Moden SSC Guru Points: 1004362 More actions · Answer 1

The best thing to do, if it can be done, is to use variables like the original code. Of course, the OR still needs to be fixed as I suggested.

--Jeff Moden

RBAR is pronounced "ree-bar" and is a "Modenism" for Row-By-Agonizing-Row.
First step towards the paradigm shift of writing Set Based code:
________Stop thinking about what you want to do to a ROW... think, instead, of what you want to do to a COLUMN.

Change is inevitable... Change for the better is not.

Helpful Links:
How to post code problems
How to Post Performance Problems
Create a Tally Function (fnTally)

Sue-651097 Say Hey Kid Points: 675 More actions · Answer 2

hi jeff,

what do you think: if you have serveral parameters in the where clause, which can be choosen in every possible way (e.g. the interface allows you to pick: state, country, rating, zipcode between and, lastname,...) from the user interface and you can pick 2, 3, none whatever - is it better to

send all parameters to the sql server anyway and implement a lot of if's to check if the parameter was selected and then generate the sql string dynamically in the procedure?

i always generate the where clause in the UI and call a procedure, mentioned before (the user has only permisson to procedures. for sql injection he has to hack my admin account.)

whats your opinion?

Susanne

ajoy_gochhayat SSC Enthusiast Points: 167 More actions · Answer 3

It seems you have huge data update, insert and delete in the table. Please try to re-organise the table index and the query will execution will be faster.

Jeff Moden SSC Guru Points: 1004362 More actions · Answer 4

I probably wouldn't do a lot of "IF's" although those can be quite effective provided that it's not building dynamic SQL. A well formed WHERE clause with CASE statements does the trick in most cases.

--Jeff Moden

RBAR is pronounced "ree-bar" and is a "Modenism" for Row-By-Agonizing-Row.
First step towards the paradigm shift of writing Set Based code:
________Stop thinking about what you want to do to a ROW... think, instead, of what you want to do to a COLUMN.

Change is inevitable... Change for the better is not.

Helpful Links:
How to post code problems
How to Post Performance Problems
Create a Tally Function (fnTally)

Patrick Russell-486609 SSC Veteran Points: 219 More actions · Answer 5

Thanks Jeff, I tried the restructured WHERE clause, and there is no noticable difference in the time. Removing the DISTINCT keyword still causes it to return duplicates.

It seems that the dynamicSQL is the way to go, the question is how little. What would be the best way to validate the incoming values to make sure that thay are safe? If the input parameters are defined as int, how much damage can an attacker cause? Can SQL injection be accomplished if I validate the incoming values as int?

Patrick

Gail Shaw SSC Guru Points: 1004504 More actions · Answer 6

Patrick Russell (11/11/2008)
It seems that the dynamicSQL is the way to go, the question is how little. What would be the best way to validate the incoming values to make sure that thay are safe? If the input parameters are defined as int, how much damage can an attacker cause? Can SQL injection be accomplished if I validate the incoming values as int?

If you parameterise the dynamic SQL and use sp_executesql, like I'm suggesting, there's no risk of SQL injection. The risk comes in when you concatenate pieces of SQL string together, like in Sue's example

-- assume 3 parameters to a proc that contains this code, @locSITEID, @locSHOWID and @locBUCKETTOISSUEID

DECLARE @sSQL nvarchar(2000), @WhereClause mVARCHAR(2000)

SET @sSQL = 'SELECT col1, col2, col3 FROM SomeTable '

IF @locSITEID IS NOT NULL

SET @WhereClause = 'SiteID = @SiteID '

IF LEN(@WhereClause) !=0

SET @WhereClause = @WhereClause + ' AND '

IF @locSHOWID IS NOT NULL

SET @WhereClause = 'ShowID = @ShowID'

IF LEN(@WhereClause) !=0

SET @WhereClause = @WhereClause + ' AND '

If @locBUCKETTOISSUEID IS NOT NULL

SET @WhereClause = 'BucketID = @BucketID'

IF LEN(@WhereClause) !=0

SET @sSQL = @sSQL + ' WHERE ' + @WhereClause

exec sp_executesql @sSQL, '@SiteID int, @ShowID int, @BucketID int', @SiteID = @locSITEID, @ShowID = @locSHOWID , @BucketID = @locBUCKETTOISSUEID

If you look at that, you'll notice that I'm not ever concatenating the value of a parameter into a string that will be executed. What I'm doing is building up a parameterised query based on the values of certain variables/parameters and then executing that and passing the parameters in. It requires the user to have rights on the base tables, but there's no chance of injection from that.

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

We walk in the dark places no others will enter
We stand on the bridge and no one may pass

Gail Shaw SSC Guru Points: 1004504 More actions · Answer 7

Sue (11/11/2008)
thanks for your hint, Jeff.
but isn't this better than to give users permissions on tables and generate the whole select statement dynamically as mentioned before?

But that's pretty much what you're doing. Any form of dynamic SQL breaks the ownership chain and requires that the users have permissions on the base tables and having the entire where clause concatenated in is far more risky than building up a parameterised query piece by piece and executing that. What if your where clause read '1=1; delete from tblxy'

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

We walk in the dark places no others will enter
We stand on the bridge and no one may pass

Sue-651097 Say Hey Kid Points: 675 More actions · Answer 8

thanks Gail. I see what you mean and will do it your way from now on. 🙂

sql injection: but first of all he has to know the name of the table and then he has no delete permissions to the table. but anyway, it is dangerous for sure.

Susanne

Jeff Moden SSC Guru Points: 1004362 More actions · Answer 9

Patrick Russell (11/11/2008)
Thanks Jeff, I tried the restructured WHERE clause, and there is no noticable difference in the time. Removing the DISTINCT keyword still causes it to return duplicates.
It seems that the dynamicSQL is the way to go, the question is how little. What would be the best way to validate the incoming values to make sure that thay are safe? If the input parameters are defined as int, how much damage can an attacker cause? Can SQL injection be accomplished if I validate the incoming values as int?
Patrick

Gail pretty much summed it up.

Also, can you post your restructured WHERE clause, please?

--Jeff Moden

RBAR is pronounced "ree-bar" and is a "Modenism" for Row-By-Agonizing-Row.
First step towards the paradigm shift of writing Set Based code:
________Stop thinking about what you want to do to a ROW... think, instead, of what you want to do to a COLUMN.

Change is inevitable... Change for the better is not.

Helpful Links:
How to post code problems
How to Post Performance Problems
Create a Tally Function (fnTally)

RBarryYoung SSC Guru Points: 143329 More actions · Answer 10

Sue (11/11/2008)
sql injection: but first of all he has to know the name of the table and then he has no delete permissions to the table.

Right, but counting on that is known as "Security through Obscurity" and it is generally understood to be a security fallacy (as in "Not secure"), because you are relying on the user's ignorance of things that you are not actually in a position to keep secure: like the names of your company's tables. You have effectively moved your security perimeter from things that you can control (like what SQL code gets executed on your server) to things you cannot control (like how many people know what your database schema is).

[font="Times New Roman"]-- RBarryYoung[/font], [font="Times New Roman"] (302)375-0451[/font] blog: MovingSQL.com, Twitter: @RBarryYoung [font="Arial Black"]
Proactive Performance Solutions, Inc. [/font][font="Verdana"] "Performance is our middle name."[/font]

Gail Shaw SSC Guru Points: 1004504 More actions · Answer 11

rbarryyoung (11/11/2008)
Right, but counting on that is known as "Security through Obscurity" and it is generally understood to be a security fallacy (as in "Not secure"), because you are relying on the user's ignorance of things that you are not actually in a position to keep secure: like the names of your company's tables.

And it's not hard to find out the names of the other tables, especially if the web site/app has descriptive errors turned on. Find a place where any number of records are displayed and injection is possible and tag a "union all select name from sysobjects where xtype='U'" on the end. Tweak columns and positions until no error is returned.

With patience, it's even possible to work out most of the schema without using the system tables, just by paying attention to the errors returned. I have done that to a system (with permission). It's time consuming, and the automated attacks won't do it, but it certainly is possible.

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

We walk in the dark places no others will enter
We stand on the bridge and no one may pass

Patrick Russell-486609 SSC Veteran Points: 219 More actions · Answer 12

Jeff Moden (11/11/2008)
Also, can you post your restructured WHERE clause, please?

Below is the code using the restructured WHERE clause you posted. In testing I saw no noticable difference; 8-10 seconds. And when I removed the DISTINCT keyword, It still returns duplicates.

SELECT DISTINCT ET.iId, ET.cName, C.cCategoryName

FROM tbl_ReportIssues RI

FULL OUTER JOIN tbl_EquipmentTypes ET

INNER JOIN tbl_Categories C

ON ET.iCategoryId = C.iCategoryId

INNER JOIN tbl_Issues I

ON ET.iCategoryId = I.iCategoryId

INNER JOIN tbl_EquipTypeToShow ETS

ON ET.iId = ETS.iEquipTypeId

INNER JOIN tbl_BucketsToIssues BI

ON I.iIssueId = BI.iIssueId

LEFT OUTER JOIN tbl_ReportIssueDetails RID

ON ETS.iEquipTypeId = RID.iEquipTypeId

ON RI.iReportIssueId = RID.iReportIssueId

AND RI.iBucketToIssueId = BI.iBucketToIssueId

WHERE

(

(ET.iSiteId = @locSITEID)

AND (ETS.iShowId = @locSHOWID OR @locSHOWID IS NULL)

AND (BI.bDeleted = 0)

AND (ETS.bDeleted = 0)

AND (BI.iBucketToIssueId = @locBUCKETTOISSUEID OR @locBUCKETTOISSUEID IS NULL)

)

OR

(

(ET.iSiteId = @locSITEID)

AND (ETS.iShowId = @locSHOWID OR @locSHOWID IS NULL)

AND (BI.iBucketToIssueId = @locBUCKETTOISSUEID OR @locBUCKETTOISSUEID IS NULL)

AND (RI.iReportId = @locREPORTID OR @locREPORTID IS NULL)

)

Patrick

Jeff Moden SSC Guru Points: 1004362 More actions · Answer 13

Patrick Russell (11/12/2008)
Jeff Moden (11/11/2008)
Also, can you post your restructured WHERE clause, please?
Below is the code using the restructured WHERE clause you posted. In testing I saw no noticable difference; 8-10 seconds. And when I removed the DISTINCT keyword, It still returns duplicates.

That because I apparently didn't have enough coffee when I made the suggestion... there is no difference in the code and I just didn't see it that way. Thanks and sorry about the bum answer.

--Jeff Moden

RBAR is pronounced "ree-bar" and is a "Modenism" for Row-By-Agonizing-Row.
First step towards the paradigm shift of writing Set Based code:
________Stop thinking about what you want to do to a ROW... think, instead, of what you want to do to a COLUMN.

Change is inevitable... Change for the better is not.

Helpful Links:
How to post code problems
How to Post Performance Problems
Create a Tally Function (fnTally)