optimising the procedure
-
i have a huge procedure in which several blocks of inserts and updates are repeated many times according to IF statements.
What's the best solution in order not to copy these blocks many times? Do i have to create small stored procedures of UpdateTable and CreateTable and call them by sending the values as parameters? or is GOTO a solution? i don't like using it that much, i feel i lose control of the procedure!
your experience in that is most welcome!
-
In general my feeling is that a procedure should be a set of SQL statements performing one common, repeatable/repeated task. If you have a large number of conditional statements you might be better off writing a single wrapper procedure with your conditional logic and then calling several smaller procedures based on that. In my experience I've seen better performance this way and it makes code maintenance easier if you have a standard input/output for each of these smaller procedures. If this is part of a larger application, there's an argument that can be made for moving much of this conditional business logic into the front-end.
Also I tend to recommend people avoid using GOTO at all costs. I really only use it in error handling.
-
Post some of your code (and DDL) and maybe someone can provide an example of a more efficient solution for you. My gut feeling is that you could probably pass everything in as a couple of VARCHAR parameters, split them up into a temp table and perform a couple of set-based INSERT/UPDATEs, but without seeing your code and DDL I can't tell you that for certain.
-
this is my code... u don't have to understand much of it but you will see that there is the same update or insert instructions many times
GO
IF OBJECT_ID (N'sp_ImportEquipeTV', N'P') IS NOT NULL
DROP PROCEDURE sp_ImportEquipeTV;
GO
CREATE PROCEDURE sp_ImportEquipeTV
AS
declare @oneline varchar(8000)
declare @Priorite varchar(2)
declare @Origine varchar(80)
declare @Nom Varchar(50)
declare @Prenom Varchar(50)
declare @Civilite Varchar(20)
declare @Datedenaiss Varchar(20)
declare @Email Varchar(80)
declare @NumClientTV Varchar(50)
declare @DateAbo Varchar(20)
declare @Montant float
declare @Devise Varchar(10)
declare @RecipientId int
declare @FolderId int
declare @LastAction int
declare @RecEmail varchar(80)
declare @RecOrigin varchar(80)
declare @RecAdresseQuality int
declare @RecBirthDateQuality int
declare @RecEmailQuality int
declare @RecFirstName varchar(50)
declare @RecLastName varchar(50)
declare @RecSalutation varchar(20)
declare @RecBirth datetime
if exists(select [id] from tempdb.dbo.sysobjects where id = object_id(N'tempdb..#textfile'))
drop table #textfile
CREATE TABLE #textfile (line varchar(8000))
BULK INSERT #textfile FROM 'D:\websites\neolane\www\Import\init_tv.txt'
DECLARE table_cursor CURSOR FOR
SELECT line
FROM #textfile
OPEN table_cursor
FETCH NEXT FROM table_cursor INTO @oneline
IF (@@FETCH_STATUS = -1)
BEGIN
PRINT 'No records Found'
CLOSE table_cursor
DEALLOCATE table_cursor
RETURN
END
WHILE (@@FETCH_STATUS = 0 AND @oneline != '')
BEGIN
BEGIN TRY
BEGIN TRANSACTION
print '------------------------------------------------------------'
PRINT @oneline
set @Priorite = dbo.f_GetEntryDelimitted (@oneline , 1, '|', 'N')
set @Origine = dbo.f_GetEntryDelimitted (@oneline , 2, '|', 'N')
set @Nom = dbo.f_GetEntryDelimitted (@oneline , 3, '|', 'N')
set @Prenom = dbo.f_GetEntryDelimitted (@oneline , 4, '|', 'N')
set @Civilite = dbo.f_GetEntryDelimitted (@oneline , 5, '|', 'N')
set @Datedenaiss = dbo.f_GetEntryDelimitted (@oneline , 6, '|', 'N')
set @Email = dbo.f_GetEntryDelimitted (@oneline , 7, '|', 'N')
set @NumClientTV = dbo.f_GetEntryDelimitted (@oneline , 8, '|', 'N')
set @DateAbo = dbo.f_GetEntryDelimitted (@oneline , 9, '|', 'N')
set @Montant = dbo.f_GetEntryDelimitted (@oneline , 10, '|', 'N')
set @Devise = dbo.f_GetEntryDelimitted (@oneline , 11, '|', 'N')
SET @RecipientId = (Select iRecipientId from neolane.CusEquipeTV
where sEquipetvId = @NumClientTV)
IF @RecipientId is NULL
BEGIN
if ltrim(rtrim(@Email)) = ''
begin
print 'email blank'
FETCH NEXT FROM table_cursor INTO @oneline
end
SET @RecipientId = (Select iRecipientId from neolane.NmsRecipient
where sEmail = @Email)
IF @RecipientId is NULL
BEGIN
print 'create new NmsRecipient'
exec neolane.up_GetNewId @RecipientId OUTPUT
set @FolderId = (select ifolderId from neolane.XtkFolder where sName = 'nmsRootPartition')
INSERT INTO Neolane.NmsRecipient([iPartitionId]
,[iRecipientId]
,[sEmail]
,[sOrigin]
,[tsCreated]
,[tsLastModified]
,[iAdresseQuality]
,[iBirthDateQuality]
,[iEmailQuality]
,[iPhoneQuality]
,[tsLastUpdateBirth]
,[tsLastUpdatePhone]
,[tsLastUpdateadresse]
,[tsLastUpdatemail])
VALUES
(@FolderId
,@RecipientId
,@Email
,@Origine
,getdate()
,getdate()
,@Priorite
,@Priorite
,@Priorite
,@Priorite
,getdate()
,getdate()
,getdate()
,getdate())
IF @@ERROR <> 0
BEGIN
PRINT 'Error in insertion of NmsRecipient. Error is ' + LTRIM(STR(@@ERROR))
END
print 'create new EquipeTV'
INSERT INTO [neolane].[CusEquipeTv]
([dMontantAbonnement]
,[iRecipientId]
,[sDeviseAbonnement]
,[sEmailEquipetv]
,[tsSubscription]
,[sEquipetvId])
VALUES
(@Montant
,@RecipientId
,@Devise
,@Email
,@DateAbo
,@NumClientTV)
IF @@ERROR <> 0
BEGIN
PRINT 'Error in insertion of CusEquipeTV. Error is ' + LTRIM(STR(@@ERROR))
END
END
ELSE
BEGIN
goto update_NmsRecipient
print 'create new EquipeTV'
INSERT INTO [neolane].[CusEquipeTv]
([dMontantAbonnement]
,[iRecipientId]
,[sDeviseAbonnement]
,[sEmailEquipetv]
,[tsSubscription]
,[sEquipetvId])
VALUES
(@Montant
,@RecipientId
,@Devise
,@Email
,@DateAbo
,@NumClientTV)
IF @@ERROR <> 0
BEGIN
PRINT 'Error in insertion of CusEquipeTV. Error is ' + LTRIM(STR(@@ERROR))
END
END
END
ELSE
BEGIN
print 'update CusEquipeTV'
UPDATE [neolane].[CusEquipeTv]
SET [dMontantAbonnement] = @montant
,[sDeviseAbonnement] = @Devise
,[sEmailEquipetv] = @Email
,[tsSubscription] = @Dateabo
WHERE [iRecipientId] = @RecipientId
goto update_NmsRecipient
END
update_NmsRecipient:
print 'update NmsRecipient - 1'
select @RecEmail = Neolane.NmsRecipient.[sEmail],
@RecEmailQuality
= Neolane.NmsRecipient.[iEmailQuality],
@RecFirstName
= Neolane.NmsRecipient.[sFirstName],
@RecLastName
= Neolane.NmsRecipient.[sLastName],
@RecSalutation
= Neolane.NmsRecipient.[sSalutation],
@RecAdresseQuality
= Neolane.NmsRecipient.[iAdresseQuality],
@RecBirth
= Neolane.NmsRecipient.[tsBirth],
@RecBirthDateQuality
= Neolane.NmsRecipient.[iBirthDateQuality]
FROM Neolane.NmsRecipient
WHERE Neolane.NmsRecipient.[iRecipientId] = @RecipientId
if (ltrim(rtrim(@Email)) != ''
and ( (@Email != @RecEmail and @Priorite <= @RecEmailQuality )
or (ltrim(rtrim(@RecEmail)) = '' or @RecEmail is null)
)
)
begin
print 'meilleure priorite Email'
update Neolane.NmsRecipient
set [sEmail] = @Email
,[tsLastModified] = getdate()
,[iEmailQuality] = @RecEmailQuality
,[tsLastUpdatemail] = getdate()
where Neolane.NmsRecipient.iRecipientId = @RecipientId
IF @@ERROR <> 0
BEGIN
PRINT 'Error in update of NmsRecipient. Error is ' + LTRIM(STR(@@ERROR))
END
end
if (ltrim(rtrim(@Datedenaiss)) != ''
and ( (@Datedenaiss != @RecBirth and @Priorite <= @RecBirthDateQuality )
or (ltrim(rtrim(@RecBirth)) = '' or @RecBirth is null)
)
)
begin
print 'meilleure priorite Date de naissance'
update Neolane.NmsRecipient
set [tsBirth] = @Datedenaiss
,[tsLastModified] = getdate()
,[iBirthDateQuality] = @RecBirthDateQuality
,[tsLastUpdateBirth] = getdate()
where Neolane.NmsRecipient.iRecipientId = @RecipientId
IF @@ERROR <> 0
BEGIN
PRINT 'Error in update of NmsRecipient. Error is ' + LTRIM(STR(@@ERROR))
END
end
if (ltrim(rtrim(@Civilite)) != ''
and ( (@Civilite != @RecSalutation and @Priorite <= @RecAdresseQuality )
or (ltrim(rtrim(@RecSalutation)) = '' or @RecSalutation is null)
)
)
begin
print 'meilleure priorite Civilite'
update Neolane.NmsRecipient
set [sSalutation] = @Civilite
,[tsLastModified] = getdate()
,[iAdresseQuality] = @RecAdresseQuality
,[tsLastUpdateAdresse] = getdate()
where Neolane.NmsRecipient.iRecipientId = @RecipientId
IF @@ERROR <> 0
BEGIN
PRINT 'Error in update of NmsRecipient. Error is ' + LTRIM(STR(@@ERROR))
END
end
if (ltrim(rtrim(@Prenom)) != ''
and ( (@Prenom != @RecFirstName and @Priorite <= @RecAdresseQuality )
or (ltrim(rtrim(@RecFirstName)) = '' or @RecFirstName is null)
)
)
begin
print 'meilleure priorite Prenom'
update Neolane.NmsRecipient
set [sFirstName] = @Prenom
,[tsLastModified] = getdate()
,[iAdresseQuality] = @RecAdresseQuality
,[tsLastUpdateAdresse] = getdate()
where Neolane.NmsRecipient.iRecipientId = @RecipientId
IF @@ERROR <> 0
BEGIN
PRINT 'Error in update of NmsRecipient. Error is ' + LTRIM(STR(@@ERROR))
END
end
if (ltrim(rtrim(@Nom)) != ''
and ( (@Nom != @RecLastName and @Priorite <= @RecAdresseQuality )
or (ltrim(rtrim(@RecLastName)) = '' or @RecLastName is null)
)
)
begin
print 'meilleure priorite Nom'
update Neolane.NmsRecipient
set [sFirstName] = @Nom
,[tsLastModified] = getdate()
,[iAdresseQuality] = @RecAdresseQuality
,[tsLastUpdateAdresse] = getdate()
where Neolane.NmsRecipient.iRecipientId = @RecipientId
IF @@ERROR <> 0
BEGIN
PRINT 'Error in update of NmsRecipient. Error is ' + LTRIM(STR(@@ERROR))
END
end
COMMIT TRANSACTION
END TRY
BEGIN CATCH
print 'error found'
IF @@TRANCOUNT > 0
BEGIN
PRINT '******** ' + @oneline + '************'
ROLLBACK TRANSACTION
END
END CATCH
FETCH NEXT FROM table_cursor INTO @oneline
END /* while fetch status = 0 */
DROP
TABLE #textfile
CLOSE
table_cursor
DEALLOCATE
table_cursor
PRINT
'DONE IMPORTING'
GO
-
I'd make each of these a separate procedure just for readability. But I'm not sure it's more than personal preference.
-
OK, first you should get rid of that God-awful cursor. It will be a little easier, I think, if you define the structure of the temp table in advance and BULK INSERT using a field terminator. See: http://msdn2.microsoft.com/en-us/library/ms188365.aspx for details. Something like the following:
CREATE TABLE #TempData(Priorite varchar(2),
Origine varchar(80),
Nom Varchar(50),
Prenom Varchar(50),
Civilite Varchar(20),
Datedenaiss Varchar(20),
Email Varchar(80),
NumClientTV Varchar(50),
DateAbo Varchar(20),
Montant float,
Devise Varchar(10))
BULK INSERT #TempData
FROM 'D:\websites\neolane\www\Import\init_tv.txt'
WITH ( DATAFILETYPE = 'char',
FIELDTERMINATOR = '|',
KEEPNULLS)
Here's the sample text file I used to test it:
1|Paris|Jetson|George||2006-12-20|george.jetson@france.fr|11293831|2006-12-23|159.99|Tivo|
23|Bordeaux|Flintston|Fred|F|2006-11-30||23746292|2006-12-1|699.99|PS/3|
-
Yep, lose the cursor. Not because cursors are inherently evil, but because there's a more efficient, set-based way. Those function calls to parse each field in the row have gotta be killing performance. Add an error flag column to your temp table, then change your IFs to set the error flag for each row in an UPDATE statement.
Your line based method:
if ltrim(rtrim(@Email)) = ''
begin
print 'email blank'
end
New, set based method:
UPDATE #TempData
SET ErrorFlag = 'email blank'
WHERE ltrim(rtrim(Email)) = ''
AND ErrorFlag IS NULL
There is no "i" in team, but idiot has two. -
tx for ur advices!
but i don't really get it how can i drop the cursor while i need it to read the temptable and insert into my db?
whether i predefine the fields in the temp table of not, i still need to run through all the records, no?
-
The bulk insert does that for you!!
-
The WHERE clause of the UPDATE statement I provided tests in one statement every row of the temp table that has not been tested yet and sets the error flag. Rewrite your IF statements into UPDATEs as I showed you to perform the test on every row in the table instead of using the cursor to loop through it. If you need to capture multiple errors in each row, build your temp table with one column for each error condition and UPDATE the appropriate error column.
There is no "i" in team, but idiot has two. -
The BULK INSERT splits up the text file for you automatically and inserts each row into the temp table. That eliminates the need for the cursor and the function calls to split apart each line as it's read into memory.
Once you have the text stored in the table, you can do something like this:
INSERT INTO destination_table (col1, col2, col3, ...)
SELECT col1, col2, col3, ...
FROM #TempData
You can use the CASE expression and other built-in functions to combine your data manipulations into the INSERT statement instead of doing them one-by-one, procedurally. That eliminates the second justification for a cursor. I think you can knock the whole body of this procedure down into the following:
1. CREATE TABLE - to create your temp table
2. BULK INSERT - to get the data from the flat file into your temp table
3. A few INSERT statements (like above, depending on how many target tables), and possibly an UPDATE or two
4. Of course you want to DROP your temp table when you get done (not required, but a good practice... always clean up after yourself
)Getting rid of that cursor and all those function calls should help improve your performance by a huge factor. And cursors are inherently evil
lol -
Let's not start this war again pls !!!!!
-
LOL. I just call 'em as I see 'em
On another note, I cringe every time I look at the source code for system stored procedures on SQL Server. Whoever wrote them loved the heck out of their cursors!
-
I don't look at those too much... I just checked out the sp_msforeach? and that's about it!
Are they all that bad or just a few of them?
-
They've gotten better with new versions, but SP's like sp_helpindex and sp_helpdb make my eyes water and my teeth grind
Fortunately a lot of those old SP's are being replaced with catalog views in SS2005, so we can probably expect the SP's to go away before the cursors
Viewing 15 posts - 1 through 15 (of 20 total)
You must be logged in to reply to this topic. Login to reply