I rather drastically cut down the fiddle, but I think the intent shines through. [Def] is a [DefaultClassification] table, and [Cls] is a [Classification] table that has some old records that need to be added in. The DefaultClassification table will be used in the future to spawn new groups of records into yet another table, and we're "unbreaking" an existing bit of logic/data with this process.

I did my best to distill the entire thing down to the barest essentials, but I've got another similar process that runs right after this one, so I'm looking to learn best practices and why this query is broken.

When I wrote this I intended it to run in a 2008+ environment, scripted or manually in SSMS. I don't know that it won't be scripted, but it might. Right now it's entirely run by hand. The rollback and select at the top above the print messages at the end are because it doesn't work right, so this lets me validate it before committing. I would prefer a valid script that I don't have to muck with, however

The specific issue I have is on line 62 of that revision one link http://sqlfiddle.com/#!3/ef71e/1 and it looks like this:


if I flip that RIGHT to a LEFT it does the insert but doesn't detect duplicates (to prevent insertion) and if I do the RIGHT it detects duplicates but doesn't handle insertions.

What did I break? And Why?

And I'm told the use of the @@ERROR was bad juju, but I don't know why, so bonus points (a 100 rep bounty) for that explanation too.


Why is @@ERROR bad juju?

In SQL Server 2005, using @@ERROR makes life harder than it needs to be for detecting and trapping errors.

1) You must test after every statement. Just from a maintenance perspective, my dog the goggles will do nothing to protect you. You can also argue that all the copy/paste logic for error handling will obscure the "true" purpose of your code.

2) @@ERROR is too volatile. In the following code, line 2 throws error 8134. Line 3 I select the value and 8134 is consumed. Why? Because the SELECT statement didn't raise an error. Even if it had, it would have been a different error than the one that failed.

DECLARE @ErrorCode int
SELECT 1/0 AS div_zero
SELECT @@error AS [@@error]
SET @errorCode = @@error
SELECT @errorCode AS errorCode

3) Even if you handle the @@ERROR well with the above 2, my challenge around it is that error still "comes through" at least in SSMS. Run the first half of the query and even though I have logically handled the error, it will still percolate back to the caller.


DECLARE @ErrorCode int;
SELECT 1/0 AS div_zero;
SET @errorCode = @@error;
IF @ErrorCode <> 0
    -- I did something here to handle the error
    -- SSMS still reports that the Query completed with errors
    print 'developer electrocuted';

Contrast that with

-- error handled *and* query executes successfully
    -- This will report query executed successfully
     SELECT 1/0 AS handled_div_zero;
    ERROR_NUMBER()AS error_number --returns the number of the error.
,   ERROR_SEVERITY() AS error_severity --returns the severity.
,   ERROR_STATE()AS error_state  --returns the error state number.
,   ERROR_PROCEDURE() AS error_procedure --returns the name of the stored procedure or trigger where the error occurred.
,   ERROR_LINE() AS error_line --returns the line number inside the routine that caused the error.
,   ERROR_MESSAGE() AS error_message; --returns the complete text of the error message. The text includes the values supplied for any substitutable parameters, such as lengths, object names, or times.

SQL Server 2012 finishes what 2005 started and provides THROW This allows you to handle your error in your CATCH block and then instead of smashing your call stack, you are now able to percolate that same error back up to the caller. Otherwise, you're reduced to having to raiserror each time you try to fix a level in the call stack.

| improve this answer | |
  • In almost every case where I have seen people use @@error they have neglected to test after every indivudal piece of code and the person wonders why the errors are not stopping the proc. I refuse to pass a code review when it contains @@error. – HLGEM Mar 14 '13 at 21:01

Ok, I wrote an article on SQL Server Central here that may help you with your left/right outer join questions. However if you are trying to remove duplicates in this case I think what you are looking for is a NOT EXISTS.

    , tTCD.ParID
    , tTCD.Name
    , tTCD.IsExpense
    , tTCD.CD
FROM @Default tTCD
        SELECT 1 FROM [Def] tcd
            WHERE  tcd.ClsID = tTCD.ClsID
            AND tcd.Name = tTCD.Name
            AND tcd.IsExpense = tTCD.IsExpense)

Your existing code using a RIGHT OUTER JOIN has a WHERE clause on the LEFT table. Since you are excluding NULLs and on a RIGHT OUTER JOIN any time there is not a match you will get a NULL on the LEFT table. Using the NOT EXISTS should help more. However your test data doesn't actually have any duplicates so it's hard to tell.

| improve this answer | |
  • Yeah, that's a discussion we were having in the chatroom. There are only about a dozen dupes, but I can't have any of them when this process gets run. – jcolebrand Mar 14 '13 at 21:06

You cannot just swap right and left joins. They mean two different things. You also need to understand the effect of having a where condition on the table on the left side of a right join or the right side of a left join. See this link to understand why this changes teh join to an inner join. http://wiki.lessthandot.com/index.php/WHERE_conditions_on_a_LEFT_JOIN

See if this code is what you want:

        , tTCD.ParID
        , tTCD.Name
        , tTCD.IsExpense
        , tTCD.CD
   FROM  [Def] tcd
        ON  tcd.ClsID = tTCD.ClsID
        AND tcd.Name = tTCD.Name
        AND tcd.IsExpense = tTCD.IsExpense
        AND tTCD.IsExpense IS NOT NULL

For the future is is generally best to use only left joins.

| improve this answer | |

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.