February 8, 2012 at 5:08 pm
I am trying to improve the following procedure by applying some of the basic concepts of transactions. This procedure is supposed to store a record in a table and some "aftermath" records to idem number of tables. It pseudo works right now, but we have seen inconsistencies in the underlying data thus causing me to review this. This code is not very elegant (and not mine originally so please excuse the poorly formatted pasted text).
ALTER PROCEDURE [dbo].[InsertSoldInventory]
@SalesInvID as int,
@FrequencyID as int,
@ContractID as int,
@StatusID as int,
@FromDay as int = 1,
@ToDay as int = 7,
@StartDate datetime = NULL,
@EndDate datetime = NULL
AS
BEGIN
DECLARE @points decimal(13,2),@Price decimal(18,2), @status varchar(20),@InvOwner varchar(20)
DECLARE @EventType VARCHAR(20), @EventTypeID INT, @EventSubType VARCHAR(20), @EventSubTypeID INT, @SoldInventoryID INT
DECLARE @SoldLookupID int, @SalesInvStatusID INT, @Frequency VARCHAR(40), @ContractDate varchar(10), @OccupancyDate varchar(10)
Declare @ExchangeRate decimal(18,5), @ExchangePrice decimal(18,5)
IF ISNULL(@FromDay,0) = 0
SET @FromDay = 1
SELECT DISTINCT @status = LookupName
FROM tsw.Lookup
WHERE lookupid = @StatusID
SELECT @Frequency = Frequency FROM tsw.frequency WHERE frequencyid = @FrequencyID
-- Added on 09/14/10 to see the owner bucket the inventory was selected from.
SET @InvOwner = dbo.udf_GetInventoryOwnerForSalesInvID(@SalesInvID,@FrequencyID)
--Get the LookupID for the 'Sold' Inventory Status lookup value
SELECT @SoldLookUpID = cLookupID FROM tsw.cLookup WHERE cLookUpParentID = 33 and lookupName = 'Sold'
--Get the Event related values
SELECT @EventType = et.EventType, @EventTypeID = et.EventTypeID, @EventSubType = est.EventSubType,@EventSubTypeID = est.EventSubTypeID
FROM tsw.t_EventType et
LEFT JOIN tsw.t_EventSubType est ON et.EventTypeID = est.EventTypeID AND est.EventSubType = 'Create'
WHERE et.EventType = 'Contract'
--Get values from t_contract
SELECT @ContractDate = Convert(varchar(10),ContractDate,101), @OccupancyDate = Convert(varchar(10),OccupancyDate,101)
FROM tsw.t_contract WHERE contractID = @contractID
--get price from setup
SET @Price = 0
SELECT @Price = spd.SalePrice
FROM tsw.SalesInv si
JOIN tsw.t_room r ON si.roomid = r.roomid
JOIN tsw.Interval i ON si.IntervalID = i.IntervalID
JOIN tsw.RoomSalePRice rsp ON rsp.roomid = r.roomid AND (i.IntervalNum BETWEEN rsp.FromWeek AND rsp.ToWeek OR (rsp.FromWeek = 0AND rsp.ToWeek = 0))
JOIN tsw.SalePRice sp ON sp.SalePRiceID = rsp.SalePRiceID
JOIN tsw.SalePriceDetail spd ON spd.SalePRiceID = sp.SalePriceID AND spd.FrequencyID = @FrequencyID
WHERE si.SalesInvID = @SalesInvID
SELECT @ExchangeRate = tsw.tsw_fnContractCurrencyGetRate(@ContractID)
SELECT @ExchangePrice = @Price * @ExchangeRate
--Get Point value from the club setup.
Select @Points = ci.PointValue
From tsw.ClubPVInterval ci
Join tsw.ClubPointValue cv on cv.ClubPointValueID = ci.ClubPointValueID
Join tsw.SalesInv si on si.SalesInvID = @SalesInvID
Join tsw.Interval i on i.IntervalID = si.IntervalID
join tsw.t_Room r on si.roomid = r.roomid
join tsw.RoomType rt on r.roomtypeid = rt.roomtypeid
Join tsw.SalesInvOwner sio on si.salesinvID = sio.SalesInvID
Join tsw.InventoryOwner iown on sio.inventoryOwnerID = iown.inventoryOwnerID
Where i.intervalnum Between ci.StartInterval and ci.EndInterval
And cv.ClubID = iown.ClubID
And cv.RoomTypeID = rt.RoomTypeID
And cv.SiteID = r.SiteID
And @StartDate Between cv.StartDate and cv.EndDate
--Double-check to make sure room isn't already sold
IF EXISTS
(Select *
From tsw.SalesInvStatus sis
where sis.SalesInvID = @salesInvID
And sis.FrequencyID = @frequencyID
And (
sis.FromDay BETWEEN @FromDay AND @toDay
OR sis.ToDay BETWEEN @FromDay AND @toDay
)
And sis.Active = 1
And IsNull(sis.SoldInventoryID,0) = 0
)
Or Exists
(Select *
From tsw.t_SoldInventory so
Join tsw.SalesInvStatus sis on so.SoldInventoryID = sis.SoldInventoryID
Where sis.SalesInvID = @salesInvID
and sis.active = 1
And sis.FrequencyID = @frequencyID
And (
sis.FromDay BETWEEN @FromDay AND @toDay
OR sis.ToDay BETWEEN @FromDay AND @toDay
)
And (so.StartDate Between @StartDate and IsNull(@EndDate,'99991231')
or
@StartDate Between so.StartDate and IsNull(so.ExpirationDate,'99991231'))
)
Begin
RAISERROR('Availability conflict: this room is not available for at least part of that time.', 16, 2)
RETURN
End
BEGIN TRY
BEGIN TRANSACTION
-- insert the soldinventory record
INSERT INTO
dbo.t_SoldInventory(SiteID,ContractID,SalesInventoryID,[week],WeekTypeID,WeekType,roomNumber,Roomtypeid,RoomType,frequency,
DaysAllowed,[Status],STatusID,SalePrice,SaleDate,Points,RoomID,SeasonID,Season,
--InventoryTypeID,InventoryType,
StartDate, ExpirationDate, User1,User2,User3,User4,User5,User6,User7,User8,User9,User10,User11,User12,User13,User14,User15,BaseAmount)
SELECT DISTINCTr.siteid,@ContractID,@SalesInvID,i.intervalnum,si.WeekTypeID,(Select WeekType from tsw.WeekType where WeekTypeID = si.WeekTypeID),
r.roomNumber,r.roomtypeid,rt.roomtype,@Frequency, @toDay - @FromDay +1,@Status,@StatusID,@Price,@ContractDate,@Points,r.roomid,si.SeasonID,
(Select Season from tsw.Season where SeasonID = si.SeasonID and siteID = r.SiteID),
@OccupancyDate, @EndDate, si.User1, si.User2,si.User3,si.User4,si.User5,si.User6,si.User7,si.User8,si.User9,@InvOwner,si.User11,@FrequencyID,
si.User13,si.User14,si.User15,@ExchangePrice
from tsw.SalesInv si
join tsw.t_room r on si.roomid =r.roomid
join tsw.roomtype rt on r.roomtypeid = rt.roomtypeid
Join tsw.Interval i on i.IntervalID = si.IntervalID
where si.SalesInvID = @SalesInvID
SET @SoldInventoryID = SCOPE_IDENTITY() --SCOPE_IDENTITY returns values inserted only within the current scope
-- Insert the SalesInvStatus
Insert into tsw.SalesInvStatus (SalesInvID,FromDay,ToDay,SoldInventoryID,Active,FrequencyID,InventoryStatusID)
Select @SalesInvID,@FromDay,@ToDay,@SoldInventoryID,1,@FrequencyID,@SoldLookupId
SET @SalesInvStatusID = SCOPE_IDENTITY() --SCOPE_IDENTITY returns values inserted only within the current scope
INSERT INTO dbo.SoldInventoryExchangeRate(SoldInventoryID,ExchangeRate) VALUES (@SoldInventoryID,@ExchangeRate)
-- Now create the event
INSERT INTO tsw.t_Event(SiteID, EventTypeID, EventType, EventSubTypeID, EventSubType, Comments, DateCreated, TimeCreated, UserName, RecordID, TableName)
SELECT s.SiteID, @EventTypeID, @EventType, @EventSubTypeID, @EventSubType, 'SoldInventory ' + CONVERT(varchar, s.SoldInventoryID) + ' from Buy Days',
CONVERT(varchar, GETDATE(), 112), GETDATE(), tsw.fnUserName(), s.ContractID, 'Contract'
FROM tsw.t_SoldInventory s
WHERE s.SoldInventoryID = @SoldInventoryID
--All is good - write it out
COMMIT TRANSACTION
END TRY
BEGIN CATCH
-- Test XACT_STATE for 1 or -1.
-- XACT_STATE = 0 means there is no transaction and
-- a commit or rollback operation would generate an error.
-- Test whether the transaction is uncommittable.
IF (XACT_STATE()) = -1
BEGIN
PRINT N'The transaction is in an uncommittable state. Rolling back transaction.'
ROLLBACK TRANSACTION
END
-- Test whether the transaction is active and valid.
IF (XACT_STATE()) = 1
BEGIN
PRINT N'The transaction is committable. Committing transaction.'
COMMIT TRANSACTION
END
END CATCH
END
Any comments/suggestions are always welcome for they help me learn and move forward.
Thank you in advance.
February 9, 2012 at 12:43 pm
The only comment I'd have is that your check for availability is outside the transaction, so in theory you could have another session create a conflicting reservation at the same time. You could fix that by putting the check within the transaction so the tables are locked from the check through the end of the transaction. That way any other process that is attempting that check would likely be blocked until this transaction completes.
Jack Corbett
Consultant - Straight Path Solutions
Check out these links on how to get faster and more accurate answers:
Forum Etiquette: How to post data/code on a forum to get the best help
Need an Answer? Actually, No ... You Need a Question
February 9, 2012 at 2:56 pm
Thanks Jack!
I had the same concern, just looking for reassurance I guess. I will move the check into the Transaction so I get all my bases covered.
Kind regards,
February 9, 2012 at 3:04 pm
Jack Corbett (2/9/2012)
The only comment I'd have is that your check for availability is outside the transaction, so in theory you could have another session create a conflicting reservation at the same time. You could fix that by putting the check within the transaction so the tables are locked from the check through the end of the transaction. That way any other process that is attempting that check would likely be blocked until this transaction completes.
However you'd need a non-default isolation level (repeatable read, serializable or snapshot) or a locking hint to take an xlock, or even in the transaction the shared lock taken by the availability check would be released immediately.
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
February 9, 2012 at 3:42 pm
Hi Gail,
Well, for those of us who are really NEWBIES, how can I do that? if you don't mind my ignorance 🙂
February 10, 2012 at 5:15 am
I'm not sure I understand the logic behind the "IF (XACT_STATE()) = 1" part in the catch statement. Do you really want to commit just because you can? 🙂
February 10, 2012 at 6:53 am
kent_secher (2/10/2012)
I'm not sure I understand the logic behind the "IF (XACT_STATE()) = 1" part in the catch statement. Do you really want to commit just because you can? 🙂
Good catch. I should have mentioned that as well. Since you are in the CATCH block you have an error so you probably just want to ROLLBACK. I usually do an IF XACT_STATE <> 0 ROLLBACK TRANSACTION, in the CATCH block.
What Gail means and I should have caught is you need to supply a locking hint like WITH (UPDLOCK) or WITH (XLOCK) or use a SET statement to change your isolation level like SET TRANSACTION ISOLATION LEVEL REPEATABLE READ. Check out BOL for SET TRANSACTION ISOLATION LEVEL
I always struggle with remembering all the locking and blocking semantics.
Jack Corbett
Consultant - Straight Path Solutions
Check out these links on how to get faster and more accurate answers:
Forum Etiquette: How to post data/code on a forum to get the best help
Need an Answer? Actually, No ... You Need a Question
February 10, 2012 at 7:23 am
I used XACT_STATE just for troubleshooting or debugging and it won't be in the production version. Sorry, I should've put that in the code.
As for the isolation level, I believe that it might be best to use the table hint instead, no? (Again, looking for confirmation and/or best practices)
Thank you all!!
February 10, 2012 at 7:57 am
You should leave the XACT_STATE in but change it to XACT_STATE() <> 0. It is a best practice to check to make sure the transaction is available to be ROLLED BACK before rolling it back.
Jack Corbett
Consultant - Straight Path Solutions
Check out these links on how to get faster and more accurate answers:
Forum Etiquette: How to post data/code on a forum to get the best help
Need an Answer? Actually, No ... You Need a Question
February 10, 2012 at 12:36 pm
Thanks Jack!
If I may abuse of your knowledge a bit more, I have moved my availability check inside the BEGIN TRY BEGIN TRANSACTION block but I am at a loss as to how do I break if the condition is met, with a BREAK or just raise the error as I have it right now?
BEGIN TRY
BEGIN TRANSACTION
--Double-check to make sure room isn't already sold
IF EXISTS
(Select *
From tsw.SalesInvStatus sis
where sis.SalesInvID = @salesInvID
And sis.FrequencyID = @frequencyID
And (
sis.FromDay BETWEEN @FromDay AND @toDay
OR sis.ToDay BETWEEN @FromDay AND @toDay
)
And sis.Active = 1
And IsNull(sis.SoldInventoryID,0) = 0
)
Or Exists
(Select *
From tsw.t_SoldInventory so
Join tsw.SalesInvStatus sis on so.SoldInventoryID = sis.SoldInventoryID
Where sis.SalesInvID = @salesInvID
and sis.active = 1
And sis.FrequencyID = @frequencyID
And (
sis.FromDay BETWEEN @FromDay AND @toDay
OR sis.ToDay BETWEEN @FromDay AND @toDay
)
And (so.StartDate Between @StartDate and IsNull(@EndDate,'99991231')
or
@StartDate Between so.StartDate and IsNull(so.ExpirationDate,'99991231'))
)
Begin
RAISERROR('Availability conflict: this room is not available for at least part of that time.', 16, 2)
RETURN
End
Thanks again!
JC
February 10, 2012 at 12:55 pm
Raising an error with that severity level in a TRY block sends execution to the CATCH block so you don't need the RETURN. You just need to decide what you want to send back to the client from the CATCH block when that happens and ROLLBACK the TRANSACTION.
Jack Corbett
Consultant - Straight Path Solutions
Check out these links on how to get faster and more accurate answers:
Forum Etiquette: How to post data/code on a forum to get the best help
Need an Answer? Actually, No ... You Need a Question
Viewing 11 posts - 1 through 10 (of 10 total)
You must be logged in to reply to this topic. Login to reply