Our template has evolved over the years, but we are still checking @@ERROR rather than using TRY / CATCH. I ought to update it to something more modern, which takes advantage of all the best error checking etc. and, perhaps?, in the process reduces development time and/or bugs.
Anyone got a pointer to recommended article / blog etc. with a killer, best-of-breed, template?
Or fire-away and post your own?
Thanks
Look here and see if it helps. For a (SSMS) template however try catch (throw) and explicit transactions may not be a best practice for procedures that select data however for procedures that change data they would be.
Here's a template for that:
USE <Database Name,,>
GO
IF Object_Id(N'<Schema name,,>.<Procedure name,,>', 'P') IS NOT NULL DROP PROCEDURE <Schema name,,>.<Procedure name,,>;
GO
/* RCIT Database Support
By: Joe Torre
On: <Creation date,,>
Description:
<Description,,>
*/
CREATE PROCEDURE <Schema name,,>.<Procedure name,,>
AS
SET NOCOUNT ON;
BEGIN TRY
BEGIN TRAN
--DML
COMMIT
END TRY
BEGIN CATCH
ROLLBACK TRAN
INSERT ErrLog (ErrMessage, ErrNumber, ErrState, ErrSeverity, ErrProcedure, ErrLine)
VALUES(ERROR_MESSAGE(), ERROR_NUMBER(), ERROR_STATE(), ERROR_SEVERITY(), ERROR_PROCEDURE(), ERROR_LINE());
THROW;
END CATCH
--finally code
GOThanks Joseph.
Some thoughts for debate!
Yup, I have that in my SProc template, but also some others:
SET NOCOUNT ON
SET XACT_ABORT ON
SET ARITHABORT ON
but Googling for this yesterday I found
SET ANSI_NULLS ON
SET QUOTED_IDENTIFIER ON
SET ANSI_NULLS ON
SET ANSI_PADDING ON
SET ANSI_WARNINGS ON
and this:
BEGIN
SET NOCOUNT ON;
SET XACT_ABORT,
QUOTED_IDENTIFIER,
ANSI_NULLS,
ANSI_PADDING,
ANSI_WARNINGS,
ARITHABORT,
CONCAT_NULL_YIELDS_NULL ON;
SET NUMERIC_ROUNDABORT OFF;
I like the syntax of that (more obviously shows what is ON and what is OFF to me), but do I really need to set all those in every SProc? Maybe I have just been lucky, to date, that the environment my stuff is running in has never had any of those with wrong-state for my code, or my code doesn't have any elements that would be impacted one way or the other.
I only bother with a transaction if we are Updating (or Inserting / Deleting, Natch!). Is that OK? (we use Read Committed Snapshot)
But where I do I use a SAVE transaction, because if I just do
BEGIN TRANSACTION
... and then later on ...
ROLLBACK
then all bets are off for any Sproc that called this one, and its going to end in a non-recoverable error, which is not good from my APPs perspective. So I do:
ALTER PROCEDURE dbo.MySProc_Save
-- ... stuff ...
-- Save Section
BEGIN TRANSACTION MySProc_Save_01
SAVE TRANSACTION MySProc_Save_02
-- ... modify data ...
-- ... if necessary : SET @RetVal = 123 ... GOTO MySProc_Save_ABORT
MySProc_Save_ABORT:
IF @RetVal = 0 AND @ValidationErrCount = 0
BEGIN
COMMIT TRANSACTION MySProc_Save_01
END
ELSE
BEGIN
ROLLBACK TRANSACTION MySProc_Save_02
COMMIT TRANSACTION MySProc_Save_01
IF @IsDebug >= 1 SELECT [MySProc_Save]='DEBUG:ROLLBACK', [@RetVal]=@RetVal
GOTO MySProc_Save_EXIT
END
END
-- ... non-transaction block stuff ...
MySProc_Save_EXIT:
All fatal errors (e.g. runtime-syntax) will fall flat on their face, but all "data validation errors" and anything that @@ERROR can catch will be returned-to-caller (as a location-specific [i.e. within that SProc] Return-Value where 0=OK, and 1+=Error)
My Sprocs all have this (trailing) set of Parameters:
-- If the Sproc modifies data then these:
, @ValidationErrCount int = NULL OUTPUT -- Count of Validation Errors
, @ValidationMsg varchar(8000) = NULL OUTPUT -- Validation Message(s) - User Friendly
-- All Sprocs have all of these:
, @ErrorMsg varchar(8000) = NULL OUTPUT
, @ReturnVal int = NULL OUTPUT
, @IsSilent int=0 -- 1=Silent on all ERROR resultsets (only used if this SP can be called from another SP, rather than an ASP page where it is desirable to see errors during debugging
, @IsDebug int=0 -- 0=No debugging data, 1+ = debugging at increasing levels of details
Validation return values are soft. APP will display message to user and allow them to "try again".
@ErrorMsg / @ReturnVal >= 1 are Fatal in the APP (controlled abort of process, error logged for checking by Support, and so on)
If @IsSilent = 0 then the Sproc does a SELECT to output details of the error. This uses a special first-column-name which the APP will intercept (we have a central routine for "Get the next recordset" which handles that situtation, and avoids having to build error checking into every single use-case of a SQL SProc EXEC 
).
If an Sproc calls another child-SProc then it sets @IsSilent = 1, and it then becomes the parent SProc's responsibility to receive any error from the child-SProc and return it (if appropriate) to the caller
If @IsDebug >=1 then various SELECT statements will output data. This is used, Natch!, in debugging. If the logging records EXEC MySproc @PARAM1=123, @PARAM2='XYZ' I can copy & paste that into SSMS and tag on:
BEGIN TRANSACTION
EXEC MySproc @PARAM1=123, @PARAM2='XYZ'
, @IsDebug >= 1
ROLLBACK
to safely run the SProc with the original parameters, and hopefully! the DEBUG output will help be diagnose the problem. (Otherwise I can add more @IsDebug statements, which will then be available next time too
i use xact_abort if I don't need anything fancy in the (missing) CATCH
Thanks. I'll need to review that when implementing TRY/CATCH.
Its donkey's years since we added that, and I can't now remember the debate we had, but I think it was around an SProc calling a Child Sproc, that Sproc terminating with a fatal error, and whether the outer SProc would just carry on, or itself abort - the latter seemed important! and I think that's the situation we use XACT_ABORT for.
If I wasn't so blinking busy I'd build a test rig ... I'll do that as soon as I have time (or get fed up with the tedium of the current mini-project ...)
Because of requirements for certain index settings, the settings below (in alpha order) should be set in every proc (except SET NOCOUNT, of course, which could be left on for testing purposes only).
ANSI_NULLS and QUOTED_IDENTIFIER should not be set in the proc (they are meaningless there, but could easily confuse someone into believing they have meaning), but before the proc is created.
Those settings are stored as part of the proc definition.
SET ANSI_NULLS ON;
SET QUOTED_IDENTIFIER ON;
GO
CREATE PROCEDURE ...
AS
SET ANSI_PADDING ON;
SET ANSI_WARNINGS ON;
SET ARITHABORT ON;
SET CONCAT_NULL_YIELDS_NULL ON;
SET NOCOUNT ON;
SET NUMERIC_ROUNDABORT OFF;
SET XACT_ABORT ON;Thanks Scott.
I suppose, over the years, I've resisted doing this as overkill ... plus a big helping of laziness. Of course if I had put them in my TEMPLATE on Day One they'd be in everything now.
My best guess is that my code is the only code (no third party stuff that might interfere), no situation that would cause them to NOT be those values [when any/all of my SProcs are called], and therefore by serendipity I've never had a problem.
Is there any micro-second cost to having these things scattered everywhere in code? Seems to me it must have some cost, even if tiny. But that's only a legacy argument for me ... I started out in the 70s and every bit of every byte was precious ... Old Dog only able to perform Old Tricks ...
I have no idea if there's some minute bit of overhead. I know if the settings are not correct any modification to tables with certain types of indexes will fail. As a DBA, that's enough for me to know to always use the settings.
Yes, you are quite right Uncle Scott I just need to extract-digit and bung them into my template and be done with it.
By the by, I have an Sproc which LOGS that my SProc was created - [i.e. the script file was run]. It does a couple of things:
Log the Name of Sproc/Script and a 'Version' parameter value, Natch!
Check if a transaction is open - I would not normally be expecting to run a script to create an Sproc INSIDE a Transaction
And finally, check that various SETTINGS are as-intended. (and if not then output a script-snippet which will change them to my preferred defaults). So I do have a little "insurance", if not the actual ANSI_NULLS / QUOTED_IDENTIFIER settings in the script.
EXEC MyLoggingSproc 'MySprocName', '20171221' -- Date as pseudo version number
GO
CREATE PROCEDURE dbo.MySprocName ...
I'm a fan of using a pattern to CREATE a stub if it doesn't exist and then ALTER. That keeps the permissions intact.
And using SQL Server 2016 SP1 you can use a CREATE OR ALTER statement
Btw, I don't at all care for this approach:
SET XACT_ABORT,
QUOTED_IDENTIFIER,
ANSI_NULLS,
ANSI_PADDING,
ANSI_WARNINGS,
ARITHABORT,
CONCAT_NULL_YIELDS_NULL ON;
because if I scan for a specific setting, for example "ANSI_PADDING", and return the line it's on, I still won't know how it was set in that proc. We had a situation where some procs had the padding setting wrong and it was causing us issues.
Thanks Graz
This is the code for how I do that (I obviously want it to fail if the STUB itself winds up being called)
IF OBJECT_ID(N'[dbo].[XX_SP_MySproc]', N'P') IS NULL
EXEC
(
'CREATE PROC dbo.XX_SP_MySproc
AS RAISERROR(''XX_SP_MySproc:stub version, full version not found'', 10, 1)
WITH LOG, SETERROR
RETURN -1'
)
GO
I've thrown some line breaks in there to make it readable here, its all on one line in my actual code.
I set those at the bottom of the script [file], so they get set whenever the script [file] is run. But, yes, with ALTER if there is a Syntax Error the original version is still in place, which is handy if I'm running something on PRODUCTION and there is an unexpected hiccup ...
But for a release-script I prefer DROP / CREATE so that if there is an error (amongst all the miriad of Procedures, Triggers, etc. that I am building, then the absence of the object is better than an old, stale, and probably incompatible version left silently behind.
Here's my whole template for an Adhoc Sproc (I have other flavours of template for SAVE and FIND etc. jobs)
I've stripped out all the parameter declaration, and the meat.
In the template I have a block at the top with name of the Sproc (XX_SP_MySproc in this sample), the "main table", (XXX_TableName) the PKey column (xxx_ID), and so on. So I first do a Find & Replace on those (and then delete that block of pseudo-code) which gets me the basic framework. Using <ParamName> substitution would work fine too (just doesn't suit the way I work)
The script starts with a -- comment line because I concatenate multiple files into a single [e.g. release] script, and if there is no trailing new-line at the end of a file the opening -- which gets appended to the last line of the previous file is harmless.
--
PRINT 'Create procedure XX_SP_MySproc' -- formatting is mucked up, this for balance: '
GO
-- AdHocSproc template version 171209
EXEC dbo.KK_SP_LogScriptRun 'XX_SP_MySproc' ,'171221'
GO
-- DROP PROCEDURE dbo.XX_SP_MySproc
IF OBJECT_ID(N'[dbo].[XX_SP_MySproc]', N'P') IS NULL
EXEC ('CREATE PROC dbo.XX_SP_MySproc AS RAISERROR(''XX_SP_MySproc:stub version, full version not found'', 10, 1) WITH LOG, SETERROR RETURN -1')
GO
ALTER PROCEDURE dbo.XX_SP_MySproc
...
/* WITH ENCRYPTION */
AS
/*
* XX_SP_MySproc TODO
*
* Returns:
*
* TODO
*
* HISTORY:
*
* 21-Dec-2017 XXX Started
*/
SET NOCOUNT ON
SET XACT_ABORT ON
SET ARITHABORT ON
...
XX_SP_MySproc_EXIT:
SET NOCOUNT OFF
RETURN @intRetVal -- Return error number, 0=No error
/* TEST RIG
SELECT TOP 10 *
FROM dbo.XXX_TableName
WHERE 1=1
-- AND xxx_ID = 1234
--
ORDER BY xxx_zUpDt DESC
BEGIN TRANSACTION
EXEC dbo.XX_SP_MySproc
@xxx_ID = 1234
--
-- COMMIT
ROLLBACK
*/
--================== XX_SP_MySproc ==================--
GO
IF OBJECT_ID(N'[dbo].[XX_SP_MySproc]', N'P') IS NOT NULL
GRANT EXECUTE ON dbo.XX_SP_MySproc TO GenericPermissionsRoleName
GO
PRINT 'Create procedure XX_SP_MySproc DONE'
GO
Hadn't thought of that, I have been thinking that the multi-parameter-delimited-list and then a single SET looked neat, but I agree with you (and have in fact never done it that way) so I won't, now, change But I think I will group all ON and OFF items in two distinct groups (to further avoid mis-reading)
In case of any interest these are the settings that my Log SProc checks:
ansi_null_dflt_on, ON
ansi_nulls, ON
ansi_padding, ON
ansi_warnings, ON
arithabort, ON
concat_null_yields_null, ON
quoted_identifier, ON
ansi_null_dflt_off, OFF
arithignore, OFF
cursor_close_on_commit, OFF
disable_def_cnst_check, OFF
implicit_transactions, OFF
nocount, OFF
numeric_roundabort, OFF
xact_abort, OFF
its just to check that the "environment" in which the script, then, runs is "as expected", just in order to reduce the chance of something unexpected happening [if the environment settings had become non-standard for some reason]
Once again, do NOT set "ANSI_NULLS" and "QUOTED_IDENTIFIER" inside a proc. They have no effect there, but can fool people into thinking they do because the settings are present.
The above is the check made [by an Sproc that is called to Log the fact that the actual Sproc / Script was run] and it just complains about the environment that the Sproc is being run in.
Setting the various controls inside an Sproc is also done
The scripts, of this nature, which start with the Logging Sproc may well create indexes and views etc., so I want to be sure that I'm on a level playing field, regardless of whether all the settings may or may not be relevant.
I've found that way too many people spend way too many hours doing TRY/CATCH (and the older @@ERROR check) and then they just return what SQL Server would have done much better if there were no TRY/CATCH.
+1 totally agree
My APP does a bad job of reacting to errors it gets from SQL - the error could be "anything" so has to be capable of interpreting the error and deciding if it is terminal or not. So if the APP gets a SQL Error it treats it as fatal.
I also need to check @@ROWCOUNT at times (e.g. "only 1 row expected to be processed"). So on that basis also checking @@ERROR might be "reasonable" compared to TRY/CATCH
My code is littered with checks for @@ERROR <> 0, if they ever fire the logic is to terminate the Sproc and return an error code, which the APP will treat as fatal (as distinct from a Soft Error e.g. where Sproc has a logic statement for a data-validation test which then fails)
UPDATE MyTable
SET SomeColumn =1
WHERE SomeID = @TheID
SELECT @MyErrNo = @@ERROR, @MyRowCount = @@ROWCOUNT
IF @MyErrNo <> 0 OR @MyRowCount <> 1
BEGIN
SELECT @OutputErrNo = 1234 -- Unique "location ID" within the Sproc
, @OutputMessage =
CONCAT('Error #1234:Update of MyTable'
, ', Error=', @MyErrNo
, ', Rows=', @MyRowCount
)
GOTO SprocExit
END
I never test that those @@ERROR statements work (i.e. by deliberately testing with @parameters that force each error), in fact I'm not sure what errors could arise that are not SQL-fatal, and would return a non-zero @@ERROR, in the first place. But I would write less code, and have more chance of a generic "catch all" using TRY/CATCH rather than @@ERROR test on every statement - for sure not EVERY statement has an @@ERROR check
My @@ERROR test to "just return an ErrorNo" is just boilerplate copy & paste, so probably will be bug-free ... it also logs / returns an ErrorNo unique for every such test so if some error-checking code "fires" then I have an ErrNo which uniquely identifies WHERE in the code the error had been raised. I don't suppose I would have that luxury with TRY/CATCH. (Yeah, "Line Number", but that presupposes that I have ready access to the source of the deployed version - will I even know, for a specific SProc, what that is? SProcs usually deployed with ENCRYPTION to stop well-meaning! people from meddling with them ... so sp_HelpText would be sp_NoHelp!)
Currently APP treats any SQL Error as fatal; in practice I achieve that by having four OUTPUT return @parameters: @ErrMsg and @ErrNo which is fatal if <> 0; also non-fatal: @ValidationCount which is a count of the number of recoverable errors - not sure this has any use other than Zero / Non-Zero test, and a @ValidationMessage which is one/many concatenated messages intended to be useful to the operator to fix the problem (and try again).
I did a Google and found a simple example which used the following code
CREATE TABLE #StudentDetails
(
MyRoll int NOT NULL
, MyName varchar(100) NOT NULL
)
Test #1a:
DECLARE @intErrNo int, @intRowCount int
PRINT 'Test #1a'
INSERT INTO #StudentDetails (MyRoll, MyName)
VALUES ('K', 'Kristen') -- Error : MyRoll is NOT numeric
-- SQL block terminated here
SELECT @intErrNo = @@ERROR, @intRowCount = @@ROWCOUNT
SELECT [TestName]='Test #1a', [@intErrNo]=@intErrNo, [@intRowCount]=@intRowCount
GO
For me the SQL terminates with the INSERT statement. If I then manually type SELECT @@ERROR I do indeed get the Error No 245.
I don't know of any setting?? that will allow this to fail (in an SProc) without a fatal error, so in practice, this error would not be trapped.
Whereas the Try/Catch Test #2:
PRINT 'Test #2a'
BEGIN TRY
INSERT INTO #StudentDetails (MyRoll, MyName)
VALUES ('K', 'Kristen') -- Error : MyRoll is NOT numeric
-- SQL THROWS a CATCH here
END TRY
BEGIN CATCH
SELECT [TestName]='Test #2a', [ErrorNumber]=ERROR_NUMBER(), [ErrorSeverity]=ERROR_SEVERITY(), [ErrorState]=ERROR_STATE(), [ErrorProcedure]=ERROR_PROCEDURE(), [ErrorLine]=ERROR_LINE(), [ErrorMessage]=ERROR_MESSAGE()
END CATCH
GO
definitely catches that error.
So if I change from @@ERROR to TRY/CATCH a whole load of previously fatal errors will become "caught" ... and then I'm right back to @JeffModen point of allowing the original SQL error to be returned to the APP as the better solution - whilst my APP will abort at that point, the APP will at least have logged the original SQL Error message, and that's all it used to do.
I also tried a non-fatal @@ERROR test (trying to store NULL in a NOT NULL column)
SELECT [TestName]='Test #1a', [@intErrNo]=@intErrNo, [@intRowCount]=@intRowCount
GO
DECLARE @intErrNo int, @intRowCount int
PRINT 'Test #1b'
INSERT INTO #StudentDetails (MyRoll, MyName)
VALUES (NULL, 'Kristen') -- Error : MyRoll cannot be NULL
-- SQL error, but block not terminated
SELECT @intErrNo = @@ERROR, @intRowCount = @@ROWCOUNT
SELECT [TestName]='Test #1b', [@intErrNo]=@intErrNo, [@intRowCount]=@intRowCount
GO
this also issues a SQL Level=16 error, but continues. But my APP is going to abort as soon as it sees that SQL error, it won't be interested in the subsequent result set(s).
The ErrorState is different in each case, maybe that's the salient info?
Same Test2b as TRY/CATCH:
PRINT 'Test #2b'
BEGIN TRY
INSERT INTO #StudentDetails (MyRoll, MyName)
VALUES (NULL, 'Kristen') -- Error : MyRoll cannot be NULL
-- SQL THROWS a CATCH here
END TRY
BEGIN CATCH
SELECT [TestName]='Test #2b', [ErrorNumber]=ERROR_NUMBER(), [ErrorSeverity]=ERROR_SEVERITY(), [ErrorState]=ERROR_STATE(), [ErrorProcedure]=ERROR_PROCEDURE(), [ErrorLine]=ERROR_LINE(), [ErrorMessage]=ERROR_MESSAGE()
END CATCH
GO