12
\$\begingroup\$

Background: I'm trying to synchronize order information between Oracle and SQL-Server databases and allow users to be able to track any changes between what has been replicated via an ASP.NET web page. Users will be able to select which line items they want to be able to work on via this page and the remainder will be excluded going forward. Users will also have the option to change which items have been excluded at any time until the process has been finalized.

The initial load of this order information replicates (from Oracle to SQL-Server) all of the line items and presents them for assignment to the user. Any subsequent load of the order will compare replicated data to the original data. I'm using the following statement to handle the replication and reconciliation based on user selections:

CREATE TABLE #tempSpecOrderReplication 
( 
    Ord_L_ID INT, 
    Order_Number INT, 
    Ord_ID INT, 
    Item_Number VARCHAR(50), 
    Quantity DECIMAL(18, 2), 
    UOM VARCHAR(50), 
    Price DECIMAL(18, 2), 
    Is_Spec_Order BIT  
); 

INSERT INTO #tempSpecOrderReplication 
([Ord_L_ID], [Ord_ID], [Quantity], [UOM], [Price], 
 [Is_Spec_Order], [Item_Number], [Order_Number]) 
VALUES (...); --These values come from the .NET DataTable

MERGE Spec_Order_Replication WITH (HOLDLOCK) AS target 
USING (SELECT * FROM #tempSpecOrderReplication) AS source 
ON (target.Ord_L_ID = source.Ord_L_ID) 
WHEN MATCHED THEN 
    UPDATE SET Is_Spec_Order = source.Is_Spec_Order 
WHEN NOT MATCHED THEN 
    INSERT 
    (Ord_L_ID, Order_Number, Ord_ID, Item_Number, 
     Quantity, UOM, Price, Is_Spec_Order) 
    VALUES 
    (source.Ord_L_ID, source.Order_Number, source.Ord_ID, source.Item_Number, 
     source.Quantity, source.UOM, source.Price, source.Is_Spec_Order); 

DROP TABLE #tempSpecOrderReplication 

Questions:

  1. Is there any way I can improve the existing SQL statement?
  2. Is there a better way that I can go about this?

I'm using SQL-Server version 2008 R2.

Please ask if you need any further information.

\$\endgroup\$
10
  • 2
    \$\begingroup\$ Are there any specific areas in which you would want it to be better? For example, are you concerned about performance, fault-tolerance, something else, or all of the above? Also, is this a one-way replication and how frequently is this query run? \$\endgroup\$
    – xDaevax
    Oct 20, 2014 at 14:59
  • \$\begingroup\$ @xDaevax I've read a couple articles about the MERGE statement not necessarily being an atomic statement causing some issues with race conditions. This is a one-way replication, only going from my Oracle instance to my SQL-Server instance. The order values are compared each time the user saves their work and this statement will be ran each time the user is forced to update the current replicated data. Hard to say exactly, but basically only when the original order data is changed. \$\endgroup\$ Oct 20, 2014 at 15:04
  • \$\begingroup\$ So, your ASP.NET code is bound to Oracle, then replicated to SQL Server, correct? \$\endgroup\$
    – xDaevax
    Oct 20, 2014 at 15:05
  • 1
    \$\begingroup\$ @xDaevax Correct, the ASP.NET DataTable referenced is bound to Oracle and being replicated to SQL-Server. \$\endgroup\$ Oct 20, 2014 at 15:07
  • \$\begingroup\$ sounds like you could use a Gridview to do this, like you could have the load be from one database and the insert and update be on the other database, and could have an insert button that inserts all records, etc. \$\endgroup\$
    – Malachi
    Oct 20, 2014 at 15:58

2 Answers 2

6
\$\begingroup\$

Well done.

I honestly cannot find anything bad to say about your SQL code. Your capitalization of keywords and indentation are consistent. Your query is properly explicit, there is no guessing work that the SQL engine would have to make. You clean up after your operation by dropping your #tempSpecOrderReplication.

The only thing that looked unusual, though not bad, is that you used square brackets in this clause but nowhere else. I would personally prefer if it were consistent thoughout, if I had to maintain it. But it's just a nitpick.

INSERT INTO #tempSpecOrderReplication 
([Ord_L_ID], [Ord_ID], [Quantity], [UOM], [Price], 
 [Is_Spec_Order], [Item_Number], [Order_Number]) 
VALUES (...); --These values come from the .NET DataTable
\$\endgroup\$
4
  • \$\begingroup\$ Good spot on the []'s, I forget to use those consistently. \$\endgroup\$ Oct 20, 2014 at 16:15
  • 1
    \$\begingroup\$ @MichaelMcGriff, you only need the brackets when you are using a keyword as a column, or your table name has spaces or the column name has spaces, etc. if your names are weird and the SQL Engine doesn't like them put them in Square Brackets. otherwise it's a waste of characters, and personally it makes the code clunky \$\endgroup\$
    – Malachi
    Oct 20, 2014 at 16:44
  • 1
    \$\begingroup\$ @Malachi you're right of course. I got into the habit of using them as SSMS provides them for you on all of their auto-generated queries and it makes it easier for me to tell exactly what RBDMS I'm querying against by a simple glance. \$\endgroup\$ Oct 20, 2014 at 16:51
  • \$\begingroup\$ square brackets help to search for fields in text searches, since the field names are often part of other variable names \$\endgroup\$
    – Markus
    Mar 14, 2017 at 15:01
3
\$\begingroup\$

I don't know that I would create a Temporary table to do this.

You have two things that you are doing here

  1. Inserting if the record doesn't exist
  2. updating the record if a record with the same ID already exists

I think that instead of a MERGE I would just use an If Then Statement. I always think about keeping it simple, and I think that using an If Statement would be much simpler than trying to Merge a Temp Table into an Existing table.

It would look something like this

DECLARE @Ord_L_ID INT --a very important input variable
DECLARE @Is_Spec_Order BIT
DECLARE @Order_Number INT
DECLARE @Ord_ID INT
DECLARE @Item_Number INT
DECLARE @Quantity INT
DECLARE @UOM INT -- Wasn't sure what this was Guessed it was another INT
DECLARE @Price Decimal -- There is also Money and SmallMoney DataTypes

-- ETC.

IF EXISTS (SELECT * FROM Spec_Order_Replication AS SOR WHERE SOR.Ord_L_ID = @Ord_L_ID)
    UPDATE Spec_Order_Replication
    SET 
        Is_Spec_Order = @Is_Spec_Order
    WHERE 
        Ord_L_ID = @Ord_L_ID
ELSE
    INSERT INTO Spec_Order_Replication
    (
        @Ord_L_ID
        , @Order_Number
        , @Ord_ID
        , @Item_Number
        , @Quantity
        , @UOM
        , @Price
        , @Is_Spec_Order
    )

This should be housed in a stored procedure so that you can call it from your Application using whatever data is sent to it.

Forcing the assignment of the table's Primary Key is a little weird but makes sense in this circumstance, but be careful because it could cause issues.


Your code does this

  • takes a comma delimited list
  • creates a temporary table
  • inserts records into the Temporary Table
  • Merges a permanent table to a Temporary Table on an ID
    • ID may or may not exist
  • finds the matches
    • updates table
  • finds where there are no matches
    • inserts into permanent table

my code does this

  • takes in data for a row
  • determines if data exists in permanent table
    • if it exists the permanent table is updated
    • if it doesn't exist new data is inserted into the permanent table

My code does far less than your code, and as such will be much more efficient and take up less processing on the server, regardless of the fact that the procedure will be called once for every row of data.

\$\endgroup\$
8
  • 1
    \$\begingroup\$ Won't I have to do this row-by-row then? I'm specifically trying to avoid doing that. \$\endgroup\$ Oct 20, 2014 at 16:44
  • \$\begingroup\$ you were inserting into the Temporary table row by row as well. \$\endgroup\$
    – Malachi
    Oct 20, 2014 at 16:46
  • \$\begingroup\$ I'm not actually, I truncated that part of it for brevity. I build the entire values list via a LINQ query out of the DataTable so that it's one large INSERT statement. \$\endgroup\$ Oct 20, 2014 at 16:47
  • 1
    \$\begingroup\$ My (possibly incorrect) impression was that the IF...EXISTS approach was slightly less efficient and more prone to race conditions than the MERGE approach, which is why I was doing it this way at all. I'll test your approach to see for sure one way or the other. Thank you for your review. \$\endgroup\$ Oct 20, 2014 at 17:16
  • 1
    \$\begingroup\$ I'm doubtful about this answer. As @MichaelMcGriff pointed out, it is typically better to use a set based approach. \$\endgroup\$
    – RubberDuck
    Oct 20, 2014 at 17:17

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.