Understanding how to properly create a stored procedure that only does INSERT from sourcetable to targettable with dynamic sql

Question

Background and use-case I need to create a stored procedure where the input parameters will be: @sourceTable, @targetTable, @compositeKey. I'm using available fields for the composite key, since I need to create myself what makes the row unique. Also, I want to use this procedure on other tables in other scenarios with the same purpose. I think I need to use dynamic-SQL since this is the only way that supports having table names as parameters (might be wrong in this).

My main issues are that I've read that you should be very careful with dynamic-sql. Also my error-handling is probably not best-practice. So that I would very much need some feedback on... Read somewhere that I could use SET XACT_ABORT ON and that would sort of take care of error-handling?

Goal

• Do INSERT based on composite-key from available fields. It should take the rows from the source table and if they do not exist in the target table, insert them. Otherwise do nothing.
• Print a row to a log table, specifying if the run was successful, the time of execute and number of rows affected.

Any help you guys give is helpful!

Outline of what I want to achieve

CREATE PROCEDURE [dbo].[usp_InsertByCompositeKey]
    @targetTable    NVARCHAR(255), 
    @sourceTable    NVARCHAR(255), 
    @compositeKey   NVARCHAR(MAX)

AS

BEGIN

    BEGIN TRY

        BEGIN TRANSACTION;

        --do dynamic-sql here.

        COMMIT;

    END TRY

    BEGIN CATCH

        ROLLBACK;

    END CATCH
END

Current code within the BEGIN TRANSACTION-block:

DECLARE @SqlQuery NVARCHAR(MAX);
        DECLARE @WhereClause NVARCHAR(MAX) = '';

        -- Split the compositeKey string into individual rows/values with a temporary table with the column "Field".
        DECLARE @CompositeKeyFields TABLE (Field NVARCHAR(255));
        INSERT INTO @CompositeKeyFields
        SELECT value FROM STRING_SPLIT(@compositeKey, '|');

        -- Construct the WHERE clause dynamically using the rows in column "Field" in @CompositeKeyFields. Iterate through the rows and build the WHERE clause.
        SELECT @WhereClause = @WhereClause + QUOTENAME(Field) + ' = S.' + QUOTENAME(Field) + ' AND '
        FROM @CompositeKeyFields;

        SET @WhereClause = LEFT(@WhereClause, LEN(@WhereClause) - LEN(' AND ')); -- We need to remove the trailing 'AND' from the iteration above.

        -- Construct the dynamic SQL query for INSERT.
        SET @SqlQuery =
            'INSERT INTO ' + @targetTable + ' ' +
            'SELECT * ' +
            'FROM ' + @sourceTable + ' AS S' +
            ' WHERE NOT EXISTS (' +
            '     SELECT 1 FROM ' + @targetTable + ' AS T' +
            '     WHERE ' + @WhereClause +
            ' );';

        -- Executes the dynamic SQL query
        EXEC sp_executesql @SqlQuery;

        -- Log how many rows that was inserted. Used for logging to AuditLog.
        SET @InsertedRowCount = @@ROWCOUNT;

        COMMIT;

        -- Log the result of the insert to an AuditLog table
        INSERT INTO [transform].[AuditLog] (LogMessage, LogDateTime, IsError, InsertedRowCount)
        VALUES ('sp_InsertByCompositeKey executed successfully for source table ' + @sourceTable + ' and target table ' + @targetTable + ' with the composite key: ' + @compositeKey, GETDATE(), 0, @InsertedRowCount);

Answer 1

documentation

CREATE PROCEDURE [dbo].[usp_InsertByCompositeKey]
    @targetTable    NVARCHAR(255), 
    @sourceTable    NVARCHAR(255), 
    @compositeKey   NVARCHAR(MAX)

This is inadequate. The parameter identifiers are lovely, and the pair of table names are very clear. But compositeKey? As written the identifier suggests we will see (a,b,c), or perhaps a,b,c. Caller should not have to read the code in order to call this. The a|b|c syntax came as a surprise.

The identifier suggests we will produce an INSERT side effect, good. The comments up top should also explain we produce a logging side effect, at least in the case where the transaction doesn't fail due to permission errors or whatever.

comments

Your comments for the most part are helpful. But consider writing shorter comments, and fewer of them. We put the WHY into English language comments; the HOW should be explained by the code.

adding conjuncts

        DECLARE @WhereClause NVARCHAR(MAX) = '';

This works, but it is inconvenient, as evidenced by that kludgey LEN(' AND ') expression. On the plus side we do have a syntactically valid clause at this point.

        SELECT @WhereClause = @WhereClause +  ...

This is slightly painful because as we build it up we do not have a syntactically valid clause -- we'll need to fix it up at the end.

Prefer to initialize the where clause as 'TRUE'. Then tacking on ' AND this', ' AND that' conjuncts still leaves us with a valid clause. No need for a later cleanup.

log the truth

        VALUES ('sp_InsertByCompositeKey executed ...

No.

You told us that the name of this procedure is usp_InsertByCompositeKey.

algorithm

It's unclear what your query plan looks like. If the PK is guid, or (guid, timestamp), then we're doomed to wastefully tablescan both the source and destination tables. Or the moral equivalent, which is a full scan of covering index. If OTOH PK looks more like (timestamp, guid) then there is hope that the backend planner can perform work proportional to number of rows inserted, as opposed to number of rows that exist.

I need to pick up on all new rows always inserted into the sourcetable.

That is some valuable Review Context which did not appear in OP and doesn't show up in comments that introduce the stored procedure.

Give each table a created timestamp column, which is assigned upon initial INSERT and is never UPDATEd. In other words, we have an append-only log. Put an index on this column.

Then there's no need for paranoid code to try reading rows from the target table at all. Do a single (indexed!) SELECT MAX(created) FROM @targetTable. That tells us, roughly, the last time we ran. All that remains to do is

INSERT INTO @targetTable
SELECT *
FROM @sourceTable
WHERE created > @lastRun;

This produces an efficient query plan that is very simple.

You might finish up with a pair of COUNT(*) queries, to verify the two tables are indeed in sync. (It's racy -- hopefully the source table doesn't see a ton of INSERTs at the time the midnight cron job runs.)

In the case of moving a day's records with a fortnight's history, we're reading only 7% of the rows, meaning that 93% of the rows sit on disk untouched. They don't even need to be dragged into memory, evicting other rows as they pollute the row cache.