1
\$\begingroup\$

I have pasted a method I wrote, which is too slow to my liking (easily takes 60+ seconds to execute). The method loops through 12 different servers, checks if userX has access to any of the databases in this server and reports the result. I'm already using a table to save all of this information. That information is already stored. This method is supposed to check if the records in that table and the reality match. If not, report back any differences.

public string showMissingDatabaseInfo()
{
    MyUser user = (MyUser)Session["CurrentUser"];

    MyFramework.Database myDB = new MyFramework.Database();
    List<LoginDatabase> listExistingDBpermsOfUser = dbRepos.getExistingDBpermissionsByUser(user.LoginID);
    List<string> listMissingDBpermsOfUsers = new List<string>();

    StringBuilder result = new StringBuilder();

    List<SqlParameter> myParams = new List<SqlParameter>();

    for (int teller = 13; teller <= 25; teller++)
    {
        //The brackets, [ ], are essential in the query below. Otherwise the processor will be confused with certain DB names
        string qryAboutToBeExecuted = "DECLARE @DBuser_sql VARCHAR(4000)" + Environment.NewLine +
                    "DECLARE @DBuser_table TABLE([DBName] VARCHAR(200), UserName VARCHAR(250), LoginType VARCHAR(500))" + Environment.NewLine +
                    "SET @DBuser_sql = 'SELECT DISTINCT ''[?]'' AS [DBName],[?].[sys].[database_principals].[name] AS Name,[?].[sys].[database_principals].[type_desc] AS LoginType FROM [?].sys.database_principals " + Environment.NewLine +
                    "WHERE [?].sys.database_principals.sid NOT IN(0x01, 0x00) AND [?].sys.database_principals.sid IS NOT NULL AND [?].sys.database_principals.type NOT IN(''C'') AND [?].sys.database_principals.is_fixed_role <> 1 " + Environment.NewLine +
                    "AND [?].sys.database_principals.name NOT LIKE ''##%'' AND ''[?]'' NOT IN (''master'',''msdb'',''model'',''tempdb'') AND Name LIKE ''" + user.LoginUsername + "'' ORDER BY Name'" + Environment.NewLine +
                    "INSERT @DBuser_table" + Environment.NewLine +
                    "EXEC sp_MSforeachdb @command1 = @dbuser_sql" + Environment.NewLine +
                    "SELECT *" + Environment.NewLine +
                    "FROM @DBuser_table" + Environment.NewLine +
                    "ORDER BY [DBName]";
        try
        {
            DataSet ds = myDB.executeResultDataset("DBServer" + teller, qryAboutToBeExecuted, myParams);

            foreach (DataRow row in ds.Tables[0].Rows)
            {
                try
                {
                    string currentDBnaam = row[ds.Tables[0].Columns[0]].ToString();
                    LoginDatabase testDB = listExistingDBpermsOfUser.Where(c => c.DBName.Equals(currentDBnaam)).FirstOrDefault();
                    if (testDB != null && !String.IsNullOrWhiteSpace(testDB.DBName))
                    {
                        listExistingDBpermsOfUser.Remove(testDB);
                    }
                    else
                    {
                        listMissingDBpermsOfUsers.Add(currentDBnaam);
                    }
                }
                catch (Exception)
                {
                }
            }
        }
        catch (Exception)
        {
        }
    }
    result.Append("<div>");
    int totalForMissingDBperms = listMissingDBpermsOfUsers.Count;
    int totalForExistingDBperms = listExistingDBpermsOfUser.Count;

    if (totalForExistingDBperms + totalForMissingDBperms == 0)
    {
        result.Append("The database permissions of the user in SQL Server Management Studio and the LoginDatabase table are perfectly synced and up to date.");
    }
    else
    {
        if (totalForMissingDBperms > 0)
        {

            result.Append("The user has access to the following databases, even though that's not specified below: <ul>");
            foreach (string objMissingDB in listMissingDBpermsOfUsers)
            {
                result.Append("<li>" + objMissingDB + "</li>");
            }
            result.Append("</ul> <br />");
        }


        if (totalForExistingDBperms > 0)
        {
            result.Append("<br />The LoginDatabase table also indicates the user has access to the following tables, though in reality he/she doet not. <ul>");
            foreach (LoginDatabase objLoginDB in listExistingDBpermsOfUser)
            {
                result.Append("<li>" + objLoginDB.DBName + "</li>");
            }
            result.Append("</ul>");
        }
    }
    result.Append("</div>");


    return result.ToString();
}
\$\endgroup\$
3
  • 1
    \$\begingroup\$ Wouldn't caching be a good solution? Instead of checking every servers, you could cache all the content in one place? \$\endgroup\$
    – IEatBagels
    Oct 1 '15 at 15:24
  • \$\begingroup\$ Possibly yes. The permissions I'm checking change at a rather uncommon pace, so caching might not be a bad idea. I'm afraid I don't know how to do that though. Will google in a moment. \$\endgroup\$
    – GillesDV
    Oct 1 '15 at 15:25
  • 1
    \$\begingroup\$ You really should investigate that, it's the way to go I think \$\endgroup\$
    – IEatBagels
    Oct 1 '15 at 15:27
4
\$\begingroup\$

Let's see...

    string qryAboutToBeExecuted = "DECLARE @DBuser_sql VARCHAR(4000)" + Environment.NewLine +
                "DECLARE @DBuser_table TABLE([DBName] VARCHAR(200), UserName VARCHAR(250), LoginType VARCHAR(500))" + Environment.NewLine +
                "SET @DBuser_sql = 'SELECT DISTINCT ''[?]'' AS [DBName],[?].[sys].[database_principals].[name] AS Name,[?].[sys].[database_principals].[type_desc] AS LoginType FROM [?].sys.database_principals " + Environment.NewLine +
                "WHERE [?].sys.database_principals.sid NOT IN(0x01, 0x00) AND [?].sys.database_principals.sid IS NOT NULL AND [?].sys.database_principals.type NOT IN(''C'') AND [?].sys.database_principals.is_fixed_role <> 1 " + Environment.NewLine +
                "AND [?].sys.database_principals.name NOT LIKE ''##%'' AND ''[?]'' NOT IN (''master'',''msdb'',''model'',''tempdb'') AND Name LIKE ''" + user.LoginUsername + "'' ORDER BY Name'" + Environment.NewLine +
                "INSERT @DBuser_table" + Environment.NewLine +
                "EXEC sp_MSforeachdb @command1 = @dbuser_sql" + Environment.NewLine +
                "SELECT *" + Environment.NewLine +
                "FROM @DBuser_table" + Environment.NewLine +
                "ORDER BY [DBName]";

The first thing that comes to mind is.... why does this horrible string needs to be in the code?

Make it a parameterized stored procedure and get this nastiness out of your sight!

So that's the SQL part.


StringBuilder result = new StringBuilder();

Fair enough, you're building a string; a StringBuilder looks like the right tool for the job right?

Wrong! Why are you building a string?

result.Append("<div>");
result.Append("</ul> <br />");
result.Append("<li>" + objLoginDB.DBName + "</li>");

Don't do that. I don't do much web development, but the little I've done has shown me that if you're building the HTML string output yourself, you're doing it wrong. Very wrong.

Make the SQL a stored procedure, run the stored procedure from C#, get the results into a DataTable if you have to, or an IEnumerable<SomeComplexType> if you would rather use Entity Framework, and use whatever is appropriate to pass this model to the view.


Performance-wise, this can't be helping:

foreach (DataRow row in ds.Tables[0].Rows)
{
    try
    {

Do whatever it takes to avoid throwing and catching exceptions in a loop. And you probably don't want to catch System.Exception here, but something much more specific, that you should avoid having thrown any way. If you're catching exceptions repeatedly in that loop, chances are that you have a very much avoidable bottleneck there.

Catching System.Exception is one thing; swallowing it is another - now if anything goes wrong, you have no idea what happened, where, why, or how.


This is just plain weird:

result.Append("The database permissions of the user in SQL Server Management Studio and the LoginDatabase table are perfectly synced and up to date.");

SSMS is just a client; the permissions are in the database, not in SSMS.

\$\endgroup\$
4
  • \$\begingroup\$ Thank you for the detailed reply. Allow me to give a little feedback of my own. First of all, I know that huge query in the code looks horrible, but it's kind of a necessity. The query needs to be processed multiple times over multiple servers. So I'd have to copy/paste and create that SP 12 times, which isn't that great either. Putting it in the master table isn't very clean either. As for the try...catch blocks. This method takes a large amount of time and it cannot suddenly interrupt, because there was a silly, almost unimportant exception. As for the final part, that's indeed stupid. \$\endgroup\$
    – GillesDV
    Oct 1 '15 at 15:02
  • \$\begingroup\$ That silly, unimportant exception is causing the CLR to unwind the stack and slowing the loop down. Avoid throwing it in the first place, and whatever you do don't swallow System.Exception. As for the SP, just set up your backend with linked servers, and make sure the SP can access everything it needs to access - it's not a concern that belongs in the code. \$\endgroup\$ Oct 1 '15 at 15:08
  • \$\begingroup\$ Ah I see. In this case I'm 95% certain there won't be any more SQL exception errors anymore. TimeoutException or invalid network exceptions are pretty likely though. How would I avoid swallowing them then, if you don't mind me asking? Add a throws SQLExcpetion at the top of the method? \$\endgroup\$
    – GillesDV
    Oct 1 '15 at 15:13
  • \$\begingroup\$ I would let the outer try/catch block deal with SqlExceptions. If one iteration is timing out, there's no reason to believe the next iterations won't time out as well; jumping out of the loop seems like the only sensible thing to do IMO. \$\endgroup\$ Oct 1 '15 at 15:23
2
\$\begingroup\$

Talking about performance, the try-catch block costs. I would change the way you handle errors in your code. For one example, I'd change:

try
{
    DataSet ds = myDB.executeResultDataset("DBServer" + teller, qryAboutToBeExecuted, myParams);

    // other code...
}
catch(Exception) { }

to something like:

DataSet ds = null;
try
{
    ds = myDB.executeResultDataset("DBServer" + teller, qryAboutToBeExecuted, myParams);
}
catch(Exception) { }

if(ds != null)
{
    // other code...
}

For more ways to handle exceptions I would suggest to read this article.

That being said, try to avoid creating new objects inside cycles where possible. For example:

for (int teller = 13; teller <= 25; teller++)
{
    //The brackets, [ ], are essential in the query below. Otherwise the processor will be confused with certain DB names
    string qryAboutToBeExecuted = "DECLARE @DBuser_sql VARCHAR(4000)" + Environment.NewLine +
                "DECLARE @DBuser_table TABLE([DBName] VARCHAR(200), UserName VARCHAR(250), LoginType VARCHAR(500))" + Environment.NewLine +
                "SET @DBuser_sql = 'SELECT DISTINCT ''[?]'' AS [DBName],[?].[sys].[database_principals].[name] AS Name,[?].[sys].[database_principals].[type_desc] AS LoginType FROM [?].sys.database_principals " + Environment.NewLine +
                "WHERE [?].sys.database_principals.sid NOT IN(0x01, 0x00) AND [?].sys.database_principals.sid IS NOT NULL AND [?].sys.database_principals.type NOT IN(''C'') AND [?].sys.database_principals.is_fixed_role <> 1 " + Environment.NewLine +
                "AND [?].sys.database_principals.name NOT LIKE ''##%'' AND ''[?]'' NOT IN (''master'',''msdb'',''model'',''tempdb'') AND Name LIKE ''" + user.LoginUsername + "'' ORDER BY Name'" + Environment.NewLine +
                "INSERT @DBuser_table" + Environment.NewLine +
                "EXEC sp_MSforeachdb @command1 = @dbuser_sql" + Environment.NewLine +
                "SELECT *" + Environment.NewLine +
                "FROM @DBuser_table" + Environment.NewLine +
                "ORDER BY [DBName]";

    // other code...
}

should become:

string qryAboutToBeExecuted = string.Empty;
for (int teller = 13; teller <= 25; teller++)
{
    //The brackets, [ ], are essential in the query below. Otherwise the processor will be confused with certain DB names
    qryAboutToBeExecuted = "DECLARE @DBuser_sql VARCHAR(4000)" + Environment.NewLine +
                "DECLARE @DBuser_table TABLE([DBName] VARCHAR(200), UserName VARCHAR(250), LoginType VARCHAR(500))" + Environment.NewLine +
                "SET @DBuser_sql = 'SELECT DISTINCT ''[?]'' AS [DBName],[?].[sys].[database_principals].[name] AS Name,[?].[sys].[database_principals].[type_desc] AS LoginType FROM [?].sys.database_principals " + Environment.NewLine +
                "WHERE [?].sys.database_principals.sid NOT IN(0x01, 0x00) AND [?].sys.database_principals.sid IS NOT NULL AND [?].sys.database_principals.type NOT IN(''C'') AND [?].sys.database_principals.is_fixed_role <> 1 " + Environment.NewLine +
                "AND [?].sys.database_principals.name NOT LIKE ''##%'' AND ''[?]'' NOT IN (''master'',''msdb'',''model'',''tempdb'') AND Name LIKE ''" + user.LoginUsername + "'' ORDER BY Name'" + Environment.NewLine +
                "INSERT @DBuser_table" + Environment.NewLine +
                "EXEC sp_MSforeachdb @command1 = @dbuser_sql" + Environment.NewLine +
                "SELECT *" + Environment.NewLine +
                "FROM @DBuser_table" + Environment.NewLine +
                "ORDER BY [DBName]";

    // other code...
}

Another thing, when working with strings it's better to work with the StringBuilder class and/or the string.Format method.

\$\endgroup\$
1
  • \$\begingroup\$ Alright, thank you for your reply! I'll incorporate those comments into my method. \$\endgroup\$
    – GillesDV
    Oct 1 '15 at 15:06
1
\$\begingroup\$

If I have understood this correctly you are making 12 call to one or more database servers. Could the calls be executed asynchronously or in parallel? Surely the time taken to make the request from the server is going to be the largest proportion of you runtime.

Along similar lines, is there any chance you could run a profiling tool over your code, to see where the time is spent. Even if you have to resort to writing a tick count to the console line it might be helpful.

\$\endgroup\$
1
  • \$\begingroup\$ Good point. There's about a solid 10 calls to the DB servers, which probably take most of the time, or at least a large part of it. Relatively speaking of course. I'll take a look to see if I can make those methods execute async \$\endgroup\$
    – GillesDV
    Oct 5 '15 at 6:42

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.