Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

Having been informed that sp_MSforeachdb is undocumented and unsupported, I have been working on creating a replacement stored procedure based off the work done here.

Is it ok as it is? I'm concerned about:

  1. Using cursors
  2. That the error raising may not iterate through stored procedures or when executed via SQL agent
  3. Lack of user permissions

Ideally I'd like the only permissions to be required would be execute on the db this procedure resides in and read permission. Is it possible to capture the reason that the try catch failed and present that back in the error message?

Create Procedure [dbo].[ExecForEachDB] ( @cmd NVarchar(max) )
As /*
//////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
///// Stored Procedure created by Chris Johnson///////////////////////////////////////////////////////////////////////////////////////////////
///// 20th January 2016///////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
//////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
///// The purpose of this stored procedure is to replace the undocumented procedure sp_MSforeachdb as this may be removed in future versions//
///// of SQL Server. The stored procedure iterates through all user databases and executes the code passed to it//////////////////////////////
//////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
//////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
///// Based off of http://sqlblog.com/blogs/aaron_bertrand/archive/2010/02/08/bad-habits-to-kick-relying-on-undocumented-behavior.aspx  //////
//////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
///// Changes made////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
//////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
///// Date////////////Name//////////////////////Description///////////////////////////////////////////////////////////////////////////////////
----------------------------------------------------------------------------------------------------------------------------------------------
///// 20/JAN/2016/////Chris Johnson           //Initial version created - stripped back variables, added error handling, testing         /////
/////            /////                        //validity and logging tables                                                              /////
/////------------/////------------------------//-----------------------------------------------------------------------------------------/////
///// DD/MMM/YYYY/////                        //                                                                                         /////
/////------------/////------------------------//-----------------------------------------------------------------------------------------/////
///// DD/MMM/YYYY/////                        //                                                                                         /////
/////------------/////------------------------//-----------------------------------------------------------------------------------------/////
///// DD/MMM/YYYY/////                        //                                                                                         /////
/////------------/////------------------------//-----------------------------------------------------------------------------------------/////
///// DD/MMM/YYYY/////                        //                                                                                         /////
/////------------/////------------------------//-----------------------------------------------------------------------------------------/////
///// DD/MMM/YYYY/////                        //                                                                                         /////
/////------------/////------------------------//-----------------------------------------------------------------------------------------/////
///// DD/MMM/YYYY/////                        //                                                                                         /////
/////------------/////------------------------//-----------------------------------------------------------------------------------------/////
///// DD/MMM/YYYY/////                        //                                                                                         /////
//////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
*/
    Begin
        Set NoCount On;

    --Try to create Logging table
        If Not Exists ( Select  *
                        From    [sys].[tables] [T]
                                Left Join [sys].[schemas] As [S] On [S].[schema_id] = [T].[schema_id]
                        Where   [S].[name] = 'dbo'
                                And [T].[name] = 'ExecForEachDBLogs' )
            Begin
                Begin Try
                    Create Table [dbo].[ExecForEachDBLogs]
                        (
                          [LogID] BigInt Identity(1 , 1)
                        , [LogTime] DateTime2 Default GetDate()
                        , [Cmd] NVarchar(2000)
                        );
                End Try
                Begin Catch
                    Print 'unable to create logging table';
                End Catch;
            End;

    --Add Logging details
        If Exists ( Select  *
                    From    [sys].[tables] [T]
                            Left Join [sys].[schemas] As [S] On [S].[schema_id] = [T].[schema_id]
                    Where   [S].[name] = 'dbo'
                            And [T].[name] = 'ExecForEachDBLogs' )
            Begin
                Begin Try
                    Insert  [dbo].[ExecForEachDBLogs]
                            ( [Cmd] )
                    Values  ( @cmd );
                End Try
                Begin Catch
                    Print 'unable to capture logging details';
                End Catch;
            End;

    --Declare variables, SqlScript is for 
        Declare @SqlScript NVarchar(Max)
          , @Database NVarchar(257)
          , @ErrorMessage NVarchar(500);

    --Test validity, all scripts should contain a "?" to be used in place of a db name
        If @cmd Not Like '%?%'
            Begin
                Set @ErrorMessage = 'ExecForEachDB failed, script does not contain the string "?" '
                    + @cmd;
                Raiserror (@ErrorMessage,13,1);
            End;

        If @cmd Like '%?%'
            Begin
    --Use Cursor to hold list of databases to execute against
                Declare [DbNames] Cursor Local Forward_Only Static Read_Only
                For
                    Select  QuoteName([name])
                    From    [sys].[databases]
                    Where   [state] = 0 --only online databases
                            And [is_read_only] = 0 --only databases that can be executed against
                            And [database_id] > 4 --only user databases
                    Order By [name];

                Open [DbNames];

                Fetch Next From [DbNames] Into @Database; --Get next database to execute against

                While @@fetch_status = 0 --when fetch is successful
                    Begin
                        Set @SqlScript = Replace(Replace(Replace(@cmd , '?' ,
                                                              @Database) ,
                                                         '[[' , '[') , ']]' ,
                                                 ']');--Adds the database name and in the case of [[]] 
                        --Print @SqlScript;
                        Begin Try --try to execute script
                            Exec(@SqlScript);
                        End Try
                        Begin Catch --if error happens against any db, raise a high level error advising the database and print the script
                            Set @ErrorMessage = 'Script failed against database '
                                + @Database;
                            Raiserror (@ErrorMessage,13,1);
                            Print @SqlScript;
                        End Catch;

                        Fetch Next From [DbNames] Into @Database;--Get next database to execute against
                    End;

                Close [DbNames];
                Deallocate [DbNames];
            End;
    End;
Go

/*
--Testing Script
This test is designed to generate error messages by using a table that may not exist in other databases

--Create script
Declare @Script NVarchar(2000);

--generate script that will pass on current database but fail on others
Select Top 1
        @Script = 'Use [?]; Insert [#Test]
        ( [DBName] ) Select [' + [C].[name] + '] from ' + [T].[name]
From    [sys].[tables] As [T]
        Left Join [sys].[schemas] As [S] On [S].[schema_id] = [T].[schema_id]
        Left Join [sys].[columns] As [C] On [C].[object_id] = [T].[object_id]
Where   [S].[name] = 'dbo'
        And [C].[system_type_id] = 167;

--test script is good
Print @Script

--Set @Script = 'use [?]; select db_name()'

--create table to capture results
Create Table [#Test] ( [DBName] Varchar(150) );



Exec [dbo].[ExecForEachDB] @cmd = @Script;

Select  *
From    [#Test] As [T];

Drop Table [#Test];

*/
share|improve this question
    
What do you mean limited to 2000 characters as script errors occur trying to execute scripts with more characters that's a bug in your code if you are encountering this. There is no such limitation in SQL Server. – Martin Smith Jan 22 at 9:26
    
@MartinSmith I've found that when passing dynamic SQL values into sp_MSforeachdb that occasionally the script is cut off - causing the code to fail. I'm confident it's not an issue with my code as this has happened a number of times and is always resolved by removing tabs, spaces and returns within the code to reduce the number of characters to less than 2000. – Chris J Jan 22 at 9:30
    
The limit is 2GB not 2,000 characters so you must be truncating it somewhere. stackoverflow.com/questions/12639948/… – Martin Smith Jan 22 at 9:32
1  
@MartinSmith good to know, that's a great answer - I think this is to do with needing to declare and set the variable initially Declare @SqlString varchar(max) = '' Set @SqlString = @SqlString+'Select * from test' instead of Declare @SqlString varchar(max) Set @SqlString = 'Select * from test' Cheers – Chris J Jan 22 at 9:38
    
May I suggest my set-based implementation: here? – Kittoes0124 Feb 18 at 1:17
up vote 8 down vote accepted

First off, please heed the advice from @Zak about Version Control System (VCS). That should be the first take-away from this.


Slashy comment block

You have 31 lines, 4462 characters of slashy documentation template. It seems like an eye-sore, at least to me, and distracts from the code and even the documentation itself because it is so dense.

With some color schemes it can be really irritating to read as well:

irritating

Given the look of your comment block, you could perhaps go for a style like Java code:

Create Procedure [dbo].[ExecForEachDB] ( @cmd NVarchar(2000) )--limited to 2000 characters as script errors occur trying to execute scripts with more characters
As /*
    * The purpose of this stored procedure is to replace the undocumented procedure sp_MSforeachdb as this may be removed in future versions
    * of SQL Server. The stored procedure iterates through all user databases and executes the code passed to it
    * Based off of http://sqlblog.com/blogs/aaron_bertrand/archive/2010/02/08/bad-habits-to-kick-relying-on-undocumented-behavior.aspx
    * Change history:
    * 20/JAN/2016 | Chris Johnson | Initial version created - stripped back variables, added error handling, testing validity and logging tables 
    */
    Begin

Preferably, it's also better to not have the change history as part of the script and instead keep that history in VCS instead, although I will admit that I have seen many SQL developers do this so that the change history is easily accessible when looking at the code. Just keep in mind though that it is a bit of a maintenance hassle to have to add change history to the script any time it needs changed (which is probably not that often, thankfully).


This check can be made more "lightly" (as in less likely to cause locking and performance issues) by using Select 1 instead of Select *, that way you're only checking that the row exists, without actually getting any data from the table. Select * in general shouldn't be used unless you actually need data from all the columns.

    If Not Exists ( Select  1
                    From    [sys].[tables] As [T] With(nolock)
                            Left Join [sys].[schemas] As [S] With(nolock) On [S].[schema_id] = [T].[schema_id]
                    Where   [S].[name] = 'dbo'
                            And [T].[name] = 'ExecForEachDBLogs' )

There is also another way to check for existence of database objects using the OBJECT_ID() function, like so:

IF OBJECT_ID('dbo.ExecForEachDBLogs') IS NULL
BEGIN --...

You can repeat this method throughout your script.


Cannot be false

In the spirit of querying lightly, I noticed you have a redundant call that will always be true*:

--Add Logging details
    If Exists ( Select  --...

Since you just got done checking whether the table exists, and creating it if it didn't, this IF EXISTS call is just redundant and could be removed,

* Except if another transaction went and dropped the table between the two calls, which is extremely unlikely.


Catch & print

You have several blocks like this one:

        Begin
            Begin Try
                Insert  [dbo].[ExecForEachDBLogs]
                        ( [Cmd] )
                Values  ( @cmd );
            End Try
            Begin Catch
                Print 'unable to capture logging details';
            End Catch;
        End;

While printing to the console can be good for writing and debugging code, it is rarely the desired outcome in production code. I think it would make more sense to do one or more of these:

Which one(s) to choose really depends on how your script is used. For instance, if it is used by an application then RAISERROR would help the exception float up the stack and the user of the application could be notified. If this is more of a maintenance script then perhaps just sending an email would be sufficient. (just be careful not to overuse sp_send_dbmail, as it can generate a lot of "noise" in your email -- I would only use it for things that are important, otherwise log the errors instead).


Small things

This comment is incomplete, it is also quite obvious you are declaring variables by the keyword DECLARE on the next line.

--Declare variables, SqlScript is for 
    Declare @SqlScript NVarchar(Max)

This comment is already obvious by the code:

Fetch Next From [DbNames] Into @Database; --Get next database to execute against

As is this one:

                    Begin Try --try to execute script
                        Exec(@SqlScript);
                    End Try

And multiple others. Comments should only be used when the code needs explanation. When there are too many comments it just becomes noise.


You have testing/debugging code commented out. This can be both good or bad. If you expect the debugging code to be used in the future, then make a note of that in your documentation at the top. And if you don't expect it will be needed, then just delete it.


It's generally a good idea to wrap your cursor logic within TRY / CATCH, that way if it fails during execution you can still catch the error and deallocate it. See this Stack Overflow post: T-SQL: A proper way to CLOSE/DEALLOCATE cursor in the update trigger

share|improve this answer
    
Thanks for the feedback, I've implemented a lot of these changes - including implementing source control – Chris J Jan 21 at 13:46
    

Two Words. Version. Control.

Even VBA has an addin for .git integration these days. Version control is awesome for the following reasons:

  • Complete, comprehensive record of changes to your codebase, complete with explanations and descriptions of each revision (so long as you provide them).

  • Ability to instantly rollback to any past revision point. Wrote some code 3 weeks ago and just realised that it completely breaks what you want to do? Just rollback the relevant parts to before the change.

  • No need to have wall-of-text comments which *won't* be consistently maintained, or up to date and simply add clutter.

  • No need to leave commented-out previous versions of your code in the code base, where all sorts of problems can arise.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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