Is there any way in which this code can be improved?
The procedure parameters should reflect the underlying data types. Each component of an identifier is sysname
, a synonym for nvarchar(128) NOT NULL
. Using varchar(50)
artificially limits the length, and characters that might be used. Likewise, Unicode should be used for the dynamic SQL itself.
The user-supplied table identifier strings should be split using PARSENAME
into schema and object portions, then quoted using QUOTENAME
. This is safer and more correct than manually surrounding the whole identifier with square brackets.
QUOTENAME
returns nvarchar(258)
, so a safe type for the input is twice this, plus one for the .
separator, giving nvarchar(517)
.
Add a @Debug
bit parameter, or similar, so the user can view the generated dynamic SQL (and the associated execution plan) before deciding to execute it.
The procedure should have a basic error handling framework, for example as described by Erland Sommarskog in Error and Transaction Handling in SQL Server.
Add some basic checks for things like object existence, the presence of a primary key, and so on.
STRING_AGG
should normally have a WITHIN GROUP
clause to provide deterministic ordering.
Compute the distinct column lists once, then reuse this value as necessary.
Separate the code into more steps to make it clearer, and easier to debug. Try to avoid code that scrolls off to the right, and deeply nested function calls.
It is difficult to anticipate all the conditions that could cause the MERGE
statement to fail, but consider adding extra checks for e.g. computed columns. Even if this is covered by another condition, being explicit makes the intent clearer.
Avoid hard-coding a numeric type id. Use the TYPE_ID
function instead.
Using CONCAT_WS
can result in cleaner code than CONCAT
when using separators.
Also, I am not sure how to make this work with compound keys...
This can be done by consulting sys.key_constraints
and sys.index_columns
, see the example code below.
Example
This illustrates how the above points might be incorporated:
CREATE OR ALTER PROCEDURE dbo.MergeTables
@Source nvarchar(517),
@Target nvarchar(517),
@Debug bit = 0
AS
BEGIN
SET NOCOUNT, XACT_ABORT ON;
BEGIN TRY
-- Cannot specify a database or server in the input table names
IF PARSENAME(@Source, 3) IS NOT NULL
BEGIN
RAISERROR(N'Three- or four-part names are not supported for the table identifer %s.', 16, 1, @Source);
END;
IF PARSENAME(@Target, 3) IS NOT NULL
BEGIN
RAISERROR(N'Three- or four-part names are not supported for the table identifer %s.', 16, 1, @Target);
END;
-- Break and quote input e.g. 'My Schema.My Object' - > [My Schema].[My Object]
SET @Source = CONCAT_WS(N'.',
QUOTENAME(PARSENAME(@Source, 2)),
QUOTENAME(PARSENAME(@Source, 1)));
SET @Target = CONCAT_WS(N'.',
QUOTENAME(PARSENAME(@Target, 2)),
QUOTENAME(PARSENAME(@Target, 1)));
-- Check tables are accessible
DECLARE @SourceID integer = OBJECT_ID(@Source, N'U');
DECLARE @TargetID integer = OBJECT_ID(@Target, N'U');
IF @SourceID IS NULL
BEGIN
RAISERROR(N'Table %s not found in the current database.', 16, 1, @Source);
END;
IF @TargetID IS NULL
BEGIN
RAISERROR(N'Table %s not found in the current database.', 16, 1, @Target);
END;
IF CONVERT(integer, OBJECTPROPERTYEX(@SourceID, 'TableHasPrimaryKey')) = 0
BEGIN
RAISERROR(N'Table %s does not have a primary key.', 16, 1, @Source);
END;
IF CONVERT(integer, OBJECTPROPERTYEX(@TargetID, 'TableHasPrimaryKey')) = 0
BEGIN
RAISERROR(N'Table %s does not have a primary key.', 16, 1, @Target);
END;
/* TODO: Check source and target schemas are compatible*/
DECLARE
-- Find the primary key columns for the MERGE statement's ON clause
@PrimaryKeyColumnList nvarchar(max) =
(
SELECT
STRING_AGG(
CONVERT(nvarchar(max),
CONCAT(N'S.', QUOTENAME(C.[name]), N'=T.', QUOTENAME(C.[name]))),
N' AND ')
WITHIN GROUP (ORDER BY IC.key_ordinal)
FROM sys.key_constraints AS KC
JOIN sys.index_columns AS IC
ON IC.[object_id] = KC.parent_object_id
AND IC.index_id = KC.unique_index_id
JOIN sys.columns AS C
ON C.[object_id] = IC.[object_id]
AND C.column_id = IC.column_id
WHERE
KC.parent_object_id = @SourceID
AND KC.[type_desc] = N'PRIMARY_KEY_CONSTRAINT'
),
@InsertClause nvarchar(max) = N'INSERT (column_list) VALUES (column_list)',
-- Find the insert columns list
@InsertColumnList nvarchar(max) =
(
SELECT
STRING_AGG(
CONVERT(nvarchar(max),
QUOTENAME(C.[name])),
N',')
WITHIN GROUP (ORDER BY C.column_id)
FROM sys.columns AS C
WHERE
C.[object_id] = @SourceID
AND C.is_computed = 0
AND C.is_identity = 0
AND C.is_column_set = 0
AND C.is_hidden = 0
AND C.generated_always_type = 0
AND C.system_type_id != TYPE_ID(N'timestamp')
),
-- Generate the UPDATE SET clause
@UpdateClause nvarchar(max) = N'UPDATE SET ' +
(
SELECT
STRING_AGG(
CONVERT(nvarchar(max),
CONCAT(N'T.', QUOTENAME(C.[name]), N'=S.', QUOTENAME(C.[name]))),
N',')
WITHIN GROUP (ORDER BY C.column_id)
FROM sys.columns AS C
WHERE
C.[object_id] = @SourceID
AND C.is_computed = 0
AND C.is_identity = 0
AND C.is_column_set = 0
AND C.is_hidden = 0
AND C.generated_always_type = 0
AND C.system_type_id != TYPE_ID(N'timestamp')
AND NOT EXISTS
(
-- Exclude column if it is part of the primary key
SELECT 1
FROM sys.key_constraints AS KC
JOIN sys.index_columns AS IC
ON IC.[object_id] = KC.parent_object_id
AND IC.index_id = KC.unique_index_id
WHERE
KC.parent_object_id = C.[object_id]
AND KC.[type_desc] = N'PRIMARY_KEY_CONSTRAINT'
AND IC.column_id = C.column_id
)
),
-- Basic form of the merge statement
@MergeStatement nvarchar(max) =
CONCAT_WS(
N' ',
N'MERGE @Target AS T',
N'USING @Source AS S',
N'ON primary_key_match_list',
N'WHEN MATCHED THEN merge_matched',
N'WHEN NOT MATCHED BY TARGET THEN merge_not_matched',
N'WHEN NOT MATCHED BY SOURCE THEN DELETE;');
-- Substitute the insert column list into the INSERT clause
SET @InsertClause = REPLACE(@InsertClause, N'column_list', @InsertColumnList);
-- Substitute other values into the MERGE template
SET @MergeStatement = REPLACE(@MergeStatement, N'@Target', @Target);
SET @MergeStatement = REPLACE(@MergeStatement, N'@Source', @Source);
SET @MergeStatement = REPLACE(@MergeStatement, N'primary_key_match_list', @PrimaryKeyColumnList);
SET @MergeStatement = REPLACE(@MergeStatement, N'merge_matched', @UpdateClause);
SET @MergeStatement = REPLACE(@MergeStatement, N'merge_not_matched', @InsertClause);
BEGIN TRANSACTION;
IF @Debug = 0
BEGIN
-- Peform the MERGE
EXECUTE (@MergeStatement);
END;
ELSE
BEGIN
PRINT @MergeStatement;
END;
COMMIT TRANSACTION;
RETURN 0;
END TRY
BEGIN CATCH
IF @@TRANCOUNT > 0 ROLLBACK TRANSACTION;
THROW;
RETURN -1;
END CATCH;
END;