Skip to content

Invoke-DbaQuery - Test the database the connection is on, not the one it was opened with - #10563

Closed
andreasjordan wants to merge 1 commit into
fix-connection-ownershipfrom
fix-invoke-dbaquery-reuse-check
Closed

Invoke-DbaQuery - Test the database the connection is on, not the one it was opened with#10563
andreasjordan wants to merge 1 commit into
fix-connection-ownershipfrom
fix-invoke-dbaquery-reuse-check

Conversation

@andreasjordan

Copy link
Copy Markdown
Collaborator

Draft: needs more testing, and waits for #10559.

Why this waits

The change sends the connection-reuse case through Connect-DbaInstance in situations where it did not go there before. On development alone that would be unsafe, because the old disconnect logic then closes the connection of the caller - which is what #10559 fixes. So this is based on the branch of #10559 and only the second commit belongs here. GitHub retargets the pull request to development once #10559 is merged.

What is wrong

To decide whether the connection that was passed in can be reused, the command compares ConnectionContext.DatabaseName with -Database. That property only holds the database the connection was opened with. Connect-DbaInstance asks the same question with ConnectionContext.CurrentDatabase, the database the connection is on right now.

The two differ as soon as anything runs a USE on the connection - which is exactly what the database-scoped SMO calls in #10555 do. The connection is then reused although it sits on another database, and the query silently runs in the wrong one:

$server = Connect-DbaInstance -SqlInstance $instance -Database tempdb -NonPooledConnection
$null = $server.ConnectionContext.ExecuteNonQuery("USE [master]")
Invoke-DbaQuery -SqlInstance $server -Database tempdb -Query "SELECT DB_NAME() AS dbname"
after a stray USE [master]     : DatabaseName=[tempdb] CurrentDatabase=[master]
Invoke-DbaQuery -Database tempdb runs in: master

Measured effect

Using @@SPID to see whether a connection was reused, on SQL Server 2022:

Connection of the caller -Database <the database it is on> before after
non-pooled, open reused, but through a needless trip into Connect-DbaInstance reused directly
pooled, closed new connection new connection, unchanged - CurrentDatabase is empty while the connection is closed

Tests

Two assertions: the connection is still reused while it is on the requested database, and a query runs in the requested database after the connection was moved away from it. Reverting the one-line change makes the second one fail with Expected: 'tempdb' But was: 'master', so it does test the change. With the change, Invoke-DbaQuery passes 31 of 31.

What still needs testing before this leaves draft

  • a full suite run, because Invoke-DbaQuery carries a large part of the module and the reuse path is used everywhere
  • Azure SQL Database, where the database of a connection cannot be changed the same way
  • availability group listeners together with -ReadOnly
  • inputs that are not a Server object: SqlConnection, connection strings and registered servers
  • the case where CurrentDatabase is empty because the connection has never been opened, which is unchanged behaviour but deserves a deliberate check rather than an assumption

This text was created by Claude and reviewed by Andreas Jordan.

… it was opened with

To decide whether the connection that was passed in can be reused, the
command compared ConnectionContext.DatabaseName with -Database. That
property only holds the database the connection was opened with. As soon as
anything runs a USE on that connection, the two differ, the connection is
reused although it is on another database now, and the query silently runs
in the wrong database:

    $server = Connect-DbaInstance -SqlInstance $instance -Database tempdb -NonPooledConnection
    $null = $server.ConnectionContext.ExecuteNonQuery("USE [master]")
    Invoke-DbaQuery -SqlInstance $server -Database tempdb -Query "SELECT DB_NAME()"
    # master

ConnectionContext.CurrentDatabase is the database the connection is on right
now, and it is also what Connect-DbaInstance compares, so both commands now
ask the same question. That also saves a needless trip through
Connect-DbaInstance when the connection is already on the wanted database.

See #10554

(do Invoke-DbaQuery)

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@andreasjordan

Copy link
Copy Markdown
Collaborator Author

Superseded by #10564, which combines this with #10559. This turned out to be a prerequisite rather than a cleanup: without it, #10559 breaks Install-DbaFirstResponderKit. Details in #10564.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant