Understanding of nested SQL in C#

Question

using (var connection = new SqlConnection(...))
{
    string sql = "SELECT * FROM tableA";
    using (var command = new SqlCommand(sql,connection))
    {
        using (var reader = command.ExecuteReader(...))
        {
            //***************Sample Start
            string sql2 = "INSERT into tableB(column1) VALUES('"+reader["column1"]+"')";
            using (var command2 = new SqlCommand(sql2,connection))
            {
                ...
            } 
            //***************Sample End
        }
    }
}

By using the above code snippet, I believe its the best practice to deal with SQL in C#. Now after I retrieve a list of records from tableA, for each of the row I would like to insert into tableB.

However, it's throwing an exception

There is already an open DataReader associated with this Command which must be closed first

I know this problem can be solved by creating another method and insert into the table from there, I'm wondering if there is any other way. Thanks for any input.

This is vulnerable to sql injection attacks. Don't use string concatenation to build queries like that! It's practically begging to get hacked. — Joel Coehoorn, Sep 13 '13 at 02:20
possible duplicate of [error 'there is already an open datareader associated with this command which must be closed first'](http://stackoverflow.com/questions/3839569/error-there-is-already-an-open-datareader-associated-with-this-command-which-mu) — Blorgbeard, Sep 13 '13 at 02:20
Spoiler: either turn on `MultipleActiveResultSets` in the connectionstring, or read everything into a `DataTable` or similar, then iterate over that. — Blorgbeard, Sep 13 '13 at 02:21
@JoelCoehoorn, all the values are not getting from user input but from tableA instead which should be fine isn't it? — SuicideSheep, Sep 13 '13 at 02:21
While MARS does work, it should be used "with care" - I say, "with care" because it can cover up flaws in some designs. If possible I'd try to write the query such that it can be done in one-go instead of such a nested approach. — user2246674, Sep 13 '13 at 02:21
@IsaacLem No, it's not fine. What you have there is called a **2nd Order** injection vulnerability. _Everything_ in your database had to be entered a human at some level. Also: why use a vulnerable technique when the correct parameterized option is so easy, and will run faster as a bonus? — Joel Coehoorn, Sep 13 '13 at 02:23
am I missing the obvious but shouldn't one SQL staement of INSERT into tableB(column1) SELECT column1 FROM tableA do the job — OJay, Sep 13 '13 at 02:25
Please see if what you are doing in C# could be done in SQL alone (it looks like it could be done with an insert from select). You would get much better performance, and all this nonsense with multiple readers per connection would become irrelevant. — Sergey Kalinichenko, Sep 13 '13 at 02:25
@dasblinkenlight I would guess from the comments that this is extremely simplified code and that his actual code will require processing or manipulation between read and write. — Corey, Sep 13 '13 at 02:53

Joel Coehoorn · Accepted Answer · 2013-09-13T03:21:29.523

You need to use a different sql connection for the insert than for the select...

...but we can do even better. You can re-write this to be one sql statement, like so:

INSERT into tableB(column1)
    SELECT column1 FROM tableA

And then run it all at once like this:

string sql = "INSERT into tableB(column1, column2) SELECT column1, @othervalue As column2 FROM tableA;";
using (var connection = new SqlConnection(...))
using (var command = new SqlCommand(sql,connection))
{
    command.Paramters.Add("@othervalue", SqlDbType.NVarChar, 50).Value = "something";

    connection.Open();
    command.ExecuteNonQuery();
}

The single sql statement is typically much faster, and you end up with less code, too. I understand that this is likely a simplified example of your real query, but I promise you: you can re-write it all as one statement.

Additionally, sometimes you still want to do some client-side processing or display with the new records after the insert or update. In that case, you still only need to send one call to the database, but there will be two separate sql statements in that single call. The final code would look more like this:

string sql = "INSERT into tableB(column1, column2) SELECT column1, @othervalue As column2 FROM tableA;"
sql += "SELECT columnn1, @othervalue As column2 FROM tableA;";

using (var connection = new SqlConnection(...))
using (var command = new SqlCommand(sql,connection))
{
    command.Paramters.Add("@othervalue", SqlDbType.NVarChar, 50).Value = "something";

    connection.Open();
    using (var reader = command.ExecuteReader() )
    {
        while (reader.Read() )
        {
           //...
        }
    }
}

And because someone else brought up MARS (multiple active result sets), I'll add that while this can work, I've had mixed results using it for inserts/updates. It seems to work best when everything that shares a connection is only doing reads.

Correct me if I'm wrong. The real scenario is that tableB will have ten columns where the expected source are 9 columns from tableA and another 1 column from user input. Can your example still working? — SuicideSheep, Sep 13 '13 at 02:32
@IsaacLem Yes, you can still do that. The value for the column entered by the user can be specified using query parameters. — Joel Coehoorn, Sep 13 '13 at 03:00
I also added an example of how to return data afterwards for display or other purposes, if you need it. — Joel Coehoorn, Sep 13 '13 at 03:28

Corey · Answer 2 · 2013-09-13T03:11:49.123

As has been mentioned in comments, you need a separate database connection for the insert. Each connection can handle one active statement at a time, and you have two here - one for the SELECT, one (at a time) for the INSERT.

Try this for instance:

string srcqry = "SELECT * FROM tableA";
using (SqlConnection srccon = new SqlConnection(ConnectionString))
using (SqlCommand srccmd = new SqlCommand(srcqry, srccon))
{
    srccon.Open();
    using (SqlDataReader src = srccmd.ExecuteReader())
    {
        string insqry = "INSERT INTO tableB(column1) VALUES(@v1)";

        // create new connection and command for insert:
        using (SqlConnection inscon = new SqlConnection(ConnectionString))
        using (SqlCommand inscmd = new SqlCommand(insqry, inscon))
        {
            inscmd.Parameters.Add("@v1", System.Data.SqlDbType.NVarChar, 80);
            inscon.Open();

            while (src.Read())
            {
                inscmd.Parameters["@v1"].Value = src["column1"];
                inscmd.ExecuteNonQuery();
            }
        }
    }
}

Using parameters solves the SQL Injection vulnerability. You should always do this rather than building the query string from raw user input, or from data that you're pulling from a database, or... well, always. Write some helper methods to make it easier if you like, just make sure you do it.

I couldn't find any reference for `rdr`, is it supposed to be replace with `src` instead? — SuicideSheep, Sep 13 '13 at 02:59
Based on your code, it works perfectly. Now there is this extra column that the value has to be return by stored procedure. If I understand it correctly, I will need another connection for that? — SuicideSheep, Sep 13 '13 at 03:22
Possibly, yes. Starting to get a bit complicated though, and if you're going to be calling the stored procedure for every row it might get a bit messy. Same principle can be used though - define and pass parameters to the SP the same as to the insert query. — Corey, Sep 13 '13 at 03:55

score 1 · Answer 3 · answered Sep 13 '13 at 02:24

1

aside from a bad example, why not just simplify the query to

insert into TableB (column1) select column1 from TableA

answered Sep 13 '13 at 02:24

DRapp

47,638
12
72
142

Understanding of nested SQL in C#

3 Answers3

Linked