SQL Stored Procedure Issue

1

I am trying to create a stored procedure to replace the following code:

' -- Show Vehicle Options --
Sub VehicleOptions1(strSelected, strFieldID, strFieldName, strVehicleFieldName, strTable, intSpace)
	' -- Create Recordset --
	strSQL = "SELECT " & strFieldID & ", " & strFieldName & " FROM " & strTable & " ORDER BY " & strFieldName
	Set objRS = siteConn.Execute(strSQL)
	If Not objRS.EOF Then
		response.write Space(intSpace) & "<ul>" & vbCrLf
		Do While Not objRS.EOF
			response.write Space(intSpace + 2) & "<li><input type=""checkbox"" class=""checkbox"" name=""" & strVehicleFieldName & """ value=""" & objRS(strFieldID) & """"
			If Not CheckBlank(strSelected) Then
				arrValues = CreateArray(strSelected)
				If IsArray(arrValues) Then
					For x = 0 To UBound(arrValues)
						If CStr(arrValues(x)) = CStr(objRS(strFieldID)) Then
							response.write " checked"
							Exit For
						End If
					Next
				End If
			End If
			response.write ">" & objRS(strFieldName) & "<span>(" & VehicleCount(strVehicleFieldName, objRS(strFieldID), "", "", "") & ")</span></li>" & vbCrLf
		objRS.MoveNext
		Loop
		response.write Space(intSpace) & "</ul>" & vbCrLf
	End If
	objRS.Close
	Set objRS = Nothing
End Sub

Within this code, I am creating a dynamic recordset to create a select menu for different table ID's and names. In addition, I am calling another function that performs a COUNT function for that ID in my tbl_Vehicles table. This is running VERY slow. I want to grab the ID, name, and COUNT in ONE query by creating a stored procedure. Here is what I created:

CREATE PROCEDURE [dbo].[sp_VehicleOptions]
	@strFieldID varchar(50) = NULL,
	@strFieldName varchar(100) = NULL,
	@strTable varchar(100) = NULL
AS
	DECLARE @strSQL varchar(max)
BEGIN
	SET NOCOUNT ON;

	-- Create Recordset --
	SET @strSQL = @strSQL + 'SELECT ' + @strFieldID + ', ' + @strFieldName + ', '
	SET @strSQL = @strSQL + '(SELECT COUNT(' + @strFieldID + ') FROM tbl_Vehicles WHERE ' + @strFieldName + ' = ' + @strFieldID + ') As VehicleCount '
	SET @strSQL = @strSQL + 'FROM ' + @strTable + ' ORDER BY ' + @strFieldName

	EXECUTE @strSQL
END

For whatever reason, it is not returning any rows. Any thoughts?

2

First, initialize the @strSQL variable:

DECLARE @strSQL varchar(max) = '';

If that doesn't fix the problem, instead of EXECUTE @strSQL, use PRINT @strSQL to get the dynamic sql that will be executed and see if that makes sense. If you run that string from a query window, does it return the results you want? If not, examine why not.

When you use dynamic SQL, it opens up the possibilities for SQL injection attacks. Your code is especially vulnerable to that. At the very least, use sp_executesql instead of exe

3

That seemed to solve my initial problem. Now, when I run, I am getting the following error:

  Error -2147217900 

  SELECT vm_id, vm_name, (SELECT COUNT(vm_id) FROM tbl_Vehicles WHERE vm_name = vm_id) As VehicleCount FROM tbl_VehicleMake ORDER BY vm_name

  execute sp_VehicleOptions @strFieldID = 'vm_id', @strFieldName = 'vm_name', @strTable = 'tbl_VehicleMake'

Also, the COUNT part of my code seems to be wrong. It should be counting the number of instances the vehicle make in the row is found in the tbl_Vehicles table.

Updated stored procedure:

CREATE PROCEDURE [dbo].[sp_VehicleOptions]
	@strFieldID varchar(50) = NULL,
	@strFieldName varchar(100) = NULL,
	@strTable varchar(100) = NULL
AS
	DECLARE @strSQL varchar(max) = '';
BEGIN
	SET NOCOUNT ON;

	-- Create Recordset --
	SET @strSQL = @strSQL + 'SELECT ' + @strFieldID + ', ' + @strFieldName + ', '
	SET @strSQL = @strSQL + '(SELECT COUNT(' + @strFieldID + ') FROM tbl_Vehicles WHERE ' + @strFieldID + ' = ' + @strFieldID + ') As VehicleCount '
	SET @strSQL = @strSQL + 'FROM ' + @strTable + ' ORDER BY ' + @strFieldName

	PRINT @strSQL;
	EXECUTE @strSQL
END
4

When I remove the COUNT portion, I get the following error:

 Could not find stored procedure 'SELECT vm_id, vm_name FROM tbl_VehicleMake ORDER BY vm_name'.
5

I think that

EXECUTE @strSQL

needs to be

EXECUTE (@strSQL)

Is this right?

(SELECT COUNT(' + @strFieldID + ') FROM tbl_Vehicles WHERE ' + @strFieldID + ' = ' + @strFieldID + ') ...'

as you are saying WHERE @strFieldID = @strFieldID which will always be true??

Instead of EXEC we use sp_ExcuteSQL (with a parameterised query, rather than just some Dynamic SQL as you are currently using with EXEC)

It avoids any syntax errors from parameter strings being included - although your current query doesn't have any.

6

Hello,
I use sp_executesql and it needs NVARCHAR. Does EXECUTE?

7

Yes, but I think EXEC does an implicit conversion ...

8

Hi Kristen, adding the () around the EXECUTE command worked. How simple of an issue.

As for the COUNT, here is what I am trying to do. I pass along the ID, name, and table name to the stored procedure, for instance:

EXEC sp_VehicleOptions @strFieldID = 'vm_id', @strFieldName = 'vm_name', @strTable = 'tbl_VehicleMake'

That is calling tbl_VehicleMake and is grabbing fields vm_id and vm_name to display in a dropdown for options for the customer. Here is an example:

vm_id, vm_name
1, Ford
2, Chevy
3, Audi
4, BMW

In my tbl_Vehicles table, I am storing the vm_id. I want to COUNT the number of vehicles with that vm_id in the dropdown. So, in the dropdown, the customer would see this:

Ford (569)
Chevy (1,504)
Audi (125)
BMW (96)

The numbers after the vehicle make is the number of vehicles with that vm_id in the database. I am trying to do this in ONE query as currently, I am printing the vehicle make, and then calling another query to find how many vehicles have that make. It is adding a LOT of time to my page load...

9

DOCs says:

"EXEC (@string_variable) Is the name of a local variable. @string_variable can be any char, varchar, nchar, or nvarchar data type. These include the (max) data types."

It also says that a String Constant can be Vachar or NVarchar (and if prefixed with N'xxx' it will be treated as an Nvarchar())

So looks like SQL is not fussy!!

Its an annoying, and error prone, bit of syntax ...

EXEC @strNameOfAnSproc
but
EXEC (@strSomeDynamicSQL)

Yup, I can see that would be informative to the user

Its just that your original code was:

(SELECT COUNT(' + @strFieldID + ') FROM tbl_Vehicles 
WHERE ' + @strFieldName + ' = ' + @strFieldID + ') ...

but your new code is

(SELECT COUNT(' + @strFieldID + ') FROM tbl_Vehicles 
WHERE ' + @strFieldID + ' = ' + @strFieldID + ') ...'

The first one looks more reasonable to me - WHERE @strFieldName = @strFieldID rather than WHERE @strFieldID = @strFieldID

How about adding an index? It needs to be on tbl_Vehicles on whatever column is being tested. If the field being tested can be one of several things (I assume so, as you are concatenating @strFieldName / @strFieldID into the SQL so presumably it changes?? (otherwise no need to have all this dynamic SQL and your query may well run faster without it ...) then you will need separate indexes one for each column in tbl_Vehicles that is a possible target of your COUNT statement.

The other option is to keep a count of them somewhere else (i.e. Cached) - and then just join to that Table-of-Counts which will be "instant". Of course you have to keep that count up to date - you can do that with a TRIGGER on the table, so every time one is added/changes etc. the stored count is adjusted. Or you could just go with "approximate" counts and refresh the Table-of-Counts once a day ... or once an hour. The Table-of-Counts becomes much more critical when a value changes from 1-to-0, or a new model is added, as the scheduled-refresh system won't include that, critical, change until the next time it runs. But if your counts are all 100's and 1,000s perhaps the exact number doesn't matter? - and maybe the Table-of-Counts should contain values rounded to the nearest order-of-magnitude - so

Ford (569)
Chevy (1,572)
Audi (125)
BMW (96)

would become, say

Ford (560+)
Chevy (1,500+)
Audi (120+)
BMW (90+)
10

Kristen, my original code, whether I use strFieldName or strFieldID does not work. As I iterate through each vm_id, I would essentially need to generate this query:

SELECT COUNT(vm_id) FROM tbl_Vehicles WHERE vm_id = 1
SELECT COUNT(vm_id) FROM tbl_Vehicles WHERE vm_id = 2
SELECT COUNT(vm_id) FROM tbl_Vehicles WHERE vm_id = 3
SELECT COUNT(vm_id) FROM tbl_Vehicles WHERE vm_id = 4
etc...

But my code, as it sits today, is returning the following:

SELECT COUNT(vm_id) FROM tbl_Vehicles WHERE vm_name = vm_id

Since this SQL query is part of my other query, it is not retuning properly...

I like your other suggestion having a table-of-counts, but I think that is redundant as the SQL should be able to do what I am looking for utilizing the current table?

11

FYI. When I run this query:

(SELECT COUNT(' + @strFieldID + ') FROM tbl_Vehicles 
    WHERE ' + @strFieldName + ' = ' + @strFieldID + ') ...

I get a result set back, however, it is the same TOTAL for all categories... so it is summing, but finding ALL vehicles with ANY ID entered, rather than showing the COUNT for just that ONE ID.

EDIT: I added the table reference, tbl_Vehicles v AND @strTable st to my query and it is working fine:

SET @strSQL = @strSQL + '(SELECT COUNT(v_id) FROM tbl_Vehicles v WHERE v.' + @strVehicleFieldName + ' = st.' + @strFieldID + ') As VehicleCount '
SET @strSQL = @strSQL + 'FROM ' + @strTable + ' st ORDER BY '
12

One last question. At times, I pass multiple values to my stored procedure.

For instance, I may pass a vm_id = '1, 5, 9, 15, 20' to my stored procedure. I need to add this, in my dynamic SQL, to the WHERE clause. Basically, I need to find ANY vehicle with any of those vm_id:

SELECT ... FROM ... WHERE vm_id = 1 OR vm_id = 5 OR vm_id = 9 OR vm_id = 15 OR vm_id = 20

This also needs to identify when the vm_id is just ONE value and does NOT need to be split.

SELECT ... FROM ... WHERE vm_id = 1

Any ideas?

13

it would be more efficient to do

SELECT vm_id, vm_name, VehicleCount
FROM tbl_VehicleMake AS M
JOIN
(
    SELECT COUNT(vm_id) As VehicleCount 
    FROM tbl_Vehicles 
-- Might benefit from a constraining WHERE clause here
    GROUP BY vm_id
) AS C
    ON C.vm_id = M.vm_name
ORDER BY vm_name

When I do this sort of thing I sort out the exact syntax of the query first, as a "test", and then convert that to the Dynamic SQL

Indeed, but the problem is that you will be asking SQL to do it every single time that someone needs that menu ... even if it is only a few MS it mounts up if the page that the menu is on is "popular". We find that such presentation items usually need some "supporting code" to help them along (performance-wise)

I think that was because, as I mentioned earlier, the query you were actually using for the total has the wrong syntax - along the lines of

SELECT COUNT(v_id)
FROM tbl_Vehicles
WHERE 1=1

which will indeed count every row in the table :smile:

Question: Why are you using dynamic SQL?

Do the values in @strVehicleFieldName, @strFieldID and @strTable ever change?

If so please give me an example of two different sets of values for those 3 variables so I can see what sort of scenarios you need to program.

14

Use a Splitter "User Defined Function" (UDF) to split the vm_id list into a @TempTable and then JOIN the @TempTable to your query