2
\$\begingroup\$

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);
\$\endgroup\$
3
  • 1
    \$\begingroup\$ "I need to create a stored procedure" No, you don't. Or at least OP doesn't offer sufficient problem context to reveal why stored procedure would in some way be "better" than having a DB client compose and send a new dynamic sql (INSERT) command each time. Stored procedures have upsides and some serious downsides, such as being harder to test and posing some challenges with keeping the code running in production in sync with the git repo. Recommend that you offer additional context about the business problem you're trying to solve, so we can see the set of tools that are available. \$\endgroup\$
    – J_H
    Mar 1 at 14:11
  • \$\begingroup\$ The sourcetable will always only contain 14 days of data. Each day it is truncated where max date gets erased, and the newest day gets added. So I need to pick up on all new rows always inserted into the sourcetable. Ideally, this procedure would be running once a day to pick up the new rows, and insert them into the targettable for storing the data. Im not sure what this means: "having a DB client compose and send a new dynamic sql (INSERT) command each time." Sorry, quite new to SQL. \$\endgroup\$
    – William
    Mar 1 at 14:26
  • 1
    \$\begingroup\$ A DB client is a program written in e.g. C# or Python which connects to a DB server. The client is perfectly capable of doing the same "manipulate a string and send it as a DB command" operations that this SP is doing. \$\endgroup\$
    – J_H
    Mar 1 at 14:33

1 Answer 1

1
\$\begingroup\$

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.

\$\endgroup\$
2
  • \$\begingroup\$ Really helpful tips J_H, thank you. I very much like the WHERE created > @lastRun idea. This is my fault for not giving you proper context, but I do not have permissions to edit the sourcetable. Also looking at the sourcetable now there is no telling if the rows actually are dropped completely and then replaced by new ones. Or if they add one date, remove another date. Since I do not have permissions, its difficult to test. I guess that is why I'm leaning towards dynamic sql. (which I get is more difficult than it sohuld be...) \$\endgroup\$
    – William
    Mar 1 at 15:09
  • \$\begingroup\$ You mentioned that the table content can be a bit dynamic, with rows potentially DELETEd. That is orthogonal to "dynamic sql", which in any event could be dynamically created by a sql client just as easily as it's created by a stored procedure. // The OP seemed to suggest an "append-only log" table, which is a pretty common convention. The idea that someone might DELETE a row, before or after a midnight run, is a little worrying. Prefer to "journal out" any errors, e.g. by recording "+3 widgets sold", followed by a "-3 widgets sold" row. Both will get vacuumed up by your procedure. \$\endgroup\$
    – J_H
    Mar 1 at 15:22

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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