Code Review to find problems with too many connections

[andreasjordan]

@tboggiano asked me to have a look at the code to find problems causing too many open connections.

First of all: As you extensively use .ForEach and .Where, it is very hard to know what $psitem (or $PsItem, or the offical $PSItem) is holding at that moment. Sometimes you alias ($Instance = $psitem), but even when you do, you sometimes don't use the alias but $psitem again.

But let me come to dbatools, the main command Connect-DbaInstance and the main parameter -SqlInstance. If you use a string like "SQL01" or "SQL02\TEST" for -SqlInstance, Connect-DbaInstance will connect to that instance with either a brand new connection or a ready to use connection from the connection pool. The command will then build a SMO object and fill it with some initial values from the instance. This SMO object is returned. If you pass the same string as -SqlInstance to following dbatools commands (like you do most of the time in dbachecks), these commands all pass that string to Connect-DbaInstance which will build a brand new SMO and return it to the command to be used there. The SMO is then destroyed at the end of the command and the connection should (!) be returned to the connection pool.

To avoid building all these SMO objects, all commands can use a SMO object as -SqlInstance. This object is then passed to Connect-DbaInstance. But this time, only some additional parameters like -Database are taken into account and maybe change the connection (and yes: In some cases we have to build a brand new SMO - I will not go into detail here, just have a look at the code). At the end, the entering SMO is just returned, which is way faster and should not open new connections.

This way you can also configure the connection by using -NonPooledConnection with Connect-DbaInstance for example - which will not use connection pooling at all.

So I would suggest a complete code refactoring for all lines related to calls to dbatools commands. Depending on how "near" you would like to get to the coding standards of dbatools, I would "copy" some of their naming standards. Maybe central loops like foreach ($instance in $SqlInstance) { } or naming the SMO $server, as the name of the SMO type is "Server" (you do this in Get-DatabaseDetail). Then for every reader it is very clear that $instance may hold a string or a SMO, but $server will always hold a SMO. Then all calls to dbatools commands should use -SqlInstance $server or have a comment with details why not.
But you don't have to, you can also use your current naming $Instance and $InstanceSMO. But you should start to use $InstanceSMO when running dbatools commands instead of $psitem.

[SQLDBAWithABeard]

Thanks for this @andreasjordan - I hadnt seen that it came in.

In short

My recommendations from above are (unless @andreasjordan says I am wrong

• Add -NonPooledConnection with Connect-DbaInstance at the top of Instance, Database and Agent Check files
• Ensure all calls to dbatools in Instance, Database and Agent Check files and also in Get-AllInstanceInfo use the InstanceSmo object

Any changes will need documented proof of improvement in speed, reduction of connection pooling errors and to pass all of the AST Pester validation without significant refactoring of it.

More descriptive reasoning below

If we take Instance level checks - I think that what you are suggesting is that where we make our first call to Connect-DbaInstance here

dbachecks/checks/Instance.Tests.ps1

Line 17 in 4bfe713

$InstanceSMO = Connect-DbaInstance -SqlInstance $Instance -ErrorAction SilentlyContinue -ErrorVariable errorvar

we need to add -NonPooledConnection

Which means that the Instance SMO object passed to Get-AllInstanceInfo will not use connection pooling

dbachecks/internal/assertions/Instance.Assertions.ps1

Line 89 in 4bfe713

function Get-AllInstanceInfo {

for each of its calls to the dbatools commands would be beneficial. This builds a single object with all of the results the user ahs asked for and then the Pester validation is run against this object.

I think for the database checks we are suggesting that the call to Connect-DbaInstance here

dbachecks/checks/Database.Tests.ps1

Line 13 in 4bfe713

$InstanceSMO = Connect-DbaInstance -SqlInstance $Instance -ErrorAction SilentlyContinue -ErrorVariable errorvar

adds -nonpooledconnection

and each call to a dbatools command uses the $InstanceSMO object declaratively - I think that this file will show the most improvement.

It would be nice to build an object for testing for databases in the same way as was done for the Instances but for right now that be a low-hanging fruit

The Agent Checks are even further behind in coding but would benefit from either the use of InstanceSmo object with none-pooled connection and/or a "testing" object being created

I think the HADR tests are in better shape as they do gather a testing object (this ignores some of the other glaring errors with this set of tests)

The other checks are of lesser performance worries right now.

We do use the methods because they were quicker in testing when the code was written, and we were able to update the minimum PS version requirement in relation to dbatools.

There is also the added complication of the looping being inside of a Pester instance (thats the wrong word for v4) which makes for all sorts of scoping issues.

I would be unwilling to accept the large amount of refactoring that would be required for every check file and the associated Pester test rewrites at present. This would be better done when a rewrite for Pester v5 was implemented.

Coding standards and indeed naming standards are highly enforced by the Pester validation of the AST as to ensure that both versions of the Power Bi file and the storing the results in the database commands work correctly requires some frustratingly annoying regex so I am unwilling to alter any of the current settings without a significant amount of testing.

for example -

dbachecks/tests/Unit.Tests.ps1

Line 13 in 4bfe713

Describe "Checking that each dbachecks Pester test is correctly formatted for Power Bi and Coded correctly" -Tags UnitTest {