0

I am trying to INSERT a row when the row does not exist. I have 2 tables, and I check 1 by 1, if the record does not exist in the second table, then do INSERT.

DECLARE @Counter int;
DECLARE @jumlah int;
DECLARE @namamerchant varchar(200);
DECLARE @namapemilik varchar(200);
DECLARE @alamat varchar(200);
SET @Counter = 1;

SELECT @jumlah = COUNT(*)
FROM ##TempMerchant_upload_id tmui;
SELECT @jumlah;
WHILE (@Counter <= @jumlah)
BEGIN

    SELECT @namamerchant = tmui.Nama_Merchant
    FROM ##TempMerchant_upload_id tmui
    WHERE tmui.id = @Counter;
    SELECT @alamat = tmui.Alamat_Pemilik
    FROM ##TempMerchant_upload_id tmui
    WHERE tmui.id = @Counter;
    SELECT @namapemilik = Nama_Pemilik
    FROM ##TempMerchant_upload_id
    WHERE ##TempMerchant_upload_id.id = @Counter;

    IF (SELECT COUNT(*)
        FROM merchant_negative_db_copy mndc
        WHERE mndc.Nama_Merchant = @namamerchant
          AND mndc.Alamat_Merchant = @alamat
          AND mndc.Nama_Pemilik = @namapemilik) = 0
    BEGIN
        SET @sql = 'INSERT INTO merchant_negative_db_copy SELECT *,tes, GETDATE(),0 FROM ##TempMerchant_upload_id where id = ''' + @Counter + '''';

        EXEC (@sql);
    END;

    SET @Counter = @Counter + 1;
END;

It's showing error like this when I run it:

Conversion failed when converting the varchar value 'INSERT INTO merchant_negative_db_copy SELECT *,tes, GETDATE(),0 FROM ##TempMerchant_upload_id where id = '' to data type int.

Any suggestion?

Dale K
  • 25,246
  • 15
  • 42
  • 71
Romli Eko
  • 45
  • 1
  • 9
  • 2
    Don't use `SELECT *` when doing an `INSERT INTO ... SELECT`. Instead, always list out the target columns. – Tim Biegeleisen Jul 07 '21 at 09:40
  • 2
    Don't **inject** your parameters, **parametrise** them... (Seems silly when i say it, right?) Why is this statement "dynamic" anyway, it has no reason to be. The only thing that is "dynamic" is the parameter, and parameters have no right being dynamic. – Thom A Jul 07 '21 at 09:41
  • 1
    Where is your `@sql` declaration? – HoneyBadger Jul 07 '21 at 09:42
  • Should be END statements to match the BEGIN statements. – John D Jul 07 '21 at 09:43
  • 1
    In truth, there's a lot wrong here. The fact that you are using a `WHILE` is also very likely a problem too; SQL is a set based language it excels at set based solutions and performs awfully at iterative ones. The fact you are checking the values one by one is a design flaw. If you want to `INSERT` all the rows that aren't in the table, then use an `EXISTS` and `INSERT` them **all** in one single statement, not one my one. We most certainly have an [XY Problem](http://xyproblem.info). – Thom A Jul 07 '21 at 09:48
  • 2
    `@Counter` is declared as INT so when TSQL sees `... + @Counter + ...` in your assignment of `@Sql` it thinks the result of the whole assignment should be INT too. As a start, you should use `cast(@Counter as varchar)`. That's just the start, there are good suggestions in the comments that will help you. – John D Jul 07 '21 at 09:53
  • @JohnD that's, honestly, bad advice. The statement doesn't need to be dynamic, and *if* it did, the value should be parametrised, not injected. – Thom A Jul 07 '21 at 09:54
  • 1
    @Lamu I agree, but my approach is to first to explain the error, and then guide the OP to a better solution, I did advise the OP to listen to the good advice here. – John D Jul 07 '21 at 09:58
  • L a r n u... @JohnD . – Thom A Jul 07 '21 at 09:58
  • @Larnu Sorry about that - OCR engines often make the same mistake ie confusing "rn" with "m". I just chopped onions so my eyes are watery! All your points are correct, and well made. I'm not supporting the OP's approach just trying to explain the error. – John D Jul 07 '21 at 10:13
  • @JohnD `cast(.. as varchar)` is very dangerous. The default size is `1` in variables, 30 in cast. It's all too easy to end up with silently truncated data. If eg, you stored the value in a variable for readability, you'd end up with a single-digit counter – Panagiotis Kanavos Jul 07 '21 at 11:00

1 Answers1

2

I'm posting an answer because the accepted one is a bad answer. Normally, I'd wait for a description of the real problem, so a single query can be written without the loop, or the temporary table.

There's no reason to use dynamic SQL in the first place. @Counter can be used in a SQL query directly :

INSERT INTO merchant_negative_db_copy (ColA,ColB,...)
SELECT a,b,....,tes, GETDATE(),0 
FROM ##TempMerchant_upload_id 
WHERE id = @Counter

There's no reason to loop to check if a record already exists. One can use WHERE NOT EXISTS ( select 1 from target where ....) for that. Once that's done, the Counter variable isn't needed any more. Another option is to use a LEFT JOIN between source and target, and only insert records that have no match.

INSERT INTO merchant_negative_db_copy (ColA,ColB,...)
SELECT a,b,....,tes, GETDATE(),0 
FROM ##TempMerchant_upload_id source
WHERE NOT EXISTS 
(   SELECT 1
    FROM merchant_negative_db_copy mndc
    WHERE mndc.Nama_Merchant = source.Nama_Merchant
          AND mndc.Alamat_Merchant = source.Alamat_Merchant
          AND mndc.Nama_Pemilik = source.Nama_Pemilik
)

This will be at least N times faster than the loop, especially if the join fields Nama_Merchant, Alamat_Merchant and Nama_Pemilik fields are indexed in both tables.

Panagiotis Kanavos
  • 120,703
  • 13
  • 188
  • 236
  • Does not really address the error in the question and does not explain how the suggested code is to be used. Adds confusion by adding option that's neither explained nor illustrated. – Erik Blomgren Jul 07 '21 at 12:01
  • @ErikBlomgren no it doesn't. Quite the opposite. It completely fixes the actual error - using dynamic SQL when it's not needed. Dynamic SQL is always a problem except for very specific problems – Panagiotis Kanavos Jul 07 '21 at 12:06
  • For clarity; the added option which is not explained nor illustrated and only adds confusion is the LEFT JOIN one. – Erik Blomgren Jul 07 '21 at 12:17