Replacement for sp_MSforeachdb

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:

• Using cursors

• That the error raising may not iterate through stored procedures or when executed via SQL agent

• 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 //////
///////////////////////////////////////////////

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:

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:

• RAISERROR

• ROLLBACK the transaction

• sp_send_dbmail if you want to be notified by email if the procedure fails.

• Log the errors in a file or table

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 use

StackExchange Code Review Q#117386, answer score: 9