6

Can anyone tell me the following 2 ways of inserting record creates better performance?

Case 1

SqlCommand cmd = new SqlCommand();

for (int i = 0; i < 10000; i++)
{
  cmd = new SqlCommand("insert into test(id, name) value('" + i + "', '" + i + "')");
  cmd.ExecuteNonQuery();
}

Case 2

string sql = null;

for (int i = 0; i < 10000; i++)
{
  sql += "insert into test(id, name) value('" + i + "', '" + i + "')";
}

SqlCommand cmd = new SqlCommand(sql, conn);
cmd.ExecuteNonQuery();
marc_s
  • 732,580
  • 175
  • 1,330
  • 1,459
RoyT
  • 139
  • 1
  • 1
  • 14
  • 3
    Have you run a simple timing test? Execute each one 10,000 times, and see which one runs faster. – abelenky Nov 21 '11 at 21:38
  • Same, but case one is preferred. Same because you'll use connection pooling. – SQLMason Nov 21 '11 at 21:39
  • 16
    First of all: **STOP** concatenating together your SQL code!! This is an invitation to hacker to attack you with SQL injection! Use **parametrized queries** instead! – marc_s Nov 21 '11 at 21:39
  • 4
    @DanAndrews: really?!?!?? You think creating 10'000 `SqlCommand` instances and executing them one by one is just as fast as creating a single instance and executing it just once??? – marc_s Nov 21 '11 at 21:39
  • You are right, it would be faster. – SQLMason Nov 21 '11 at 21:51

5 Answers5

63

First of all: STOP concatenating together your SQL code!! This is an invitation to hackers everywhere to attack you with SQL injection! Use parametrized queries instead!

I would use this solution: create a single SqlCommand with a parametrized query, and execute that:

string stmt = "INSERT INTO dbo.Test(id, name) VALUES(@ID, @Name)";

SqlCommand cmd = new SqlCommand(smt, _connection);
cmd.Parameters.Add("@ID", SqlDbType.Int);
cmd.Parameters.Add("@Name", SqlDbType.VarChar, 100);

for (int i = 0; i < 10000; i++)
{
    cmd.Parameters["@ID"].Value = i;
    cmd.Parameters["@Name"].Value = i.ToString();

    cmd.ExecuteNonQuery();
}

or use SqlBulkCopy, especially if you're inserting even more than 10'000 rows.

marc_s
  • 732,580
  • 175
  • 1,330
  • 1,459
  • 1
    While I usually agree that passing in unsanitized parameters directly to concatenated SQL is bad - in the context of this code it is literally harmless. There are no User Input values to be concerned with - it looks like a quick one off operation. – hajikelist Jun 25 '15 at 20:15
  • The new recommended way is to use `AddWithValue` – Si8 Dec 15 '20 at 18:11
  • @Si8: **MOST DEFINITELY NOT!!!** - see Dan Guzman says: ["AddWithValue is Evil"](http://www.dbdelta.com/addwithvalue-is-evil/) - please read the article and stop using it! – marc_s Dec 15 '20 at 18:21
  • Hmmmmm i remember seeing it somewhere but now can't find it. And you are right... Thanks. – Si8 Dec 15 '20 at 18:25
  • @Si8: I'd be interested to know who or what site is propagating this ...... really really bad advice....... :-) – marc_s Dec 15 '20 at 18:26
  • I've always used Add but recently switched to AddWithValue, because of it. Good thing I came across your advice and note. – Si8 Dec 15 '20 at 18:46
  • @marc_s Now i know where, it says it in the VS Dev application...It says .Add() is obsolete and to use .AddWithValue(); – Si8 Dec 16 '20 at 13:55
  • @Si8: **scary!** That's official MS docs..... but yes - that one overload has been deprecated - but you should *definitely NOT* replace it with `AddWithValue` ! Thanks – marc_s Dec 16 '20 at 17:27
  • 1
    I would imagine VS and C# being Microsoft, they wouldn't recommend something not accurate but again having used SharePoint that is definitely not the case! I will stick with `Add()` even if VS gives me warning. – Si8 Dec 16 '20 at 18:53
  • @Si8: just use this overload: `Add(String, SqlDbType)` and **explicitly** define the desired datatype - set the actual **value** of the parameter by using the `.Value = xxx` syntax - as shown in my response – marc_s Dec 16 '20 at 19:23
6

The second approach looks faster than #1 because you send the INSERT commands at once. In the first there's a round trip to the SQL server for each ExecuteNonQuery.

But you should try the bulk insert command: BULK INSERT (Transact-SQL), I guess you'll get a better performance than any one of the options you provided.

[]'s

Fabio
  • 3,020
  • 4
  • 39
  • 62
  • 1
    While this might be a good idea, how does it answer the question of "which is faster, option 1 or option 2?". You've said "Do option 3 instead". – Ken White Nov 21 '11 at 21:40
  • 2
    @KenWhite I think the answer is plenty appropriate. It doesn't directly answer the question because the OP is using an inefficient process. I have a hunch the OP is not in search of exactly which way uses fewer CPU cycles (as that they could easily test themselves if that was exactly what they wanted to know). Instead, I believe the OP is trying to understand which way is more efficient, and Fabio has provided a solution better than either of the OP's. – Kiley Naro Nov 21 '11 at 21:43
  • 1
    It doesn't. As you've said, it's a third option. Sometimes we don't get the exactly answer we want, but we got somwthing that make us look at things from a different point of view. []'s – Fabio Nov 21 '11 at 21:45
  • 1
    I really hate to see answers upvoted when they don't answer a very specific question that was asked. – SQLMason Nov 21 '11 at 21:46
  • If it's a third option, it's fine to add as a footnote to the answer that actually answers the question asked, or as a comment to the original question. It's not an answer to the question asked, and shouldn't be posted as such. I could have just downvoted or flagged it as "not an answer"; I didn't. I gave the poster a chance to either expand on the answer provided or delete it first. Answers should actually do that - answer the question asked. Additional advice or information on alternatives can then be provided. – Ken White Nov 21 '11 at 21:50
  • @KenWhite I suppose it's worth mentioning that while marc_s provided an answer that doesn't even come close to "answering the specific question asked" (it is definitely a 'do option 3 instead' answer), it's unquestionably the best answer. Out of curiosity, do you feel the same way about his answer as you do about Fabio's? – Kiley Naro Nov 21 '11 at 22:15
  • Yep, I do. :) He posted after the discussion in this question started However, his post was also more than a single line of text with a URL (which in itself is also not considered a proper answer, BTW, especially when that URL is to an off-site location, even if that site is MS). @Fabio, much better. :) Nicely done. – Ken White Nov 21 '11 at 22:17
1

I don't think the second one will work.

There is however a syntax in SQL Server 2008 for inserting multiple rows in a single INSERT statement and I think that will be faster than both the options you proposed:

INSERT INTO test (id, name)
VALUES
('1', 'foo'),
('2', 'bar'),
('3', 'baz')
 -- etc...

However if you really want high performance, consider using the SqlBulkCopy class.

Mark Byers
  • 811,555
  • 193
  • 1,581
  • 1,452
1

It should be noted exactly that as-is, neither case will work.

Case #1 requires a connection to be specified.

Case #2 requires you to end your statements with a semi-colon in order to run multiple commands, like so:

string sql = null;

for (int i = 0; i < 10000; i++)
{
  sql += "insert into test(id, name) value('" + i + "', '" + i + "');";
}

SqlCommand cmd = new SqlCommand(sql, conn);
cmd.ExecuteNonQuery();

Ultimately the best way would be for you to just test it yourself on several thousand rows. My guess would be that the Case #2 would be better for performance because not only would it require setting up only a single SqlCommand object, but it only hits the database a single time.

Kiley Naro
  • 1,749
  • 1
  • 14
  • 20
0

The second one is probably faster, because you end up with a single roundtrip. Both are equally awful, because you do not use parameterized queries.

Sergey Kalinichenko
  • 714,442
  • 84
  • 1,110
  • 1,523