June 18, 2008 at 1:19 pm
I takes like 4 hrs for me when i run this proc in a daily job, which moves some 1000's of rows.
I would like to tune it better performance wise so tht it takes less time. need any input from you guys.
Please check the attachement fopr Procedure.
thanks
June 18, 2008 at 1:48 pm
I'm sure it didn't start out that way on your screen, but the attachment is pretty much unreadable when I open it.
- Gus "GSquared", RSVP, OODA, MAP, NMVP, FAQ, SAT, SQL, DNA, RNA, UOI, IOU, AM, PM, AD, BC, BCE, USA, UN, CF, ROFL, LOL, ETC
Property of The Thread
"Nobody knows the age of the human race, but everyone agrees it's old enough to know better." - Anon
June 18, 2008 at 1:56 pm
If you cant read attachement, look at this
June 18, 2008 at 2:11 pm
Let me see if I understand this proc correctly.
It looks like what it does is check a set of tasks and projects, find if they are in certain tables, update them if they are, and add them if they are not.
Is that accurate?
If I'm reading it correctly, it looks like the whole thing could be turned into a couple of insert statements and update statements that would run MUCH faster than the cursor version.
- Gus "GSquared", RSVP, OODA, MAP, NMVP, FAQ, SAT, SQL, DNA, RNA, UOI, IOU, AM, PM, AD, BC, BCE, USA, UN, CF, ROFL, LOL, ETC
Property of The Thread
"Nobody knows the age of the human race, but everyone agrees it's old enough to know better." - Anon
June 18, 2008 at 2:20 pm
Exactly.
Can you pls update my proc with your best performance ideas.
June 18, 2008 at 8:49 pm
GSquared (6/18/2008)
If I'm reading it correctly, it looks like the whole thing could be turned into a couple of insert statements and update statements that would run MUCH faster than the cursor version.
I bet it would be a lot easier to read also.
[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]
June 19, 2008 at 6:35 am
Mike Levan (6/18/2008)
Exactly.Can you pls update my proc with your best performance ideas.
Actually, quite a few of us get paid to rewrite 500 line stored procedures. We'll help, but you need to do most of the heavy lifting.
You create a temporary tables, #tmpSubPrograms, #tmpIXPrograms_SubPrograms, #tmpCustomerPrograms, and then you don't use them. That will save time right there. You load the temp table, #tmpCustomerSites, and then run selects & stuff against it. Why not simply run the same selects against the core table? You're moving a lot of data around for questionable reasons.
This bit of code:
IF EXISTS(SELECT * FROM LS_PORTAL.SBMGroupPortal.DBO.SubPrograms WHERE (SolomonTaskCode = @TaskID))
BEGIN
--Exists, update the name
UPDATE LS_PORTAL.SBMGroupPortal.DBO.SubPrograms SET SubProgramName = @TaskDescription, Flag_Active = 1 WHERE (SolomonTaskCode = @TaskID)
You're reading a table and then updating a table based on the criteria for the read. Simply do the update. If the values are there, they'll be updated, if they're not, they won't, reading it once to determine if you're going to read it twice doesn't save you any processing time. You repeat that pattern several times.
I'll leave the cursor to Jeff if he swings by.
"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 19, 2008 at 6:50 am
<eyes glaze over> Right....
In addition to Grant's comments...
You have a lot of branching logic within the proc (if.. else...).
That kind of thing tends to lead to sub-optimal execution plans, as the entire proc gets compiled the first time
and the plans are based on the conditions at that time, even for branches of code that will not be called.
I would suggest you break that proc up into smaller ones. First, it makes things easier to read.
Second it lets each proc have its own execution plan. Smaller procs are also more likely to have a good plan than larger ones.
For larger ones, the optimiser's quite likely to run out of time and pick a 'good enough' plan
How many rows does the cursor gnerally return?
I'll also leave the cursor to Jeff's tender mercies, I think he has more time and patience for them than I do.
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
June 19, 2008 at 7:13 am
Here's a rough draft of what should be a more efficient proc:
set ANSI_NULLS ON
set QUOTED_IDENTIFIER ON
go
ALTER PROCEDURE [dbo].[xSynPortalToSolomon_Prog]
AS
--~~~~~~~~~~~~~~~~~~~~~~~~~
-- dcheever adding error handling 11/07
--~~~~~~~~~~~~~~~~~~~~~~~~~
Declare @errorcode int,
@rowc int,
@StartTime SmallDateTime,
@stepMarker nvarchar(300),
@TaskID nvarchar(50),
@TaskProjectID nvarchar(50),
@TaskDescription nvarchar(150),
@ProgramId int,
@iPos int,
@Prefix varchar(50),
@SiteID INT,
@SubProgramID INT,
@ProjectActive bit,
@CustomerProgramID INT,
@errorMessage nvarchar(1024)
SET @errorcode = @@ERROR
SET @StartTime = getdate()
--~~~~~~~~~~~~~~~~~~~~~~~~~
-- Customer Programs
--~~~~~~~~~~~~~~~~~~~~~~~~~
begin try
select @stepmarker = 'Temp table creation and population'
insert into #programs (taskid, projectid, descript)
SELECT
RTRIM(dbo.PJPENT.pjt_entity),
RTRIM(dbo.PJPENT.project),
RTRIM(dbo.PJPENT.pjt_entity_desc)
into #Programs
FROM
dbo.PJPENT WITH (NOLOCK)
INNER JOIN dbo.PJPROJ WITH (NOLOCK) ON dbo.PJPENT.project = dbo.PJPROJ.project
WHERE
(dbo.PJPENT.status_pa = 'A') AND
(dbo.PJPROJ.status_pa = 'A')
alter table #Programs
add Prefix nvarchar(50), ProgramID int
--~~~~~~~~~~~~~~~~~~~~~~~~~
-- Get a copy of the Program Mapping
--~~~~~~~~~~~~~~~~~~~~~~~~~
SELECT
*
INTO
#tmpProgMapping
FROM
LS_PORTAL.SBMGroupPortal.DBO.SolomonTask_ProgramMapping
--~~~~~~~~~~~~~~~~~~~~~~~~~
-- Get a copy of the sub program table
--~~~~~~~~~~~~~~~~~~~~~~~~~
SELECT
*
INTO
#tmpSubPrograms
FROM
LS_PORTAL.SBMGroupPortal.DBO.SubPrograms
--~~~~~~~~~~~~~~~~~~~~~~~~~
-- Get a copy of the IX table
--~~~~~~~~~~~~~~~~~~~~~~~~~
SELECT
*
INTO
#tmpIXPrograms_SubPrograms
FROM
LS_PORTAL.SBMGroupPortal.DBO.IX_CustomerPrograms_SubPrograms
SELECT
*
INTO
#tmpCustomerSites
FROM
LS_PORTAL.SBMGroupPortal.DBO.CustomerSites
SELECT
*
INTO
#tmpCustomerPrograms
FROM
LS_PORTAL.SBMGroupPortal.DBO.CustomerPrograms
select @stepmarker = 'Global updates'
UPDATE LS_PORTAL.SBMGroupPortal.DBO.SubPrograms SET Flag_Active = 0
UPDATE LS_PORTAL.SBMGroupPortal.DBO.IX_CustomerPrograms_SubPrograms
SET Flag_Delete = 1
update #programs
set prefix =
case
when charindex(',', Descript) > 1 then left(descript, charindex(',',descript)-1)
else descript
end
update #programs
set programid = programid
from #tmpProgMapping
where LOWER(TaskPrefix) = LOWER(Prefix)
update #programs
set programid = 0
where programid is null
select @stepmarker = 'LS_PORTAL.SBMGroupPortal.DBO.SubPrograms'
update LS_PORTAL.SBMGroupPortal.DBO.SubPrograms
set SubProgramName = Descript, Flag_Active = 1
from #programs
where SolomonTaskCode = TaskID
update LS_PORTAL.SBMGroupPortal.DBO.SubPrograms
set SolomonTaskCode = RTRIM(TaskID), Flag_Active = 1
from #programs
where len(taskid) = 5
and RTRIM(SubProgramName) = Descript
insert LS_PORTAL.SBMGroupPortal.DBO.SubPrograms(
SubProgramName,
ProgramId,
SolomonTaskCode,
Flag_Active)
select
Descript,
ProgramId,
TaskID,
1
from #programs
left outer join #tmpsubprograms
on #programs.taskid = #tmpsubprograms.solomontaskcode
and #programs.descript = #tmpsubprograms.subprogramname
where #programs.programid > 0
and #tmpsubprograms.subprogramname is null
select @stepmarker = 'LS_PORTAL.SBMGroupPortal.DBO.CustomerPrograms'
update LS_PORTAL.SBMGroupPortal.DBO.CustomerPrograms
set EndDate = NULL
from #tmpcustomersites
inner join LS_PORTAL.SBMGroupPortal.DBO.SubPrograms
on #tmpcustomersites.solomonprojectcode =
subprograms.solomonprojectcode
inner join #programs
on #tmpcustomersites.solomonprojectcode = #programs.taskid
inner join LS_PORTAL.SBMGroupPortal.DBO.CustomerPrograms cp
on #tmpcustomersites.siteid = cp.siteid
and subprograms.subprogramid = cp.programid
select @stepmarker = 'LS_PORTAL.SBMGroupPortal.DBO.CustomerPrograms'
insert LS_PORTAL.SBMGroupPortal.DBO.CustomerPrograms(
SiteId,
ProgramId
select SiteID, SubProgramID
from #tmpcustomersites
inner join LS_PORTAL.SBMGroupPortal.DBO.SubPrograms
on #tmpcustomersites.solomonprojectcode =
subprograms.solomonprojectcode
inner join #programs
on #tmpcustomersites.solomonprojectcode = #programs.taskid
left outer join LS_PORTAL.SBMGroupPortal.DBO.CustomerPrograms cp
on #tmpcustomersites.siteid = cp.siteid
and subprograms.subprogramid = cp.programid
where cp.siteid is null
and cp.programid is null
select @stepmarker = 'LS_PORTAL.SBMGroupPortal.DBO.IX_CustomerPrograms_SubPrograms'
UPDATE LS_PORTAL.SBMGroupPortal.DBO.IX_CustomerPrograms_SubPrograms
SET Flag_Delete = 0
from #tmpcustomersites
inner join LS_PORTAL.SBMGroupPortal.DBO.SubPrograms
on #tmpcustomersites.solomonprojectcode =
subprograms.solomonprojectcode
inner join #programs
on #tmpcustomersites.solomonprojectcode = #programs.taskid
inner join LS_PORTAL.SBMGroupPortal.DBO.CustomerPrograms cp
on #tmpcustomersites.siteid = cp.siteid
and subprograms.subprogramid = cp.programid
where IX_CustomerPrograms_SubPrograms.SubProgramId = cp.SubProgramID
AND IX_CustomerPrograms_SubPrograms.ItemId = cp.itemid
and flag_delete != 0
INSERT LS_PORTAL.SBMGroupPortal.DBO.IX_CustomerPrograms_SubPrograms(
ItemId,
SubProgramId,
Flag_Delete
)
select itemid, subprogramid, 0
from #tmpcustomersites
inner join LS_PORTAL.SBMGroupPortal.DBO.SubPrograms
on #tmpcustomersites.solomonprojectcode =
subprograms.solomonprojectcode
inner join #programs
on #tmpcustomersites.solomonprojectcode = #programs.taskid
inner join LS_PORTAL.SBMGroupPortal.DBO.CustomerPrograms cp
on #tmpcustomersites.siteid = cp.siteid
and subprograms.subprogramid = cp.programid
left outer join LS_PORTAL.SBMGroupPortal.DBO.IX_CustomerPrograms_SubPrograms ix
on cp.itemid = ix.itemid
and cp.subprogramid = ix.subprogramid
where ix.itemid is null
and ix.subprogramid is null
select @stepmarker = 'Mark for delete'
/*
I'm not certain of the logic requirement on this part. Please clarify under which
circumstances Flag_Delete would be marked as 1
*/
UPDATE LS_PORTAL.SBMGroupPortal.DBO.IX_CustomerPrograms_SubPrograms
SET Flag_Delete = 1
WHERE (ItemId = @CustomerProgramID)
and (@CustomerProgramID > 0)
select @stepmarker = 'LS_PORTAL.SBMGroupPortal.DBO.Solomon_NewPrograms'
INSERT LS_PORTAL.SBMGroupPortal.DBO.Solomon_NewPrograms(
ProgramName,
SolomonTaskCode
)
select descript, taskid
from #programs
left outer join LS_PORTAL.SBMGroupPortal.DBO.Solomon_NewPrograms
on descript = programname
and taskid = solomontaskcode
where Solomon_NewPrograms.programname is null
and Solomon_NewPrograms.solomontaskcode is null
--Clean up the deleted sub program IX
DELETE FROM LS_PORTAL.SBMGroupPortal.DBO.IX_CustomerPrograms_SubPrograms
WHERE (Flag_Delete = 1)
select @stepmarker = 'Clean-up'
drop table #programs
DROP TABLE #tmpProgMapping
DROP TABLE #tmpSubPrograms
DROP TABLE #tmpIXPrograms_SubPrograms
DROP TABLE #tmpCustomerSites
DROP TABLE #tmpCustomerPrograms
end try
begin catch
SET @errorMessage = 'ERROR: Stored Procedure xSynPortalToSolomon_Prog failed at '
+ @stepMarker + '.'
RAISERROR ('%s. Error Number = %d', 11, 1, @errorMessage, @errorcode) WITH SETERROR
end catch
set nocount off
You'll need to validate that I set up the logic correctly. As Gail mentioned, the original is kind of eye-glazingly convoluted.
There was one point in it, that I commented, where I simply lost track of what logic was required. You'll definitely need to rewrite that part using the kind of logic I used in the other parts.
As mentioned already, breaking this up into smaller procs, one for each table you're upserting (updating/inserting), would be even better. Then just have the master proc call each sub-proc once.
This should get you started on a good path towards a much more efficient piece of code. When I've made this kind of change before, I've seen run-times go from hours to minutes, or even seconds, in many cases.
- Gus "GSquared", RSVP, OODA, MAP, NMVP, FAQ, SAT, SQL, DNA, RNA, UOI, IOU, AM, PM, AD, BC, BCE, USA, UN, CF, ROFL, LOL, ETC
Property of The Thread
"Nobody knows the age of the human race, but everyone agrees it's old enough to know better." - Anon
June 19, 2008 at 2:15 pm
Thank you very much, you did most my work.
when i exec the proc i get this error.
OLE DB provider "SQLNCLI" for linked server "LS_PORTAL" returned message "The partner transaction manager has disabled its support for remote/network transactions.".
(0 row(s) affected)
Msg 50000, Level 11, State 1, Procedure xSynPortalToSolomon_ProgTEST, Line 256
ERROR: Stored Procedure xSynPortalToSolomon_Prog failed at Global updates.. Error Number = 0
Msg 3998, Level 16, State 1, Line 1
Uncommittable transaction is detected at the end of the batch. The transaction is rolled back.
I am about to test performance but trying to do that with rollback trans so tht i doesnt change anything in my database as it does some inserts.
June 20, 2008 at 12:24 am
The remote server or the driver for the linked server doesn't allow remote transactions.
It's nothing to do with the proc rewrite. Check the linked server settings or check with the admin of that server.
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
June 20, 2008 at 5:31 am
Even though GSquared eliminated the cursor, there are still other optimizations pointed out by others that can be done to the proc.
"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 20, 2008 at 7:55 am
Grant Fritchey (6/19/2008)
Actually, quite a few of us get paid to rewrite 500 line stored procedures. We'll help, but you need to do most of the heavy lifting...You create a temporary tables, #tmpSubPrograms, #tmpIXPrograms_SubPrograms, #tmpCustomerPrograms, and then you don't use them. That will save time right there. You load the temp table, #tmpCustomerSites, and then run selects & stuff against it. Why not simply run the same selects against the core table? You're moving a lot of data around for questionable reasons.
This bit of code:
IF EXISTS(SELECT * FROM LS_PORTAL.SBMGroupPortal.DBO.SubPrograms WHERE (SolomonTaskCode = @TaskID))BEGIN
--Exists, update the name
UPDATE LS_PORTAL.SBMGroupPortal.DBO.SubPrograms SET SubProgramName = @TaskDescription, Flag_Active = 1 WHERE (SolomonTaskCode = @TaskID)
You're reading a table and then updating a table based on the criteria for the read. Simply do the update. If the values are there, they'll be updated, if they're not, they won't, reading it once to determine if you're going to read it twice doesn't save you any processing time. You repeat that pattern several times.
I'll leave the cursor to Jeff if he swings by.
Between that and what Gail and Gus have added, anything I could add would be superfluous, at this point.
Hmmm... Grant, Gail, and Gus... "G-Cubed" sounds like a hell of a company name...:)
--Jeff Moden
Change is inevitable... Change for the better is not.
June 20, 2008 at 8:16 am
Isn't performing a remote (linked server) update going to serialize the update into cursor-like updates?
It seems to me you'd want to encapsulate and send the update statement (via OPENQUERY) to the linked server for it to do the update. As far as I know that's about the only hope you have of making this "act" set-based (even though it's written set-based).
----------------------------------------------------------------------------------
Your lack of planning does not constitute an emergency on my part...unless you're my manager...or a director and above...or a really loud-spoken end-user..All right - what was my emergency again?
June 20, 2008 at 8:17 am
Matt Miller (6/20/2008)
Isn't performing a remote (linked server) update going to serialize the update into cursor-like updates?It seems to me you'd want to encapsulate and send the update statement (via OPENQUERY) to the linked server for it to do the update. As far as I know that's about the only hope you have of making this "act" set-based (even though it's written set-based).
Of course - Gail's point about fixing the remote transaction issue would still stand.
----------------------------------------------------------------------------------
Your lack of planning does not constitute an emergency on my part...unless you're my manager...or a director and above...or a really loud-spoken end-user..All right - what was my emergency again?
Viewing 15 posts - 1 through 15 (of 18 total)
You must be logged in to reply to this topic. Login to reply