Opinion on My first SP 1

[tis9700]

Good morning Gurus!
I wrote my first stored procedured and I'd like your expert opinion on it. Just in case I need to do any fine tuning. It is a combination of insert and update. At 24 months, if an Instructor has participated in teaching a class either as a primary Instructor or an Assistant, a new certification record is inserted with a certification status = 1 then the previous certification is deactivated by updating certification status = 2.

The ExportHspAutoRecerts is a view based on class count for the Instructor. It detects if they ever participated in a class between their last certification date and getdate().

This stored procedure seems to to work great and I included it a DTS Package that exports a text file for printing new cards and exports a report that lists all Instructors who have been recertified. I plan to schedule a DTS run to automate it for end of month. Thanks!!


SET QUOTED_IDENTIFIER OFF
GO
SET ANSI_NULLS ON
GO

/*
Creates a new Hunter Safety Program certification. The new record is created at then end of month if
Instructor has participated in teaching a class within 24 months of their last active certification date.

*/

CREATE PROCEDURE [usp_HspAutoRecertification]
AS
SET NOCOUNT ON

DECLARE @Error INT
DECLARE @RowCount INT

IF NOT EXISTS (SELECT * FROM ExportHspAutoRecerts)
SELECT @Error = @@ERROR, @RowCount = @@ROWCOUNT

IF @Error > 0 OR @RowCount = 0
BEGIN
RAISERROR('No recertifications present.', 16, 1 )
END

ELSE
BEGIN
INSERT INTO [HS_PROG].[dbo].[Instructor_Certifications]
( [instID],
[certificationType],
[certificationDate],
[racfID] )
SELECT
ExportHspAutoRecerts.[instID],
ExportHspAutoRecerts.[certificationType],
ExportHspAutoRecerts.[currCertDate] AS certificationDate,
ExportHspAutoRecerts.[recertJob] AS racfID
FROM ExportHspAutoRecerts

END

--Deactivates previous new Hunter Safety Program certification.

UPDATE [HS_PROG].[dbo].[Instructor_Certifications]
SET certificationStatus = 2
FROM Instructor_Certifications IC INNER JOIN ExportHspAutoRecerts ER
ON IC.certificationID = ER.certificationID

SELECT @Error = @@ERROR, @RowCount = @@ROWCOUNT

IF @Error > 0 OR @RowCount <> 1
BEGIN
RAISERROR('Certification was not deactivated.', 16, 1 )
END

IF @Error > 0 or @RowCount <>1
RETURN 1 --Error
ELSE
RETURN 0 --Success



GO
SET QUOTED_IDENTIFIER OFF
GO
SET ANSI_NULLS ON
GO

 

[nice95gle]

Looks good but I would place this within a transaction statement, so if something goes wrong you can rollback.

begin trans
     do something
     if an error happens
rollback trans
   else
commit trans

do some reading on automicity.

good luck



Well Done is better than well said
- Ben Franklin

 

[tis9700]

Hey nice95gle,
Thanks for the advice! Would it be better to change the order of the statement if I'm going to use transactions?

Put ...

IF @Error > 0 OR @RowCount = 0
BEGIN
RAISERROR('No recertifications present.', 16, 1 )
END
After the Insert statement instead of before.

Thanks

 

[SQLSister]

I'd be concerned about the order between the update and the insert. It looks to me as if you insert it and then immediatly change the status on the record you just inserted as well an any other records. Normally if I am going to do an update and an insert I do the update first.

Also since I don't see certificationid as one of the fields you insert, how would you know they would match up correctly in the join?

"NOTHING is more important in a database than integrity." ESquared

 

[Crowley16]

Normally if I am going to do an update and an insert I do the update first.

is there any particular reason for that?
I've been doing them in whatever seems most logical at the time...

--------------------
Procrastinate Now!

 

[SQLSister]

Because most of the time if you do the update second, you may update the records you just finished inserting which is usually a bad idea - either you change something you didn't want changed (which this script looks to do) or you update records to the values they already have (unless your update specifies where the values aren't equal which is usually only done when updating only one field and often not then). The second possibility doesn't hurt data integrity but it is unnecessary load on the server. If you make sure you exclude the records you just inserted from the update, you can do them in any order but human nature being what is it, I rarely see code written that way.

"NOTHING is more important in a database than integrity." ESquared

 

[tis9700]

Hi SQLSister,
I understand what you're talking about.

The certificationID is within my view (ExportHspAutoRecerts)

This view returns the current active certification, status = 1 of the Instructor in addition to name, address ssn, etc. I'm using that information to produce cards but I'm only using 4 of those fields, instID, certificationType,certificationDate and racfID to insert a new record.

As for the order of the statements, if I update first and set the certification status = 2 then my view wouldn't show any records.

 

[tis9700]

Hi SQLSister,
Actually after inserting a new certification based on records in the view ExportHspAutoRecerts, I update the record that was in the view based on a join between the Instructor_Certification.certificationID and ExportHspAutoRecerts.certificationID.

So I create a new certification record then update only the previous record, not all of the previous records.

 

[nice95gle]

If needed you can continue the dialog with SQLSister about the order of the transactions. You would need to do something like to following to make sure that if something fails along the way to can undo everthing prior to the point of the error. You did not mention if you are using 2000 or 2005 so this will work in both versions. But you did mention DTS so I will assume 2000. For 2005 you will want to check out the TRY..CATCH statements for error handling.

DECLARE @Error INT
DECLARE @RowCount INT

    IF NOT EXISTS (SELECT * FROM ExportHspAutoRecerts)
    SELECT @Error = @@ERROR, @RowCount = @@ROWCOUNT

    IF @Error > 0 OR @RowCount = 0 
        BEGIN 
            	RAISERROR('No recertifications present.', 16, 1 )
		Return 1--Stop execution of code if error occurs
        END
         

    BEGIN TRAN
    		INSERT INTO [HS_PROG].[dbo].[Instructor_Certifications] 
	     	( [instID],
	     	[certificationType],
	     	[certificationDate],
	     	[racfID] ) 
    SELECT 
		ExportHspAutoRecerts.[instID],
	        ExportHspAutoRecerts.[certificationType],
        	ExportHspAutoRecerts.[currCertDate] AS certificationDate,
	        ExportHspAutoRecerts.[recertJob] AS racfID
    FROM ExportHspAutoRecerts 


	IF @@Error <> 0 OR @RowCount = 0 
        BEGIN 
        	RAISERROR('Your error message', 16, 1 )
		Rollback TRAN
		Return 1--Stop execution of code if error occurs
        END


 	UPDATE [HS_PROG].[dbo].[Instructor_Certifications] 
        	SET  certificationStatus  = 2
        	FROM Instructor_Certifications  IC  INNER JOIN ExportHspAutoRecerts ER
        	ON IC.certificationID = ER.certificationID

        --SELECT @Error = @@ERROR, @RowCount = @@ROWCOUNT

        IF @Error > 0 OR @RowCount <> 1 
            BEGIN 
        	RAISERROR('Your error message', 16, 1 )
		Rollback TRAN
		Return 1--Stop execution of code if error occurs
            END
COMMIT TRAN
RETURN 0 --Success

Well Done is better than well said
- Ben Franklin

 

[nice95gle]

correction

DECLARE @Error INT
DECLARE @RowCount INT

    IF NOT EXISTS (SELECT * FROM ExportHspAutoRecerts)
     --SELECT @Error = @@ERROR, @RowCount = @@ROWCOUNT

    IF @@Error > 0 OR @@RowCount = 0 
        BEGIN 
                RAISERROR('No recertifications present.', 16, 1 )
        Return 1--Stop execution of code if error occurs
        END
         

    BEGIN TRAN
            INSERT INTO [HS_PROG].[dbo].[Instructor_Certifications] 
             ( [instID],
             [certificationType],
             [certificationDate],
             [racfID] ) 
    SELECT 
        ExportHspAutoRecerts.[instID],
            ExportHspAutoRecerts.[certificationType],
            ExportHspAutoRecerts.[currCertDate] AS certificationDate,
            ExportHspAutoRecerts.[recertJob] AS racfID
    FROM ExportHspAutoRecerts 


    IF @@Error <> 0 OR @@RowCount = 0 
        BEGIN 
            RAISERROR('Your error message', 16, 1 )
        Rollback TRAN
        Return 1--Stop execution of code if error occurs
        END


     UPDATE [HS_PROG].[dbo].[Instructor_Certifications] 
            SET  certificationStatus  = 2
            FROM Instructor_Certifications  IC  INNER JOIN ExportHspAutoRecerts ER
            ON IC.certificationID = ER.certificationID

        --SELECT @Error = @@ERROR, @RowCount = @@ROWCOUNT

        IF @@Error > 0 OR @@RowCount <> 1 
            BEGIN 
            RAISERROR('Your error message', 16, 1 )
        Rollback TRAN
        Return 1--Stop execution of code if error occurs
            END
COMMIT TRAN
RETURN 0 --Success

Well Done is better than well said
- Ben Franklin

 

[tis9700]

Hey nice95gle,
That's what I'm working on now. I was taking your advice but I made the mistake of doing two separate transactions. Thanks for the posting, I'll try it and let you know how it works.

 

[tis9700]

Hey nice95gle,
Thanks for the help. Works perfect! I've also decided to revise my error handling to @@ERROR instead of declaring variable for @Error and @rowcount.I think it might be overkill. Your thought?


Everyone is so great. Lot's to think about and I gotten so many responses.

 

[tis9700]

OK! Here's my sp revised.

SET QUOTED_IDENTIFIER OFF
GO
SET ANSI_NULLS ON
GO


/*
Creates a new Hunter Safety Program certification. The new record is created at the end of month when
Instructor has participated in teaching a class within 24 months of their last active certification date.
*/

ALTER PROCEDURE [usp_HspAutoRecertification]
AS

SET NOCOUNT ON

--Check end of month for Instructors eligible for 24 month recertification
IF NOT EXISTS (SELECT * FROM ExportHspAutoRecerts)

IF @@ERROR <> 0 OR @@ROWCOUNT = 0
BEGIN
RAISERROR('No recertifications present.', 16, 1 )
RETURN (1) ---Error
END
--Insert new active certification record for eligible Instructors
BEGIN TRAN
INSERT INTO [HS_PROG].[dbo].[Instructor_Certifications]
( [instID],
[certificationType],
[certificationDate],
[racfID] )
SELECT
ExportHspAutoRecerts.[instID],
ExportHspAutoRecerts.[certificationType],
ExportHspAutoRecerts.[currCertDate] AS certificationDate,
ExportHspAutoRecerts.[recertJob] AS racfID
FROM ExportHspAutoRecerts
--If error occurs rollback transaction
IF (@@ERROR <> 0)
BEGIN
RAISERROR('Recertification Failed',16,1)
ROLLBACK TRAN
RETURN(1) ---Error
END


--Update certificationStatus of eligible Instructors previous certification to deactivated
--based on join on certificationID
UPDATE [HS_PROG].[dbo].[Instructor_Certifications]
SET certificationStatus = 2
FROM Instructor_Certifications IC INNER JOIN ExportHspAutoRecerts ER
ON IC.certificationID = ER.certificationID
--If error occurs rollback transaction
IF (@@ERROR <> 0)
BEGIN
RAISERROR('Certification was not deactivated.', 16, 1 )
ROLLBACK TRAN
RETURN (1) ---Error
END


COMMIT TRAN
RETURN (0)---Success




GO
SET QUOTED_IDENTIFIER OFF
GO
SET ANSI_NULLS ON
GO

Very Cool!
Thanks everyone.

 

[nice95gle]

I'm not too sure. Run an execution plan using both ways and see which one works better.

Well Done is better than well said
- Ben Franklin

 

[tis9700]

Hi nice95gle,
What would I be looking for in the execution plan? The Query cost as relative to the batch?

 

[tis9700]

Hi nice95gle,
I ran the Execution Plan. Wow! Because of the underlying view being used, it's a big plan. I haven't tested it with a real world sample of records, probably around 100 to 200 a month. We only have about 2500 Instructors and not all will reach the 24 month eligbilty within this year. As for testing it with a small sample, 5 records, the cost on all the actions appears to be very low to nothing.

 

[nice95gle]

If it not causing a problem, I wouldn't put to much effort into it. To be honest I always just use @@Rowcount without a variable. I figure it would be quicker than, declaring a variable, setting it to something then retrieving that info.

Well Done is better than well said
- Ben Franklin