Right code to retrieve data from sql server database

Question

I have some problems in database connection and wonder if I have something wrong in my code. Please review. This question is related: Switch between databases, use two databases simultaneously question.

cs="Data Source=mywebsite.com;Initial Catalog=database;User Id=root;Password=toor;Connect Timeout=10;Pooling='true';"

using (SqlConnection cnn = new SqlConnection(WebConfigurationManager.ConnectionStrings["cs"].ConnectionString))
{
    using (SqlCommand cmmnd = new SqlCommand("", cnn))
    {
        try
        {
            cnn.Open();

            #region Header & Description
            cmmnd.Parameters.Add("@CatID", SqlDbType.Int).Value = catId;
            cmmnd.CommandText = "SELECT UpperID, Title, Description FROM Categories WHERE CatID=@CatID;";

            string mainCat = String.Empty, rootCat = String.Empty;

            using (SqlDataReader rdr = cmmnd.ExecuteReader())
            {
                if (rdr.Read())
                {
                    mainCat = rdr["Title"].ToString();
                    upperId = Convert.ToInt32(rdr["UpperID"]);
                    description = rdr["Title"];
                }
                else { Response.Redirect("/", false); }
            }

            if (upperId > 0) //If upper category exists add its name
            {
                cmmnd.Parameters["@CatID"].Value = upperId;
                cmmnd.CommandText = "SELECT Title FROM Categories WHERE CatID=@CatID;";
                using (SqlDataReader rdr = cmmnd.ExecuteReader())
                {
                    if (rdr.Read())
                    {
                        rootCat = "<a href='x.aspx'>" + rdr["Title"] + "</a> &raquo; ";
                    }
                }
            }
            #endregion

            #region Sub-Categories
            if (upperId == 0) //show only at root categories
            {
                cmmnd.Parameters["@CatID"].Value = catId;
                cmmnd.CommandText = "SELECT Count(CatID) FROM Categories WHERE UpperID=@CatID;";

                if (Convert.ToInt32(cmmnd.ExecuteScalar()) > 0)
                {
                    cmmnd.CommandText = "SELECT CatID, Title FROM Categories WHERE UpperID=@CatID ORDER BY Title;";

                    using (SqlDataReader rdr = cmmnd.ExecuteReader())
                    {
                        while (rdr.Read())
                        {
                            subcat.InnerHtml += "<a href='x.aspx'>" + rdr["Title"].ToString().ToLower() + "</a>\n";
                            description += rdr["Title"] + ", ";
                        }
                    }
                }
            }
            #endregion
        }
        catch (Exception ex) { HasanG.LogException(ex, Request.RawUrl, HttpContext.Current); Response.Redirect("/", false); }
        finally { cnn.Close(); }
    }
}

The random errors I'm receiving are:

• A transport-level error has occurred when sending the request to the server. (provider: TCP Provider, error: 0 - An existing connection was forcibly closed by the remote host.)
• A network-related or instance-specific error occurred while establishing a connection to SQL Server. The server was not found or was not accessible. Verify that the instance name is correct and that SQL Server is configured to allow remote connections. (provider: Named Pipes Provider, error: 40 - Could not open a connection to SQL Server)
• Timeout expired. The timeout period elapsed prior to obtaining a connection from the pool. This may have occurred because all pooled connections were in use and max pool size was reached.
• Cannot open database "db" requested by the login. The login failed. Login failed for user 'root'.

Answer 1

There's no real issues here.

You don't need the extraneous finally { cnn.close(); } as the using clause will take care of that for you. However changing it will have exactly zero impact.

Another thing is that I would put the try .. catch outside of the using clause with a redirect. But, again, I don't think that would affect the dispose from being called.

It's interesting that you would get connection pool errors (timeout expired) if you are always properly disposing of your connections, as you've shown.

Which leaves us with only one real solution: switch hosting providers. They have either overloaded their DB server to the point of unusability or some hardware element in their network setup (nic, switch, router, etc) is bad and dropping packets.

Answer 2

If you're connecting remotely to a database provider, you need to look at several possibilities like your own network configuration, firewall setup, etc.

Use a packet sniffer to figure out if lost packets are the issue.

Connection pooling is setup on your local machine, the server making the connections. If the database provider only allows for 5 connections and your connection pool is setup for 50 connections, well... you can do the math. It looks like you're closing the connections properly, so no issues there.

True... one error on "description = rdr["Title"];", should that be "description = rdr["Description"].ToString()"?

No need to put a using statement around the SqlCommand object and since you're using ad-hoc queries, just use string.Format("sql test {0}", param). This way, you can reuse the SqlCommand object without having to clear the parameters.

The biggest issue I see here is that you've mixed the presentation layer with the business layer with the datasource layer. Dump the try...catch and allow the business layer to handle logging stuff. Return an object to the presentation layer and allow it to perform the redirects. Keep the datasource layer very simple... get the data and return an entity. The business layer can handle any business logic on the entity itself.

SQL Server not found could be your fault or the providers... if the provider is at fault often, change providers.

Answer 3

There are couple of inconsistencies which need to fixed:

1. description = rdr["Title"]; no proper casting defined.
2. Same command object is used for each sql statement and even you are not clearing parameters, it would be ideal if a separate command should be used for each sql statement.
3. Too many redirections as well, it is best to handle redirection at the end of method.
4. Check the database server health as well, it looks like database server is not responsive enough.

Hope it will help.

Answer 4

Are you sure that the DB is configured to grant remote access using TCP?