6

I need to speed up performing 12 queries in my application. I switched from a regular foreach to Parallel.ForEach. But sometimes I get an error saying "ExecuteReader requires an open and available connection The connection's current state is connecting." It is my understanding that since many of the 12 queries are using the same InitialCatalog, there is not really a new connection for of the 12 and that may be the problem? How can i fix this? "sql" is a list of type "Sql"- a class is just a string name, string connectiona and a List of queries. Here is the code:

 /// <summary>
    /// Connects to SQL, performs all queries and stores results in a list of DataTables
    /// </summary>
    /// <returns>List of data tables for each query in the config file</returns>
    public List<DataTable> GetAllData()
    {
        Stopwatch sw = new Stopwatch();
        sw.Start();
        List<DataTable> data = new List<DataTable>();

         List<Sql> sql=new List<Sql>();

        Sql one = new Sql();
         one.connection = "Data Source=XXX-SQL1;Initial Catalog=XXXDB;Integrated Security=True";
         one.name = "Col1";
         one.queries.Add("SELECT Name FROM [Reports]");
         one.queries.Add("SELECT Other FROM [Reports2]");
         sql.Add(one);

        Sql two = new Sql();
         two.connection = "Data Source=XXX-SQL1;Initial Catalog=XXXDB;Integrated Security=True";
         two.name = "Col2";
         two.queries.Add("SELECT AlternateName FROM [Reports1]");
         sql.Add(two);

         Sql three = new Sql();
         three.connection = "Data Source=YYY-SQL2;Initial Catalog=YYYDB;Integrated Security=True";
         three.name = "Col3";
         three.queries.Add("SELECT Frequency FROM Times");
         sql.Add(three);


        try
        {
            // ParallelOptions options = new ParallelOptions();
            //options.MaxDegreeOfParallelism = 3;
            // Parallel.ForEach(sql, options, s =>
            Parallel.ForEach(sql, s =>
            //foreach (Sql s in sql)
            {
                foreach (string q in s.queries)
                {
                    using (connection = new SqlConnection(s.connection))
                    {
                        connection.Open();
                        DataTable dt = new DataTable();
                        dt.TableName = s.name;
                        command = new SqlCommand(q, connection);
                        SqlDataAdapter adapter = new SqlDataAdapter();
                        adapter.SelectCommand = command;
                        adapter.Fill(dt);
                        //adapter.Dispose();

                        lock (data)
                        {
                            data.Add(dt);
                        }
                    }
                }
            }
            );
        }
        catch (Exception ex)
        {
            MessageBox.Show(ex.ToString(), "GetAllData error");
        }

        sw.Stop();
        MessageBox.Show(sw.Elapsed.ToString());

        return data;
    }

Here's the Sql class I made that you'd need:

/// <summary>
/// Class defines a SQL connection and its respective queries
/// </summary>
public class Sql
{
    /// <summary>
    /// Name of the connection/query
    /// </summary>
    public string name { get; set; }
    /// <summary>
    /// SQL Connection string
    /// </summary>
    public string connection { get; set; }
    /// <summary>
    /// List of SQL queries for a connection
    /// </summary>
    public List<string> queries = new List<string>();
}

3 Answers3

8

I would refactor out your business logic (connecting to the database).

public class SqlOperation
{
    public SqlOperation()
    {
        Queries = new List<string>();
    }

    public string TableName { get; set; }
    public string ConnectionString { get; set; }
    public List<string> Queries { get; set; }
}

public static List<DataTable> GetAllData(IEnumerable<SqlOperation> sql)
{
    var taskArray =
        sql.SelectMany(s =>
            s.Queries
             .Select(query =>
                Task.Run(() => //Task.Factory.StartNew for .NET 4.0
                    ExecuteQuery(s.ConnectionString, s.TableName, query))))
            .ToArray();

    try
    {
        Task.WaitAll(taskArray);
    }
    catch(AggregateException e)
    {
        MessageBox.Show(e.ToString(), "GetAllData error");
    }

    return taskArray.Where(t => !t.IsFaulted).Select(t => t.Result).ToList();
}

public static DataTable ExecuteQuery(string connectionString, string tableName, string query)
{
    DataTable dataTable = null;

    using (var connection = new SqlConnection(connectionString))
    {
        dataTable = new DataTable();
        dataTable.TableName = tableName;
        using(var command = new SqlCommand(query, connection))
        {
            connection.Open();

            using(var adapter = new SqlDataAdapter())
            {
                adapter.SelectCommand = command;
                adapter.Fill(dataTable);
            }
        }
    }

     return dataTable;
}
Dustin Kingen
  • 20,677
  • 7
  • 52
  • 92
  • 1
    Why are you using `Parallel.ForEach` to start a bunch of tasks? You should either be starting the tasks serially, or letting `Parallel.ForEach` handle the parallelization. In this case, I see no reason not to do the latter. – Servy Apr 18 '13 at 19:08
  • I was just trying to replace foreach loop with Parallel.ForEach. Basically, since some these queries take a while, I wanted to do more than 1 query at a time. – Theodosius Von Richthofen Apr 18 '13 at 19:20
  • 1
    @Romoku: Looks great but produces errors: System.Threading.Tasks.Task does not contain a definition for 'Result' and no extension method 'Result' accepting a first argument of type 'System.Threading.Tasks.Task' could be found And 'System.Threading.Tasks.Task' does not contain a definition for 'Run' . Is there a different reference I need ot something? – Theodosius Von Richthofen Apr 18 '13 at 20:36
  • how to do this in .NET 4.0? – Theodosius Von Richthofen Apr 18 '13 at 21:28
  • 1
    Sorry I didn't have a lot of time to make a proper answer yesterday, but I refactored it for you. You just need to reference [`System.Threading.Tasks`](http://msdn.microsoft.com/en-us/library/dd321468.aspx). – Dustin Kingen Apr 19 '13 at 11:53
  • Like I said, System.Threading.Tasks.Task does not contain a definition for 'Run'. Argument 1: cannot convert from 'TResult[]' to 'System.Threading.Tasks.Task[]' Delegate 'System.Func' does not take 1 arguments No overload for method 'GetAllData' takes 0 arguments The best overloaded method match for 'System.Threading.Tasks.Task.WaitAll(params System.Threading.Tasks.Task[])' has some invalid arguments – Theodosius Von Richthofen Apr 19 '13 at 12:42
  • 1
    Use `Task.Factory.StartNew` instead. – Dustin Kingen Apr 19 '13 at 12:48
  • By the way you don't have to explicitly `Open` the underlying `SqlConnection` when calling `SqlDataAdapter.Fill()` method. It does that for you internally. – user1451111 Jul 14 '18 at 17:24
6

Ado.Net has a pretty clever connection pooling, so in general you should just open connections and close connections per command and let the pool handle whether they are really being opened or closed.

So one connection per command:

  Parallel.ForEach(sql, s=>
            //foreach (Sql s in sql)
            {
                foreach (string q in s.queries)
                {
                    using (connection = new SqlConnection(s.connection))
                    {
                        connection.Open();
                        DataTable dt = new DataTable();
                        dt.TableName = s.name;
                        command = new SqlCommand(q, connection);
                        SqlDataAdapter adapter = new SqlDataAdapter();
                        adapter.SelectCommand = command;
                        adapter.Fill(dt);
                        //adapter.Dispose();

                        lock(data){
                            data.Add(dt);
                        }
                    }
                }
            }
faester
  • 14,886
  • 5
  • 45
  • 56
  • 5
    He also needs to synchronize the `data.Add(dt);` since `List(T).Add` is not thread safe. – Dustin Kingen Apr 18 '13 at 18:42
  • Hmmmmm...trying that (putting the foreach q in queries before the using results in an error saying Cannot open database "xxx" requested by the login. The login failed. Login failed for user "xxxxx". Every time. – Theodosius Von Richthofen Apr 18 '13 at 18:55
  • 1
    @user1029770: Sorry, the connection needs to be open. See edit. – faester Apr 18 '13 at 19:04
  • huh, now I get the ExecuteReader requires an open and available Connection. The connection's current state is connecting error every time. Why would I have to do connection.Open() when the original never needed to that? Sorry, I'm new! – Theodosius Von Richthofen Apr 18 '13 at 19:08
  • 1
    Perhaps because the dataadapter handles opening connections etc. If you provide some more code I will make it run. – faester Apr 18 '13 at 19:19
3

You can also use MultipleActiveResultSets=true; in connection string to support multiple readers