none
Cursors or repeating code?

    Question

  • Hi everybody,

    I am trying to figure out a good solution for the following problem. In our database we have several tables that are used for reserved seating. In particular, we have, rsVenues, rsTemplates, rsLevels, rsSections, etc. tables.

    I want to write stored procedures to delete a venue by ID, template by ID, level by ID, etc.

    Each of these stored procedures is very complex since we have many relationships between tables and we need to make sure all extra tables are considered and the deletion is happening in the right order.

    So, my problem right now is that I am repeating logic in all these procedures.

    I want to be able to re-use the same code. Unfortunately, if, for example, I will call delete sections from delete level procedure, I will need to use a cursor.

    So, I am not sure what is the best approach in my case?

    What do you think?

    Thanks in advance.


    For every expert, there is an equal and opposite expert. - Becker's Law


    My blog


    My TechNet articles

    Monday, November 04, 2013 6:55 PM

Answers

  • Cursor use is OK in your application if there is no scaling problem.

    Kalman Toth Database & OLAP Architect IPAD SELECT Query Video Tutorial 3.5 Hours
    New Book / Kindle: Exam 70-461 Bootcamp: Querying Microsoft SQL Server 2012


    Monday, November 04, 2013 7:08 PM
  • In the case you describe, I would use a cursor to call each lower stored proc, like:

    CURSOR SELECT templateid where venueid = @venueid

    EXEC deleteTemplate(@template)

    END

    DELETE venue where venueid = @venueid

    etc

    Monday, November 04, 2013 9:18 PM
  • The second option there, the one that uses a temp table to pass a set of ID values to the child procedure will work on SQL 2005.

    David


    David http://blogs.msdn.com/b/dbrowne/

    Tuesday, November 05, 2013 12:36 AM
  • Right, that's the "basically" part. 

    You are free to add a constraint to a temporary table, but the ALTER TABLE syntax doesn't allow you to add a primary key without giving it a name.  And primary keys names are in a database-wide namespace.

    So while this is legal:

    create table #bar(id int not null)
    alter table #bar add constraint pk_bar primary key (id) 

    If another session runs the same code it will fail with:

    Msg 2714, Level 16, State 5, Line 1

    There is already an object named 'pk_bar' in the database.

    Msg 1750, Level 16, State 0, Line 1

    Could not create constraint. See previous errors.


    So you would have to figure out how to generate a unique PK name, which is way more trouble than it's worth.

    David


    David http://blogs.msdn.com/b/dbrowne/


    Tuesday, November 05, 2013 5:24 PM
  • Yes.  Although you should always call RAISERROR before returning a non-zero value.  Only TSQL code that is specifically checking the return code will see it.  It creates a risk that current or future callers of the stored procedure might call the stored procedure without checking the return code.

    For instance client code in .NET or whatever almost never binds returncode parameters and checks the return code of stored procedures.

    So all stored procedures should always RAISERROR or THROW on error, in addition to returning a non-zero return value.

    Also delete

    IF OBJECT_ID(N'tempdb..#DelSections', N'U') IS NOT NULL
    			DROP TABLE #DelSections;
    

    If a session-scope temp table or procedure-scoped temp table in a calling procedure exists, you don't want to drop it.  When you create the temp table in your procedure, it will not conflict. 

    But better form would be to use a table variable when you need a temporary storage location that is purely local.

    David


    David http://blogs.msdn.com/b/dbrowne/


    Tuesday, November 05, 2013 7:09 PM

All replies

  • Cursor use is OK in your application if there is no scaling problem.

    Kalman Toth Database & OLAP Architect IPAD SELECT Query Video Tutorial 3.5 Hours
    New Book / Kindle: Exam 70-461 Bootcamp: Querying Microsoft SQL Server 2012


    Monday, November 04, 2013 7:08 PM
  • Can you use ON DELETE CASCADE Foreign Keys to reduce the number of related entities that you have to explicitly touch in your stored procedures?

    David


    David http://blogs.msdn.com/b/dbrowne/

    Monday, November 04, 2013 7:14 PM
  • I am not exactly sure why we're not using ON DELETE CASCADE in our database in general. May be to make the life more difficult and may be because we want to be in control of deletion process.  So, I am afraid it is not a solution.

    For every expert, there is an equal and opposite expert. - Becker's Law


    My blog


    My TechNet articles

    Monday, November 04, 2013 7:38 PM
  • I would break that down into several functions or stored procs, so you don't need to duplicate the check code in every one.

    IsTemplateUsed(ID), IsLevelUsed(ID), etc

    Even though in your proc you do have similar code, it makes it a little easier to read and reuse.

    Monday, November 04, 2013 8:35 PM
  • Well, just to show what I am talking about here: (I think delete sections is one of the lowest de-nominators):

    ------------------

    ALTER PROCEDURE dbo.siriussp_rsDeleteSection (@SectionID INT)
    AS
    BEGIN
    	SET NOCOUNT ON;
    	DECLARE @ErrorMessage NVARCHAR(4000), @Descrip VARCHAR(100) ;
    	IF EXISTS(SELECT 1 FROM dbo.rsHolds H WHERE SectionId = @SectionID 
    	AND HoldExpire > CURRENT_TIMESTAMP AND IsEnabled = 1)
    	   BEGIN
    			SELECT @Descrip = descrip FROM dbo.rsSections WHERE SectionID = @SectionID;
    			SET @ErrorMessage = 'Section ' + CAST(@SectionID AS VARCHAR(20)) + '(' + @Descrip + ') can not be deleted since there are existing holds referencing that section';
    			RAISERROR(@ErrorMessage, 16, 1);
    			RETURN - 1;
    	   END
    	   
    	IF EXISTS(SELECT 1 FROM dbo.rsSeats S WHERE SectionId = @SectionID 
    	AND trans_no <> 0)
    	   BEGIN
    			SELECT @Descrip = descrip FROM dbo.rsSections WHERE SectionID = @SectionID;
    			SET @ErrorMessage = 'Section ' + CAST(@SectionID AS VARCHAR(20)) + '(' + @Descrip + ') can not be deleted since there are sold Seats referencing that section';
    			RAISERROR(@ErrorMessage, 16, 1);
    			RETURN - 1;
    	   END   
    	BEGIN TRY
    		IF OBJECT_ID('TempDB..#DelBlocks', 'U') IS NOT NULL
    			DROP TABLE #DelBlocks;
    
    		SELECT BlockId
    		INTO #DelBlocks
    		FROM dbo.rsTSBlocks B
    		WHERE B.SectionID = @SectionID;
    
    		BEGIN TRANSACTION
    
    		DELETE FROM dbo.rsSeats WHERE SectionID = @SectionID ;
    		DELETE FROM dbo.rsHolds WHERE SectionID = @SectionID ;
    		DELETE FROM dbo.rsSectLink WHERE SectionID = @SectionID ;
    		DELETE FROM dbo.rsSectionAvailabilityRules WHERE SectionID = @SectionID ;
    		
    		-- Exceptions first
    		DELETE TE
    		FROM dbo.rsTExcepts TE
    		WHERE EXISTS (
    				SELECT 1
    				FROM #DelBlocks D
    				WHERE D.BlockID = TE.BlockID
    				);
    
    		-- Blocks
    		DELETE B
    		FROM dbo.rsTSBlocks B
    		WHERE B.SectionID = @SectionID;
    
    		-- Sections
    		DELETE S
    		FROM dbo.rsSections S
    		WHERE S.SectionID = @SectionID;
    
    		COMMIT TRANSACTION
    	END TRY
    
    	BEGIN CATCH
    		DECLARE @ErrorSeverity INT
    			,@ErrorNumber INT		
    			,@ErrorState INT
    			,@ErrorLine INT
    			,@ErrorProc NVARCHAR(200)
    
    		-- Grab error information from SQL functions
    		SET @ErrorSeverity = ERROR_SEVERITY()
    		SET @ErrorNumber = ERROR_NUMBER()
    		SET @ErrorMessage = ERROR_MESSAGE()
    		SET @ErrorState = ERROR_STATE()
    		SET @ErrorLine = ERROR_LINE()
    		SET @ErrorProc = ERROR_PROCEDURE()
    		SET @ErrorMessage = 'Problem deleting Section ' + CAST(@SectionID AS VARCHAR(10)) + CHAR(13) + 'SQL Server Error Message is: ' + CAST(@ErrorNumber AS VARCHAR(10)) + ' in procedure: ' + @ErrorProc + ' Line: ' + CAST(@ErrorLine AS VARCHAR(10)) + ' Error text: ' + @ErrorMessage
    
    		-- Not all errors generate an error state, to set to 1 if it's zero
    		IF @ErrorState = 0
    			SET @ErrorState = 1
    
    		-- If the error renders the transaction as uncommittable or we have open transactions, we may want to rollback
    		IF @@TRANCOUNT > 0
    		BEGIN
    			ROLLBACK TRANSACTION
    		END
    
    		RAISERROR (
    				@ErrorMessage
    				,@ErrorSeverity
    				,@ErrorState
    				,@ErrorNumber
    				)
    	END CATCH
    
    	RETURN @@ERROR
    END
    GO
    

    And the delete venue currently is (I haven't changed it yet):

    ALTER PROCEDURE dbo.siriussp_rsDeleteVenue (@VenueID INT)
    AS
    BEGIN
    	SET NOCOUNT ON;
    
    	BEGIN TRY
    		IF OBJECT_ID('TempDB..#DelTemplates', 'U') IS NOT NULL
    			DROP TABLE #DelTemplates;
    
    		IF OBJECT_ID('TempDB..#DelLevels', 'U') IS NOT NULL
    			DROP TABLE #DelLevels;
    
    		IF OBJECT_ID('TempDB..#DelSections', 'U') IS NOT NULL
    			DROP TABLE #DelSections;
    
    		IF OBJECT_ID('TempDB..#DelBlocks', 'U') IS NOT NULL
    			DROP TABLE #DelBlocks;
    
    		IF OBJECT_ID('TempDB..#DelProds', 'U') IS NOT NULL
    			DROP TABLE #DelProds;
    
    		SELECT TemplateId
    		INTO #DelTemplates
    		FROM dbo.rsTemplates
    		WHERE VenueID = @VenueID;
    
    		SELECT ProdId
    		INTO #DelProds
    		FROM dbo.rsProds P
    		WHERE EXISTS (
    				SELECT 1
    				FROM #DelTemplates T
    				WHERE T.TemplateID = P.TemplateID
    				);
    
    		SELECT LevelID
    		INTO #DelLevels
    		FROM dbo.rsLevels L
    		WHERE EXISTS (
    				SELECT 1
    				FROM #DelTemplates D
    				WHERE D.TemplateID = L.TemplateID
    				);
    
    		SELECT SectionID
    		INTO #DelSections
    		FROM dbo.rsSections S
    		WHERE EXISTS (
    				SELECT 1
    				FROM #DelLevels D
    				WHERE D.LevelID = S.LevelID
    				);
    
    		SELECT BlockId
    		INTO #DelBlocks
    		FROM dbo.rsTSBlocks B
    		WHERE EXISTS (
    				SELECT 1
    				FROM #DelSections D
    				WHERE D.SectionID = B.SectionID
    				);
    
    		BEGIN TRANSACTION
    
    		-- Seat Maps
    		DELETE M
    		FROM dbo.rsSeatMaps M
    		WHERE EXISTS (
    				SELECT 1
    				FROM #DelTemplates D
    				WHERE D.TemplateID = M.TemplateID
    				);
    
    		-- Exceptions first
    		DELETE TE
    		FROM dbo.rsTExcepts TE
    		WHERE EXISTS (
    				SELECT 1
    				FROM #DelBlocks D
    				WHERE D.BlockID = TE.BlockID
    				);
    
    		-- Blocks
    		DELETE B
    		FROM dbo.rsTSBlocks B
    		WHERE EXISTS (
    				SELECT 1
    				FROM #DelBlocks D
    				WHERE D.BlockID = B.BlockID
    				);
    
    		-- Sections
    		DELETE S
    		FROM dbo.rsSections S
    		WHERE EXISTS (
    				SELECT 1
    				FROM #DelSections D
    				WHERE D.SectionID = S.SectionID
    				);
    
    		-- Levels 
    		DELETE L
    		FROM dbo.rsLevels L
    		WHERE EXISTS (
    				SELECT 1
    				FROM #DelLevels D
    				WHERE D.LevelID = L.LevelID
    				);
    
    		-- Shows and Productions
    		DELETE Sh
    		FROM dbo.rsShows Sh
    		WHERE EXISTS (
    				SELECT 1
    				FROM #DelProds P
    				WHERE P.ProdID = Sh.ProdID
    				);
    
    		DELETE P
    		FROM dbo.rsProds P
    		WHERE EXISTS (
    				SELECT 1
    				FROM #DelProds DP
    				WHERE P.ProdID = DP.ProdID
    				);
    
    		-- Template
    		DELETE T
    		FROM dbo.rsTemplates T
    		WHERE EXISTS (
    				SELECT 1
    				FROM #DelTemplates D
    				WHERE D.TemplateID = T.TemplateID
    				);
    
    		-- Venue
    		DELETE
    		FROM dbo.rsVenues
    		WHERE VenueID = @VenueID;
    
    		COMMIT TRANSACTION
    	END TRY
    
    	BEGIN CATCH
    		DECLARE @ErrorSeverity INT
    			,@ErrorNumber INT
    			,@ErrorMessage NVARCHAR(4000)
    			,@ErrorState INT
    			,@ErrorLine INT
    			,@ErrorProc NVARCHAR(200)
    
    		-- Grab error information from SQL functions
    		SET @ErrorSeverity = ERROR_SEVERITY()
    		SET @ErrorNumber = ERROR_NUMBER()
    		SET @ErrorMessage = ERROR_MESSAGE()
    		SET @ErrorState = ERROR_STATE()
    		SET @ErrorLine = ERROR_LINE()
    		SET @ErrorProc = ERROR_PROCEDURE()
    		SET @ErrorMessage = 'Problem deleting venue ' + CAST(@VenueID AS VARCHAR(10)) + CHAR(13) + 'SQL Server Error Message is: ' + CAST(@ErrorNumber AS VARCHAR(10)) + ' in procedure: ' + @ErrorProc + ' Line: ' + CAST(@ErrorLine AS VARCHAR(10)) + ' Error text: ' + @ErrorMessage
    
    		-- Not all errors generate an error state, to set to 1 if it's zero
    		IF @ErrorState = 0
    			SET @ErrorState = 1
    
    		-- If the error renders the transaction as uncommittable or we have open transactions, we may want to rollback
    		IF @@TRANCOUNT > 0
    		BEGIN
    			ROLLBACK TRANSACTION
    		END
    
    		RAISERROR (
    				@ErrorMessage
    				,@ErrorSeverity
    				,@ErrorState
    				,@ErrorNumber
    				)
    	END CATCH
    
    	RETURN @@ERROR
    END
    GO

    So, the delete venue should call delete template for all the templates for this venue, the delete template should call delete levels and delete levels should call delete sections. Delete block is a separate procedure also.

    --------------------------------

    So, I either should use cursors and lots of nested levels or repeat the code.


    For every expert, there is an equal and opposite expert. - Becker's Law


    My blog


    My TechNet articles

    Monday, November 04, 2013 8:53 PM
  • I'm not sure where the repeated code is coming in.

    But you can consider

    1) Generating the CRUD stored procedures based on your table relationships.

    2) Using stored procedures to factor out the repeated code.

    David


    David http://blogs.msdn.com/b/dbrowne/

    Monday, November 04, 2013 9:17 PM
  • In the case you describe, I would use a cursor to call each lower stored proc, like:

    CURSOR SELECT templateid where venueid = @venueid

    EXEC deleteTemplate(@template)

    END

    DELETE venue where venueid = @venueid

    etc

    Monday, November 04, 2013 9:18 PM
  • Or you could build delete procedures that accept multiple Ids to delete.  eg:

    create procedure DeleteThings @IdsToDelete TableOfIDs readonly
    as
    begin
      delete from Thing where id in (select id from @IdsToDelete);
    end
    
    go
    
    create procedure DeleteThing @ThingID int
    as
    begin
    /*
    
    //to delete one thing
    declare @id int = 23
    exec DeleteThing 23
    
    //to delete a bunch of things
    create table #ThingIds( id int primary key)
    insert into #ThingIds select ThingId from Thing where otherId = 122
    exec DeeleteThing null
    drop table #ThingIds
    
    
    */
    
       if @ThingID is not null
       begin
         create table #ThingIds( id int primary key)
         insert into #ThingIds (id) values (@ThingId)
       end
    
       delete from ThingChildTable where ThingId in (select id from #ThingIds)
       delete from Thing where ThingId in (select id from #ThingIds)
    
    end
    

    David


    David http://blogs.msdn.com/b/dbrowne/


    Monday, November 04, 2013 9:40 PM
  • This sounds like a best idea so far except for the fact we're still supporting SQL 2005 and using XML and convert back and forth also doesn't appeal to me.

    For every expert, there is an equal and opposite expert. - Becker's Law


    My blog


    My TechNet articles

    Monday, November 04, 2013 10:25 PM
  • I think I am going to go with this idea (a dreaded cursor). Each venue may have just a few templates, each template a few levels, each level just a few sections. So, I think this should work.

    For every expert, there is an equal and opposite expert. - Becker's Law


    My blog


    My TechNet articles

    Monday, November 04, 2013 10:28 PM
  • I am not exactly sure why we're not using ON DELETE CASCADE in our database in general. May be to make the life more difficult and may be because we want to be in control of deletion process.  So, I am afraid it is not a solution.
    The usual reason is legacy code that has a horrible design. Why would anyone write with flags in SQL? Why is being on hold an entity? It sounds like a status. Since we have no DDL (Netiquette, please) we can only guess. And DRI actions are the correct way to do this RDBMS with ON DELETE CASCADE. not functions, cursors or other procedural code. 

    --CELKO-- Books in Celko Series for Morgan-Kaufmann Publishing: Analytics and OLAP in SQL / Data and Databases: Concepts in Practice Data / Measurements and Standards in SQL SQL for Smarties / SQL Programming Style / SQL Puzzles and Answers / Thinking in Sets / Trees and Hierarchies in SQL

    Monday, November 04, 2013 11:47 PM
  • The second option there, the one that uses a temp table to pass a set of ID values to the child procedure will work on SQL 2005.

    David


    David http://blogs.msdn.com/b/dbrowne/

    Tuesday, November 05, 2013 12:36 AM
  • The procedures need to have a single Id as a parameter, e.g. DeleteSection @SectionId

    However, I guess I can have it conditional and if @Id is not passed check temp table with particular name that is supposed to be created in the calling procedure.

    This should work, do you advise this solution?

    E.g. in DeleteLevel procedure

    select SectionId into #DelSections where LevelId = @LevelId

    And in deleteSections procedure look for #DelSections table or @SectionId.

    So, it will be conditional code, but should work and not use a cursor.


    For every expert, there is an equal and opposite expert. - Becker's Law


    My blog


    My TechNet articles

    Tuesday, November 05, 2013 3:28 AM
  • >So, it will be conditional code, but should work and not use a cursor.

    Yes.  Although you should have a primary key on the temp table, to ensure that in the single-row case you don't get a funky plan.

    David


    David http://blogs.msdn.com/b/dbrowne/

    Tuesday, November 05, 2013 3:40 PM
  • Hmm, this means I would need to use CREATE TABLE statement instead of SELECT INTO, right?

    I started to re-work the DeleteSection procedure today to use that idea.


    For every expert, there is an equal and opposite expert. - Becker's Law


    My blog


    My TechNet articles

    Tuesday, November 05, 2013 4:58 PM
  • >this means I would need to use CREATE TABLE statement instead of SELECT INTO

    Basically, yes. 

    David


    David http://blogs.msdn.com/b/dbrowne/

    Tuesday, November 05, 2013 5:17 PM
  • Are you sure? I think we can add a clustered index on the table created with SELECT INTO statement (need to test that, though). I also wanted to check how can I test existence of the index for the temp table. Started to run a search and got distracted...

    For every expert, there is an equal and opposite expert. - Becker's Law


    My blog


    My TechNet articles

    Tuesday, November 05, 2013 5:19 PM
  • Right, that's the "basically" part. 

    You are free to add a constraint to a temporary table, but the ALTER TABLE syntax doesn't allow you to add a primary key without giving it a name.  And primary keys names are in a database-wide namespace.

    So while this is legal:

    create table #bar(id int not null)
    alter table #bar add constraint pk_bar primary key (id) 

    If another session runs the same code it will fail with:

    Msg 2714, Level 16, State 5, Line 1

    There is already an object named 'pk_bar' in the database.

    Msg 1750, Level 16, State 0, Line 1

    Could not create constraint. See previous errors.


    So you would have to figure out how to generate a unique PK name, which is way more trouble than it's worth.

    David


    David http://blogs.msdn.com/b/dbrowne/


    Tuesday, November 05, 2013 5:24 PM
  • I see your point now. I was concentrating on the 'index' part and forgot I need to add a constraint in this case.

    I'll go with the CREATE TABLE solution then, thanks again for your help.


    For every expert, there is an equal and opposite expert. - Becker's Law


    My blog


    My TechNet articles

    Tuesday, November 05, 2013 5:33 PM
  • Yes, you could add a unique, clustered index instead of a Primary Key constraint, which is physically identical to a clustered primary key.  Index names do not have to be globally unique.

    eg

    create table #bar(id int not null)
    create unique clustered index uk_bar on #bar(id)

    David


    David http://blogs.msdn.com/b/dbrowne/

    Tuesday, November 05, 2013 5:49 PM
  • By giving me too many options you're making me procrastinate even more :)

    So, what am I going to do -  I am almost sold on the CREATE TABLE solution and if I would not get constantly distracted by emails, I would have finished at least one SP already...


    For every expert, there is an equal and opposite expert. - Becker's Law


    My blog


    My TechNet articles

    Tuesday, November 05, 2013 5:55 PM
  • IMO it's a best practice to always use CREATE TABLE to create temporary tables. 

    David


    David http://blogs.msdn.com/b/dbrowne/


    Tuesday, November 05, 2013 6:16 PM
  • That's what I have now using this idea - does it look OK? If yes, I'm going to work my way up.

    ALTER PROCEDURE dbo.siriussp_rsDeleteSection (@SectionID INT = NULL)
    AS
    BEGIN
    	SET NOCOUNT ON;
    
    	IF @SectionID IS NULL
    		AND OBJECT_ID(N'tempdb..#DelSections', N'U') IS NULL
    		RETURN - 2;-- bad call
    
    	DECLARE @ErrorMessage NVARCHAR(4000)
    		,@Descrip VARCHAR(100);
    
    	IF @SectionID IS NOT NULL
    		AND EXISTS (
    			SELECT 1
    			FROM dbo.rsHolds H
    			WHERE SectionId = @SectionID
    				AND HoldExpire > CURRENT_TIMESTAMP
    				AND IsEnabled = 1
    			)
    	BEGIN
    		SELECT @Descrip = descrip
    		FROM dbo.rsSections
    		WHERE SectionID = @SectionID;
    
    		SET @ErrorMessage = 'Section ' + CAST(@SectionID AS VARCHAR(20)) + '(' + @Descrip + ') can not be deleted since there are existing holds referencing that section';
    
    		RAISERROR (
    				@ErrorMessage
    				,16
    				,1
    				);
    
    		RETURN - 1;
    	END
    
    	IF @SectionID IS NOT NULL
    		AND EXISTS (
    			SELECT 1
    			FROM dbo.rsSeats S
    			WHERE SectionId = @SectionID
    				AND trans_no <> 0
    			)
    	BEGIN
    		SELECT @Descrip = descrip
    		FROM dbo.rsSections
    		WHERE SectionID = @SectionID;
    
    		SET @ErrorMessage = 'Section ' + CAST(@SectionID AS VARCHAR(20)) + '(' + @Descrip + ') can not be deleted since there are sold Seats referencing that section';
    
    		RAISERROR (
    				@ErrorMessage
    				,16
    				,1
    				);
    
    		RETURN - 1;
    	END
    
    	IF @SectionID IS NOT NULL
    	BEGIN
    		IF OBJECT_ID(N'tempdb..#DelSections', N'U') IS NOT NULL
    			DROP TABLE #DelSections;
    
    		CREATE TABLE #DelSections (SectionID INT PRIMARY KEY);
    
    		INSERT INTO #DelSections
    		SELECT SectionID
    		FROM dbo.rsSections
    		WHERE SectionID = @SectionID;
    	END
    
    	BEGIN TRY
    		IF OBJECT_ID('TempDB..#DelBlocks', 'U') IS NOT NULL
    			DROP TABLE #DelBlocks;
    
    		SELECT BlockId
    		INTO #DelBlocks
    		FROM dbo.rsTSBlocks B
    		WHERE B.SectionID IN (
    				SELECT SectionID
    				FROM #DelSections
    				);
    
    		BEGIN TRANSACTION
    
    		DELETE
    		FROM dbo.rsSeats
    		WHERE SectionID IN (
    				SELECT SectionID
    				FROM #DelSections
    				);
    
    		DELETE
    		FROM dbo.rsHolds
    		WHERE SectionID IN (
    				SELECT SectionID
    				FROM #DelSections
    				);
    
    		DELETE
    		FROM dbo.rsSectLink
    		WHERE SectionID IN (
    				SELECT SectionID
    				FROM #DelSections
    				);
    
    		DELETE
    		FROM dbo.rsSectionAvailabilityRules
    		WHERE SectionID IN (
    				SELECT SectionID
    				FROM #DelSections
    				);
    
    		-- Exceptions first
    		DELETE TE
    		FROM dbo.rsTExcepts TE
    		WHERE EXISTS (
    				SELECT 1
    				FROM #DelBlocks D
    				WHERE D.BlockID = TE.BlockID
    				);
    
    		-- Blocks
    		DELETE B
    		FROM dbo.rsTSBlocks B
    		WHERE B.SectionID IN (
    				SELECT SectionID
    				FROM #DelSections
    				);
    
    		-- Sections
    		DELETE S
    		FROM dbo.rsSections S
    		WHERE S.SectionID IN (
    				SELECT SectionID
    				FROM #DelSections
    				);
    
    		COMMIT TRANSACTION
    	END TRY
    
    	BEGIN CATCH
    		DECLARE @ErrorSeverity INT
    			,@ErrorNumber INT
    			,@ErrorState INT
    			,@ErrorLine INT
    			,@ErrorProc NVARCHAR(200)
    
    		-- Grab error information from SQL functions
    		SET @ErrorSeverity = ERROR_SEVERITY()
    		SET @ErrorNumber = ERROR_NUMBER()
    		SET @ErrorMessage = ERROR_MESSAGE()
    		SET @ErrorState = ERROR_STATE()
    		SET @ErrorLine = ERROR_LINE()
    		SET @ErrorProc = ERROR_PROCEDURE()
    		SET @ErrorMessage = 'Problem deleting Section ' + CAST(@SectionID AS VARCHAR(10)) + CHAR(13) + 'SQL Server Error Message is: ' + CAST(@ErrorNumber AS VARCHAR(10)) + ' in procedure: ' + @ErrorProc + ' Line: ' + CAST(@ErrorLine AS VARCHAR(10)) + ' Error text: ' + @ErrorMessage
    
    		-- Not all errors generate an error state, to set to 1 if it's zero
    		IF @ErrorState = 0
    			SET @ErrorState = 1
    
    		-- If the error renders the transaction as uncommittable or we have open transactions, we may want to rollback
    		IF @@TRANCOUNT > 0
    		BEGIN
    			ROLLBACK TRANSACTION
    		END
    
    		RAISERROR (
    				@ErrorMessage
    				,@ErrorSeverity
    				,@ErrorState
    				,@ErrorNumber
    				)
    	END CATCH
    
    	RETURN @@ERROR;
    END
    GO


    For every expert, there is an equal and opposite expert. - Becker's Law


    My blog


    My TechNet articles

    Tuesday, November 05, 2013 6:20 PM
  • Yes.  Although you should always call RAISERROR before returning a non-zero value.  Only TSQL code that is specifically checking the return code will see it.  It creates a risk that current or future callers of the stored procedure might call the stored procedure without checking the return code.

    For instance client code in .NET or whatever almost never binds returncode parameters and checks the return code of stored procedures.

    So all stored procedures should always RAISERROR or THROW on error, in addition to returning a non-zero return value.

    Also delete

    IF OBJECT_ID(N'tempdb..#DelSections', N'U') IS NOT NULL
    			DROP TABLE #DelSections;
    

    If a session-scope temp table or procedure-scoped temp table in a calling procedure exists, you don't want to drop it.  When you create the temp table in your procedure, it will not conflict. 

    But better form would be to use a table variable when you need a temporary storage location that is purely local.

    David


    David http://blogs.msdn.com/b/dbrowne/


    Tuesday, November 05, 2013 7:09 PM
  • Interesting, I didn't know that creating a temp table with the same name as exists in the caller procedure, will not conflict. That's a bit hard to believe.

    Basically, there should be only two scenarios of calling DeleteSection procedure - direct call from the application with a single ID parameter or a call from the DeleteLevel procedure. In that case the #DelSections table should exist and the Id should be NULL (I will call the SP without parameters).

    So, I am not sure about the local table variable. Are you suggesting to re-select into table variable and in this case we don't create primary key on the temp table but rather on the table variable?

    E.g.

    declare @Sections table (SectionID int primary key)
    if @SectionID is not null   
        insert into @Sections (SectionID) select SectionID from dbo.rsSections where SectionID = @SectionID
    else   
        insert into @Sections (SectionID) select SectionID from #DelSections

    How to make sure that PK is clustered and does it matter?

    Thanks again.


    For every expert, there is an equal and opposite expert. - Becker's Law


    My blog


    My TechNet articles

    Tuesday, November 05, 2013 8:08 PM
    • I didn't know that creating a temp table with the same name as exists in the caller procedure, will not conflict. That's a bit hard to believe.

    Yep.  It's kind-of weird behavior, and not well known. 

    Here's the doc:

    Nested stored procedures can also create temporary tables with the same name as a temporary table that was created by the stored procedure that called it. However, for modifications to resolve to the table that was created in the nested procedure, the table must have the same structure, with the same column names, as the table created in the calling procedure.

    CREATE TABLE (Transact-SQL)

    That's why you're probably better off with a table variable @DelBlocks instead of #DelBlocks: just to make it obvious that this you are creating a purely local table.

    • Are you suggesting to re-select into table variable and in this case we don't create primary key on the temp table but rather on the table variable?

    No.  Just suggesting replacing the other temp table #DelBlocks with a table variable.  That way it's obvious what's happening and you don't need to worry about whether it already exists.

    David


    David http://blogs.msdn.com/b/dbrowne/


    Tuesday, November 05, 2013 8:59 PM
  • I see, I understood it a bit differently and already converted to @DelSections in that procedure. I am thinking that I should call delete block also as a procedure (I do have a separate procedure to delete block). So, my DeleteSection will be not the lowest step, the DeleteBlock will be.

    Looks like a lot of nested steps...


    For every expert, there is an equal and opposite expert. - Becker's Law


    My blog


    My TechNet articles

    Tuesday, November 05, 2013 9:24 PM