Newbie T-SQL dynamic stored procedure -- how can I improve it?

I'm new to T-SQL; all my experience is in a completely different database environment (Openedge). I've learned enough to write the procedure below -- but also enough to know that I don't know enough!

This routine will have to go into a live environment soon, and it works, but I'm quite certain there are a number of c**k-ups and gotchas in it that I know nothing about.

The routine copies data from table A to table B, replacing the data in table B. The tables could be in any database. I plan to call this routine multiple times from another stored procedur开发者_JS百科e. Permissions aren't a problem: the routine will be run by the dba as a timed job.

Could I have your suggestions as to how to make it fit best-practice? To bullet-proof it?

ALTER PROCEDURE [dbo].[copyTable2Table]
    @sdb        varchar(30),
    @stable     varchar(30),
    @tdb        varchar(30),
    @ttable     varchar(30),
    @raiseerror bit = 1,
    @debug      bit = 0
as
begin
    set nocount on

    declare @source      varchar(65)
    declare @target      varchar(65)
    declare @dropstmt    varchar(100)
    declare @insstmt     varchar(100)
    declare @ErrMsg      nvarchar(4000)
    declare @ErrSeverity int

    set @source      = '[' + @sdb + '].[dbo].[' + @stable + ']'
    set @target      = '[' + @tdb + '].[dbo].[' + @ttable + ']'
    set @dropStmt    = 'drop table ' + @target
    set @insStmt     = 'select * into ' + @target + ' from ' + @source
    set @errMsg      = ''
    set @errSeverity = 0

    if @debug = 1
        print('Drop:' + @dropStmt + '  Insert:' + @insStmt)

    -- drop the target table, copy the source table to the target
    begin try
        begin transaction    
        exec(@dropStmt)
        exec(@insStmt)
        commit
    end try

    begin catch
        if @@trancount > 0
            rollback

        select @errMsg      = error_message(), 
               @errSeverity = error_severity()

    end catch

    -- update the log table
    insert into HHG_system.dbo.copyaudit
        (copytime, copyuser, source, target, errmsg, errseverity)
        values( getdate(), user_name(user_id()), @source, @target, @errMsg, @errSeverity)

    if @debug = 1
        print ( 'Message:' + @errMsg + '  Severity:' + convert(Char, @errSeverity) )

    -- handle errors, return value
    if @errMsg <> ''
    begin 
        if @raiseError = 1
            raiserror(@errMsg, @errSeverity, 1)

        return 1
    end

    return 0
END

Thanks!

I'm speaking from a Sybase perspective here (I'm not sure if you're using SQLServer or Sybase) but I suspect you'll find the same issues in either environment, so here goes...

• Firstly, I'd echo the comments made in earlier answers about the assumed dbo ownership of the tables.

• Then I'd check with your DBAs that this stored proc will be granted permissions to drop tables in any database other than tempdb. In my experience, DBAs hate this and rarely provide it as an option due to the potential for disaster.

• DDL operations like drop table are only allowed in a transaction if the database has been configured with the option sp_dboption my_database, "ddl in tran", true. Generally speaking, things done inside transactions involving DDL should be very short since they will lock up the frequently referenced system tables like sysobjects and in doing so, block the progress of other dataserver processes. Given that we've no way of knowing how much data needs to be copied, it could end up being a very long transaction which locks things up for everyone for a while. What's more, the DBAs will need to run that command on every database which contains tables that might contain a '@Target' table of this stored proc. If you were to use a transaction for the drop table it'd be a good idea to make it separate from any transaction handling the data insertion.

• While you can do drop table commands in a transaction if the ddl in tran option is set, it's not possible to do select * into inside a transaction. Since select * into is a combination of table creation with insert, it would implicitly lock up the database (possibly for a while if there's a lot of data) if it were executed in a transaction.

• If there are foreign key constraints on your @target table, you won't be able to just drop it without getting rid of the foreign key constraints first.

• If you've got an 'id' column which relies upon a numeric identity type (often used as an autonumber feature to generate values for surrogate primary keys), be aware that you won't be able to copy the values from the '@Source' table's 'id' column across to the '@Target' table's id column.

• I'd also check the size of your transaction log in any possible database which might hold a '@Target' table in relation to the size of any possible '@Source' table. Given that all the copying is being done in a single transaction, you may well find yourself copying a table so large that it blows out the transaction log in your prod dataserver, bringing all processes to a crashing halt. I've seen people using chunking to achieve this over particularly large tables, but then you end up needing to put your own checks into the code to make sure that you've actually captured a consistent snapshot of the table.

Just a thought - if this is being used to get snapshots, how about BCP? That could be used to dump out the contents of the table giving you the snapshot you're looking for. If you use the -c option you'd even get it in a human readable form.

All the best, Stuart

This line seems a bit dangerous:

set @dropStmt    = 'drop table ' + @target

What if the target table doesn't exist?

I'd try to safeguard that somehow - something like:

set @dropStmt = 
    'if object_id(' + @target + ') IS NOT NULL   DROP TABLE ' + @target

That way, only if the call to OBJECT_ID(tablename) doesn't return NULL (that means: table doesn't exist) and the table is guaranteed to exist, issue the DROP TABLE statement.

Firstly, replace all the code like

set @source = '[' + @sdb + '].[dbo].[' + @stable + ']'

with code like

set @source = QuoteName(@sdb) + '.[dbo].' + QuoteName(@stable)

Secondly, your procedure assumes all objects are owned by dbo - this may not be the case.

Thirdly, your variable names are too short at 30 characters - 128 is the length of sysname.

I find the whole process you wrote to be terribly dangerous. Even if this is running from the database and not by the user, dynamic SQL is a poor practice! In databases using this to be able to do this to any table anytime is dangerous and would out and out be forbidden in the databases I work with. It is way too easy to accidentally drop the wrong tables! Nor is it possible to correctly test all possible values that the sp could run with, so this could be buggy code as well and you won't know until it has been in production.

Further, in dropping and recreating with select into, you must not have indexes or feoriegn key constraints or the things you need to havefor performance and data integrity. BAD BAD IDEA in general (OK if these are just staging tables of some type but not for anything else).

This is a task for SSIS. We save our SSIS packages and commit them to Subversion just like everything else. We can do a diff on them (they are just XML files) and we can tell what is running on prod and what configuration we are using.

You should not drop and recreate tables unless they are relatively small. You should update existing records, delete records no longer needed, and only add new ones. If you havea million records and 27000 have changed, 10 have been deleted, and 3000 are new, why drop and insert all 1,000,000 records. It is wasteful of server resources, could cause locking and blocking issues, and could create issues if the users are looking at the tables at the time you run this and the data suddenly disappears and takes some minutes to come back. Users get cranky about that.