December 17, 2007 at 10:14 am
Hi all,
Hoping for a bit of help please. I've been working in SQL for a few years now, but I'm entirely self-taught so I’m pretty certain that a lot of things that I do could be done better… the way I do them is just because that’s how I’ve learnt. I generally get results, but I’m getting to a point now where optimisation is becoming an increasing focus so I’m trying to unlearn bad habits – not easy!!
The problem I have here is that I have quite a few processes to move data between tables… and to do this I’ve always used cursors (please stop screaming!). I’ve recently become aware that While loops are much better than cursors, but I need a bit of help turning one of my standard cursors in to a while loop… once I’ve got the basics I should be able to use the principles in my other queries.
The example below is something I run on a regular basis, but it’s performance is horrible – it takes upwards of ½ an hour to process about 12k records… so I’m thinking there must be a better way of doing things. I’d appreciate it if I could get some guidance on both general tips that people can observe looking at the way I’m doing things as well as converting the provided cursor in to a while loop.
BEGIN
SET NOCOUNT ON;
DECLARE @PHONEID INT
DECLARE @PERSONID INT
DECLARE @PERSLEGACYACCOUNTID INT
DECLARE @PHONETYPE VARCHAR(40)
DECLARE @PHONENUMBER VARCHAR(20)
DECLARE MY_CURSOR CURSOR FOR
SELECT PHON_TYPE,
LEFT(PHON_NUMBER,20),
PHON_PERSLEGACYACCOUNTID
FROM ZZIMPPHONE
WHERE PHON_PERSLEGACYACCOUNTID IN (SELECT DISTINCT PERS_LEGACYACCOUNTID FROM PERSON)
OPEN MY_CURSOR
FETCH NEXT FROM MY_CURSOR INTO
@PHONETYPE,
@PHONENUMBER,
@PERSLEGACYACCOUNTID
WHILE @@FETCH_STATUS = 0
BEGIN
EXEC @PHONEID = eware_get_identity_id 'Phone'
SET @PERSONID = (SELECT TOP 1 PERS_PERSONID FROM PERSON WHERE PERS_LEGACYACCOUNTID = @PERSLEGACYACCOUNTID)
INSERT INTO Phone
(Phon_PhoneId,
Phon_PersonID,
Phon_Type,
Phon_Number,
Phon_CreatedBy,
Phon_CreatedDate,
Phon_UpdatedBy,
Phon_UpdatedDate,
Phon_TimeStamp)
VALUES
(@PHONEID,
@PERSONID,
@PHONETYPE,
@PHONENUMBER,
1,
GETDATE(),
1,
GETDATE(),
GETDATE()
)
FETCH NEXT FROM MY_CURSOR INTO
@PHONETYPE,
@PHONENUMBER,
@PERSLEGACYACCOUNTID
END
CLOSE MY_CURSOR
DEALLOCATE MY_CURSOR
END
Thanks in advance
Brett
December 17, 2007 at 10:36 am
What does eware_get_identity_id do?
Can this be turned into a function?
If it's not too long, can you post it's definition?
I ask because I'm thinking that this can be done in one INSERT statement. 😀
______________________________________________________________________
Personal Motto: Why push the envelope when you can just open it?
If you follow the direction given HERE[/url] you'll likely increase the number and quality of responses you get to your question.
Jason L. SelburgDecember 17, 2007 at 10:38 am
No yelling here....but let's see if we can do away with that cursor....It's got nasty overhead, and performance is usually why we get rid of them....
Could you post what eware_get_identity_id does? Assuming it doesn't do anything outlandish - we should be able to get that down to a few seconds.....
----------------------------------------------------------------------------------
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?
December 17, 2007 at 10:40 am
Matt Miller (12/17/2007)
No yelling here....but let's see if we can do away with that cursor....It's got nasty overhead, and performance is usually why we get rid of them....Could you post what eware_get_identity_id does? Assuming it doesn't do anything outlandish - we should be able to get that down to a few seconds.....
Copy cat ... LOL :w00t:
Get out of my head Matt! :hehe:
______________________________________________________________________
Personal Motto: Why push the envelope when you can just open it?
If you follow the direction given HERE[/url] you'll likely increase the number and quality of responses you get to your question.
Jason L. SelburgDecember 17, 2007 at 10:47 am
Jason Selburg (12/17/2007)
What does eware_get_identity_id do?Can this be turned into a function?
Basically it is a proprietary SP provided by a SQL based web application for the purpose of generating unique integer based row IDs. It does several things including calling other SP’s for record locking, remote client id range updates as well index updates… so I don’t think so, but here is the code nonetheless:
SET ANSI_NULLS ON
GO
SET QUOTED_IDENTIFIER OFF
GO
ALTER PROCEDURE [dbo].[eware_get_identity_id]
@table_name NVARCHAR(80)
AS
DECLARE @table_id INT
DECLARE @errmsg NVARCHAR(250)
SELECT @errmsg =''
DECLARE @timeout INTeger
DECLARE @start_lock datetime
DECLARE @id_inc INT
DECLARE @sql NVARCHAR(200)
DECLARE @r_start INT
DECLARE @r_end INT
DECLARE @r_nextstart INT
DECLARE @r_nextend INT
DECLARE @r_nextcontrol INT
DECLARE @range_limit INT
DECLARE @new_id INT
SELECT @new_id = 0
DECLARE @no_lock INT
SELECT @range_limit = 10000
SELECT @timeout = 5 -- try for 5 seconds to get the lock
SET NOCOUNT ON
-- Get the Table Id from the table name
SELECT @table_id = Bord_TableId FROM Custom_Tables NOLOCK WHERE Bord_Name = @table_name
IF (@@ERROR <> 0) OR @table_id = 0 BEGIN
RAISERROR('crm_new_id: No Table Found %s',16,-1,@table_name) WITH LOG
RETURN 0
END
BEGIN TRANSACTION T1
-- keep trying until we get the lock
-- must put a timeout here and RETURN ERROR IF cannot get lock
SELECT @start_lock = getdate();
SELECT @no_lock = 0
WHILE (@no_lock = 0) AND (Getdate() < DateAdd(second,@timeout,@start_lock) ) BEGIN
INSERT INTO Locks (Lock_SessionId, Lock_TableId,Lock_RecordId,Lock_CreatedBy,Lock_CreatedDate)
VALUES(0,@table_id,-99,0,getDate())
IF (@@ERROR = 0) BEGIN
SELECT @no_lock = 1
END
END
IF @no_lock = 0 BEGIN
ROLLBACK TRANSACTION T1
RAISERROR('crm_new_id: Timeout trying to get lock (%s)',16,-1,@table_name) WITH LOG
RETURN 0
END
ELSE BEGIN
COMMIT TRANSACTION T1
END
BEGIN TRANSACTION T2
-- check out the range values for this table
SELECT @r_start = Range_RangeStart, @r_end = Range_RangeEnd,
@r_nextstart = Range_NextRangeStart, @r_nextend = Range_NextRangeEnd,
@r_nextcontrol = Range_Control_NextRange FROM Rep_Ranges NOLOCK WHERE Range_TableId = @table_id
-- get the next id for this table
EXEC @new_id = crm_next_id @table_id = @table_id
-- is the id within the range?
IF (@new_id 0) BEGIN
-- have used up all the range so create a new range
UPDATE Rep_Ranges SET
Range_RangeStart = @r_nextstart,
Range_RangeEnd = @r_nextend,
Range_NextRangeStart = @r_nextcontrol,
Range_NextRangeEnd = @r_nextcontrol + @range_limit - 1,
Range_Control_NextRange = @r_nextcontrol + @range_limit
WHERE Range_TableId = @table_id
IF @@ERROR <> 0 BEGIN
SELECT @new_id = 0
SELECT @errmsg = 'crm_new_id: Update New Range Failed (%s)'
GOTO END_TRANS
END
-- result is first id FROM new range
SELECT @new_id = @r_nextstart
-- apply the new range so eware_get_next_id will RETURN next id FROM within new range
UPDATE Sql_Identity SET Id_NextId = @r_nextstart+1 WHERE Id_TableId = @table_id
IF @@ERROR <> 0 BEGIN
SELECT @new_id = 0
SELECT @errmsg = 'crm_new_id: Apply New Range Failed (%s)'
GOTO END_TRANS
END
END
END_TRANS: -- commit or rollback the transaction
IF @new_id = 0 BEGIN
ROLLBACK TRANSACTION T2
IF @errmsg = ''
SELECT @errmsg = 'crm_new_id: Get Next Id Failed (%s)'
END
ELSE BEGIN
COMMIT TRANSACTION T2
END
--Finished so remove the lock
BEGIN TRANSACTION T3
SELECT @start_lock = getdate();
SELECT @no_lock = 0
WHILE (@no_lock = 0) AND (Getdate() < DateAdd(second,@timeout,@start_lock) ) BEGIN
DELETE FROM Locks WHERE Lock_TableId=@table_id and Lock_RecordId = -99
IF (@@ERROR = 0) BEGIN
SELECT @no_lock = 1
END
END
IF @no_lock = 0 BEGIN
ROLLBACK TRANSACTION T3
RAISERROR('crm_new_id: Error Deleting Lock (%s)',16,-1,@table_name) WITH LOG
RETURN 0
END
ELSE BEGIN
COMMIT TRANSACTION T3
END
-- Make sure RaisError is the last call, so that @@ERROR is set correctly
IF (@errmsg <> '') BEGIN
RAISERROR(@errmsg,16,-1,@table_name) WITH LOG
RETURN 0
END
SET NOCOUNT OFF
RETURN @new_id
December 17, 2007 at 10:48 am
Depending on what the procedure eware_get_identity_id does, I think this is what you're looking for.
INSERT Phone
SELECT
dbo.eware_get_identity_id('Phone')
,p.PERS_PERSONID
,z.PHON_TYPE
,LEFT(z.PHON_NUMBER,20)
,1,GETDATE(),1,GETDATE(),GETDATE()
FROM
ZZIMPPHONE z
INNER JOIN PERSON p
ON p.PERS_LEGACYACCOUNTID = z.PERS_LEGACYACCOUNTID
What do you think Matt?
______________________________________________________________________
Personal Motto: Why push the envelope when you can just open it?
If you follow the direction given HERE[/url] you'll likely increase the number and quality of responses you get to your question.
Jason L. SelburgDecember 17, 2007 at 10:48 am
For a while loop you could do something like....
SET NOCOUNT ON;
DECLARE @PHONEID INT
DECLARE @PERSONID INT
DECLARE @PERSLEGACYACCOUNTID INT
DECLARE @PHONETYPE VARCHAR(40)
DECLARE @PHONENUMBER VARCHAR(20)
INSERT INTO #TempPhone ( LoopId int IDENTITY(1,1) NOT NULL,
PHON_TYPE varchar(40),
PHON_NUMBER varchar(20),
PERSLEGACYACCOUNTID int )
INSERT INTO #TempPhone
SELECT PHON_TYPE,
LEFT(PHON_NUMBER,20),
PHON_PERSLEGACYACCOUNTID
FROM ZZIMPPHONE
WHERE PHON_PERSLEGACYACCOUNTID IN (SELECT DISTINCT PERS_LEGACYACCOUNTID FROM PERSON)
DECLARE @LoopCounter int, @Iterations int
SET @LoopCounter = 1
SELECT @Iterations = MAX(LoopId) FROM #TempPhone
WHILE @LoopCounter <= ISNULL(@Iterations,0)
BEGIN
SELECT @PHONETYPE,
@PHONENUMBER,
@PERSLEGACYACCOUNTID
FROM #TempPhone
WHERE LoopId = @LoopCounter
EXEC @PHONEID = eware_get_identity_id 'Phone'
SET @PERSONID = (SELECT TOP 1 PERS_PERSONID FROM PERSON WHERE PERS_LEGACYACCOUNTID = @PERSLEGACYACCOUNTID)
INSERT INTO Phone
(Phon_PhoneId,
Phon_PersonID,
Phon_Type,
Phon_Number,
Phon_CreatedBy,
Phon_CreatedDate,
Phon_UpdatedBy,
Phon_UpdatedDate,
Phon_TimeStamp)
VALUES
(@PHONEID,
@PERSONID,
@PHONETYPE,
@PHONENUMBER,
1,
GETDATE(),
1,
GETDATE(),
GETDATE()
)
SET @LoopCounter = @LoopCounter + 1
END
However, if you can turn eware_get_identity_id into a function or show us what it does then you could do a set based approach such as ...
INSERT INTO Phone
(Phon_PhoneId,
Phon_PersonID,
Phon_Type,
Phon_Number,
Phon_CreatedBy,
Phon_CreatedDate,
Phon_UpdatedBy,
Phon_UpdatedDate,
Phon_TimeStamp)
SELECT
dbo.fn_eware_get_identity_id ( 'Phone'),
P.PERS_PERSONID,
A.PHON_TYPE,
LEFT(A.PHON_NUMBER,20),
1,
GETDATE(),
1,
GETDATE(),
GETDATE()
FROM ZZIMPPHONE A
JOIN ( SELECT
PERS_LEGACYACCOUNTID,
MIN(PERS_PERSONID) as PERS_PERSONID
FROM PERSON
GROUP BY PERS_LEGACYACCOUNTID )P
ON A.PHON_PERSLEGACYACCOUNTID = P.PERS_LEGACYACCOUNTID
December 17, 2007 at 10:56 am
Jason Selburg (12/17/2007)
Depending on what the procedure eware_get_identity_id does, I think this is what you're looking for.
INSERT Phone
SELECT
dbo.eware_get_identity_id('Phone')
,p.PERS_PERSONID
,z.PHON_TYPE
,LEFT(z.PHON_NUMBER,20)
,1,GETDATE(),1,GETDATE(),GETDATE()
FROM
ZZIMPPHONE z
INNER JOIN PERSON p
ON p.PERS_LEGACYACCOUNTID = z.PERS_LEGACYACCOUNTID
What do you think Matt?
I think you type faster than I do:)
----------------------------------------------------------------------------------
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?
December 17, 2007 at 11:00 am
Being that your procedure does all of that, IMHO you are stuck with looping of some sort.
You can always use a temp table and loop through an IDENTITY column in it.
... as below
DECLARE @PHONEID INT
CREATE TABLE #myTable
(nDex INT IDENTITY(1,1)
,PHONETYPE VARCHAR(50) -- or what ever it should be
,PHONENUMBER VARCHAR(20)
,PERSLEGACYACCOUNTID INT)
INSERT #myTable
SELECT PHON_TYPE,LEFT(PHON_NUMBER,20),PHON_PERSLEGACYACCOUNTID
FROM ZZIMPPHONE
WHERE PHON_PERSLEGACYACCOUNTID IN (SELECT DISTINCT PERS_LEGACYACCOUNTID FROM PERSON)
DECLARE @i INT
SET @i = 1
WHILE @i <= (SELECT MAX(nDex) FROM @myTable)
BEGIN
EXEC @PHONEID = eware_get_identity_id 'Phone'
INSERT Phone
SELECT
@PHONEID
,p.PERS_PERSONID
,z.PHON_TYPE
,LEFT(z.PHON_NUMBER,20)
,1,GETDATE(),1,GETDATE(),GETDATE()
FROM
#myTable z
INNER JOIN PERSON p
ON p.PERS_LEGACYACCOUNTID = z.PERS_LEGACYACCOUNTID
WHERE
nDex = @i
SELECT @i = @i + 1
END
DROP TABLE #myTable
at least I think this will work. 😛
______________________________________________________________________
Personal Motto: Why push the envelope when you can just open it?
If you follow the direction given HERE[/url] you'll likely increase the number and quality of responses you get to your question.
Jason L. SelburgDecember 17, 2007 at 11:05 am
All right - now THAT's ugly.... Tell me - what does get_next_CRM_ID do?
Using that proprietary thing right there is the reason you're jammed up. Looks to me that someone needs to be flogged... repeatedly....for that. It assumes you will only ever need one ID at a time, which for SQL Server is invariably a bad assumption
We're going to need to enlist a Tally table for this, but let's get to the bottom of this numbering scheme first
What gets inserted into Rep_ranges? (a sample of what the data looks like)... just curious.
----------------------------------------------------------------------------------
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?
December 17, 2007 at 11:08 am
Matt Miller (12/17/2007)
All right - now THAT's ugly.... Tell me - what does get_next_CRM_ID do?Using that proprietary thing right there is the reason you're jammed up. Looks to me that someone needs to be flogged... repeatedly....for that. It assumes you will only ever need one ID at a time, which for SQL Server is invariably a bad assumption
We're going to need to enlist a Tally table for this, but let's get to the bottom of this numbering scheme first
What gets inserted into Rep_ranges? (a sample of what the data looks like)... just curious.
On that note, it appears Matt is willing to dig far deeper into this particular problem. So I respectfully bow out of this one... LOL;)
______________________________________________________________________
Personal Motto: Why push the envelope when you can just open it?
If you follow the direction given HERE[/url] you'll likely increase the number and quality of responses you get to your question.
Jason L. SelburgDecember 17, 2007 at 11:24 am
Matt Miller (12/17/2007)
Tell me - what does get_next_CRM_ID do?
Basically it gets the current valid ID from a tracking table, then updates the table with the current value +1 ready for the next execution.
What gets inserted into Rep_ranges? (a sample of what the data looks like)... just curious.
Here is a sample (Excuse the formatting):
Range_TableId Range_RangeStart Range_RangeEnd Range_NextRangeStart Range_NextRangeEnd Range_Control_NextRange
------------- ---------------- -------------- -------------------- ------------------ -----------------------
10148 6001 56000 56001 106000 106001
10149 6001 56000 56001 106000 106001
10150 6000 15999 16000 25999 26000
10151 6000 15999 16000 25999 26000
December 17, 2007 at 11:36 am
Oh no... a "sequence" table. :hehe: Third party app we got used one... cause an average of 640 deadlocks per day because of the way they wrote the increment code.
--Jeff Moden
December 17, 2007 at 11:50 am
(rolling sleeves up)
Thanks for the update...I need the DDL of the tracking table it's updating. If there is a loop in the end result, it will be one updating 10K records at a time. One at a time it, well, crazy....
The thought here to fix this:
- create a temp table with the candidate records you need to update
- get the open range. If it covers the record count you need, then assign IDs (in a set) to all records. Otherwise - use all of the remaining numbers in the sequence to update as many as you can.
- if you still have rows needing an id, request another range (another 10K if I see the SP correctly), and do the above step again on all of those with no ID.
- do # 2 and 3 until you give ID's to all that need one.
- insert them all into final table.
The million $ question is whether you are ALLOWED to "reverse engineer" the sequence assigning process (or in this case - leave the existing process alone, but build another, more efficient one next to it) without your mgmt screeching/a vendor crying/ support being yanked for doing soemthing "not supported. If you think you might, then stick with Jason's solution, although I doubt you will see much in terms of gains. While is a LITTLE more efficient than a cursor, but it probably won't be enough to write home to mom about it.... On the other hand - the process I'm talking about should finish quite a bit faster.
Otherwise - get some table definitions out... For one - all of the tables mentioned in the code above, and the one in get_next_CRM_ID.
----------------------------------------------------------------------------------
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?
December 19, 2007 at 5:14 am
Matt said: "The million $ question is whether you are ALLOWED to "reverse engineer" the sequence assigning process (or in this case - leave the existing process alone, but build another, more efficient one next to it) without your mgmt screeching/a vendor crying/ support being yanked for doing soemthing "not supported."
You are so right, there! The vendor might just get petty about it if you change their code. However one thing you can do is create the parallel process that works much better then submit it to them as a suggestion for a product enhancement. (Use the word "fix", even though that's what it is, and they might just dismiss it out of hand.)
A lot depends on the wording of your EULA. It probably specifies that you are not to touch the code but there may not be anything in there that prohibits your looking at it. If they are smart they will appreciate suggestions for improving their product and everyone wins - you get a product that runs better and it makes them look good to their other clients. :w00t:
Cheers,
Don
Viewing 15 posts - 1 through 15 (of 25 total)
You must be logged in to reply to this topic. Login to reply