June 3, 2009 at 12:50 am
Be warned I think this might be a long one
OK, my primary background is with procedural programming, and I really think my issue here is that I am not thinking/looking at these kind of issues in the right way.
Basically I am looking at a more efficient way of building/coding the below query, mainly to help me when writing future sql. For the below query it's not really necessary for this to run more efficiently as it only ever needs to run on tables with ~20000 records max, and efficiency isn't an issue... but I don't want to be writing poor code 'just cause the hardware can handle it' (I'd rather learn to write more efficiently now rather than on the day that it's needed :))
A couple of points prior to code
- This is designed as a script to be run when certain inconsistancies within our client's databases are found.
- Each property within the database should only have one corresponding reference within a customform table (the amount of customform tables and the names of which are user defined). Anything more than this is bad
- The information on how many customform tables and what their names are is defined by the 'customforms' table
- The 'type' field on the properties table defines the customform entry that the property should have, and is the only one we want to keep.
- This script is designed to locate any instances of the FileID field appearing on more than one customform table
- Once these are found, it will build a temptable full of delete statements designed to remove these redundant records
- What I have works, I have people telling me it can be done more efficiently
If there's anything I've left out, or left unclear, let me know 🙂 Also if this isn't the right area for this, please move.
The MAIN problem I have with this is it uses nested loops in the form of cursors. I know that I can work without cursors, getting the same net result, which is not really what I'm asking. I'm more curious to know whether or not it is possible to enumerate this information with only one cursor/looping statement.
I personally don't believe it is, given that our goal is to get an unknown amount of information from unknown tables, and I don't see how we can both get the table information and then query these tables with the one cursor.
Any help is much appreciated 🙂
-- Declare variables
declare @cursor cursor
declare @sql nvarchar(500)
declare @id int
declare @fileid int
declare @tablename varchar(51)
declare @linktype varchar(51)
declare @type varchar(51)
/* Create temptables used to store and retrieve info
#temptable used to store all the info regarding the invalid customform data
#tempdelete used to store the build delete statements */
create table #temptable (id int, fileid int, tablename varchar(51), linktype varchar(51), type varchar(51))
create table #tempdelete (qtext nvarchar(1000))
-- Configure and open first cursor used to get customform table information
set @cursor = cursor for select tablename, linktype from customforms where filetype = 100
open @cursor
fetch next from @cursor into @tablename, @linktype
while @@fetch_status = 0
begin
/* Set dynamic secondary cursor using current tablename
This is used to get the id/fileid/property type using the current customform table */
set @sql = 'declare dyn_cursor cursor for select ' + @tablename + '.id, ' + @tablename +
'.fileid, properties.type from ' + @tablename + ' join properties on ' + @tablename +
'.fileid = properties.id'
exec(@sql)
open dyn_cursor
fetch next from dyn_cursor into @id, @fileid, @type
while @@fetch_status = 0
begin
/* If statement used to determine whether the customform data is invalid
If invalid, adds information to #temptable, and builds a delete query for #tempdelete */
if @linktype <> @type
begin
insert #temptable values(@id, @fileid, @tablename, @linktype, @type)
set @sql = 'delete from ' + @tablename + ' where id = ' + cast(@id as nvarchar)
insert #tempdelete values(@sql)
end
fetch next from dyn_cursor into @id, @fileid, @type
end
close dyn_cursor
deallocate dyn_cursor
fetch next from @cursor into @tablename, @linktype
end
close @cursor
deallocate @cursor
-- Shows resulting information and clears temp tables
select * from #tempdelete
select * from #temptable
drop table #temptable
drop table #tempdelete
June 3, 2009 at 6:47 am
If I understand the code and the explanation, I think you can change this to use a single pass through one cursor. Instead of a set of SELECT statements and a set of DELETE statements, you need to take the WHERE clause of the SELECT statements and simply make it part of the WHERE clause in the DELETE statement. That makes it, at least for any one table, into a set based operation. You'll still need to build it dynamically since you're building a list of tables from the cursor. But it ought to work.
"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
June 4, 2009 at 8:20 pm
[font="Verdana"]I haven't been able to test my code, since you didn't supply any table definitions or example data. However, here's the approach. Hopefully you can get an idea from this.
Note, I did use a loop! I think it would be possible to eliminate even the outer loop, but you'd just end up with a really huge dynamic SQL statement, so I don't really see the value in it.
declare
@sql nvarchar(500),
@tablename varchar(51),
@linktype varchar(51);
-- Create temptables used to store and retrieve info
-- #temptable used to store all the info regarding the invalid customform data
create table #temptable (id int, fileid int, tablename varchar(51), linktype varchar(51), type varchar(51));
--
-- loop through the tables an pull back the mismatched fields
--
set @tablename = '';
while (1=1) begin
--
-- fetch the next table name to use, along with the correct link type for that table
--
select top 1
@tablename = tablename,
@linktype = linktype
from customforms
where tablename > @tablename and
filetype = 100
order by
tablename;
--
-- if there are no more tables to process, then exit
--
if (@@rowcount = 0) break;
--
-- generate dynamic SQL to pull back the list of mis-matched types in the table
--
set @sql =
'select ' + @tablename + '.id, ' + @tablename + '.fileid, ' +
'@tablename as tablename, @linktype as linktype, properties.type ' +
'from ' + @tablename + ' join properties on ' + @tablename + '.fileid = properties.id ' +
'where properties.type @linktype';
--
-- save the list
--
insert into #temptable
exec sp_executesql
@stmt = @sql,
@params = N'@tablename varchar(51), @linktype varchar(51)',
@tablename = @tablename,
@linktype = @linktype;
end; -- while
--
-- generate the list of delete statements to clear the mis-matched link types
--
select 'delete from ' + tablename + ' where id = ' + cast(id as nvarchar(10)) + ';' from #temptable;
--
-- now show the raw data
--
select * from #temptable;
--
-- clean up
--
drop table #temptable;
Edited to clean up my code commentary.
[/font]
June 4, 2009 at 8:32 pm
I ended up semi-figuring this out myself last night, thanks for the replies. I still use a cursor and agree that coding this to use no loops at all is really acedemic and pretty counter-intuitive as far as efficiency goes.
Thanks for all the replies
declare @cursor cursor
declare @sql nvarchar(500)
declare @id int
declare @fileid int
declare @tablename varchar(51)
declare @linktype varchar(51)
declare @type varchar(51)
create table #deletequeries (qtext nvarchar(1000))
set @cursor = cursor for select tablename, linktype from customforms where filetype = 100
open @cursor
fetch next from @cursor into @tablename, @linktype
while @@fetch_status = 0
begin
set @sql = 'delete from ' + @tablename + ' where id in (select ' + @tablename + '.id from '
+ @tablename + ' join properties on ' + @tablename + '.fileid = properties.id and ' +
'properties.type ''' + @linktype + ''')'
insert #deletequeries values (@sql)
fetch next from @cursor into @tablename, @linktype
end
close @cursor
deallocate @cursor
select * from #deletequeries
drop table #deletequeries
June 4, 2009 at 8:36 pm
dlee (6/4/2009)
I ended up semi-figuring this out myself last night, thanks for the replies.
[font="Verdana"]Always a good thing! I don't know about you, but I usually learn more when I have to work through it myself.[/font]
dlee (6/4/2009)
I still use a cursor and agree that coding this to use no loops at all is really acedemic and pretty counter-intuitive as far as efficiency goes.
[font="Verdana"]I find the cursor syntax painful, and creating a loop without the cursor is easy enough. But maybe that's just me.[/font]
Viewing 5 posts - 1 through 4 (of 4 total)
You must be logged in to reply to this topic. Login to reply