Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expose ProcessId (SPID) as a public property on SqlConnection #657

Closed
shueybubbles opened this issue Jul 20, 2020 · 20 comments · Fixed by #660
Closed

Expose ProcessId (SPID) as a public property on SqlConnection #657

shueybubbles opened this issue Jul 20, 2020 · 20 comments · Fixed by #660
Labels
💡 Enhancement New feature request

Comments

@shueybubbles
Copy link

Is your feature request related to a problem? Please describe.

SSMS tries to keep track of the current SPID in each query editor window. The SPID can change between idle connection opens or even between queries, especially on Azure SQL DB. This extra query can be time consuming and it exposes SSMS to potential SQL exceptions that are unrelated to the user's actual query.

Describe the solution you'd like

If possible, expose the SPID used for the most recent command/Open call as a public property on SqlConnection.

@JRahnama
Copy link
Member

@shueybubbles we will get back to you soon regarding this issue.

@cheenamalhotra cheenamalhotra added the 💡 Enhancement New feature request label Jul 20, 2020
@Wraith2
Copy link
Contributor

Wraith2 commented Jul 21, 2020

SSMS tries to keep track of the current SPID in each query editor window.

Why?
The only time you know the current spid is when you've just executed a command that returns it, anytime after the command completes the inner connection could be swapped out underneath the connection. It isn't a stable identifier on the client side.

@shueybubbles
Copy link
Author

I haven't been with the SQL team long enough to know "why" SSMS displays the SPID. I just know the extra query it runs (on the ui thread, no less) is a source of problems and I'd like to get rid of it. I don't know how many SSMS users would complain if the SPID display went away completely; we have millions of users and when things change they tend to complain a lot.

@roji
Copy link
Member

roji commented Jul 21, 2020

IMHO sounds like a question to take up with SSMS rather than introduce a new public API on SqlClient whose utility isn't really known... Also not sure if SSMS is built over Microsoft.Data.SqlClient (as opposed to S.D.SqlClient or even native).

@shueybubbles
Copy link
Author

SSMS is currently built on System.Data.SqlClient but I want to move it to Microsoft.Data.SqlClient eventually, which is a big undertaking. At the end of the journey I want it to be more reliable, not less. Given how many users SSMS has, if some changes to M.D.S improve SSMS reliability, it will have a huge benefit for productivity in our industry. If somebody else's app doesn't care about SPID they can just not use the new property.

@Wraith2
Copy link
Contributor

Wraith2 commented Jul 21, 2020

I use SMSS quite a lot and as far as I know the only reason you want to know the spid is so you can work out what query you're running to investigate why it's not finishing as quickly as you thought, usually with sp_who or sp_who2 or other query along those lines. The problem is of course that if you don't show the id in the ui you'd have to stop the query to work out which spid it corresponded to and at that point you'd have lost any work.

The problem i see is the persistence. In the "old" days where you could be sure you were talking to an on-premise and fairly local instance the tcp connection was very unlikely to ever drop so asking for the spid and presenting that over a long period of time wasn't likely to be a problem. These days with azure and more distribution and failover capabilities the result isn't as reliable and SQLClient is going to try and make you think you've got the same connection even when you haven't. It's only moderately more possible for it to be wrong now but given how important it is "let me find my connection spid and kill it from another connection" actually kills the right thing it might be worth seeing if it can be replaced.

Is the spid directly queried at the moment or is it a byproduct of some other execution?

@roji
Copy link
Member

roji commented Jul 21, 2020

If somebody else's app doesn't care about SPID they can just not use the new property.

IMHO that can easily lead to a lot of API/feature creep - every new API has a price in terms of maintenance, user discoverability, etc. New stuff definitely shouldn't just get added without careful thought just because some product might benefit from it, when there are other ways to achieve the same thing.

As @Wraith2 wrote (I think), rather than debating this it's important to understand if the SPID is something that's automatically sent back with for every query, as part of the protocol. If so, it may make sense to expose that via an API. If not, and this requires an additional explicit query (as I suspect), then I don't really understand what SqlClient would do here - make the additional roundtrip automatically?

On last thought - assuming this is a separate SQL query, SSMS could conceivably just append it as part of the same batch with the user's query, removing the need for an additional roundtrip.

Note that I'm just stating an opinion here, the SqlClient team would be the ones making the decision here.

@shueybubbles
Copy link
Author

Today, when the user presses ctrl-e or f5 in the query window, SSMS first issues a query for the spid on the connection, blocking the ui thread until it finishes. Once it has the result of that query, it issues the user's query on the same connection, on a background worker.
My ask is that if TDS provides the SPID to the client in the protocol as part of the query result or error, the client should expose it so SSMS can just grab it after the user's query completes. That way there's assurance that the spid for the "last run query" is accurate, at least.

BTW - we did try having SSMS append requests for spid and trancount to the user's query in one of our 17.x releases, and users got mad. DBAs like to keep an eye on the last executed query from their SSMS users and suddenly the last batch from everyone was "select @@trancount" instead of part of their real query. Also, those injected batches had some negative side effects on users who like to keep open transactions and temp tables around between query runs.

@roji
Copy link
Member

roji commented Jul 21, 2020

@shueybubbles did not realize you were an engineer on SSMS, sorry.

Do we know whether the SPID is part of the protocol? That seems to be the most relevant question here...

@Wraith2
Copy link
Contributor

Wraith2 commented Jul 21, 2020

Just taking a look at the TDS spec and it looks like spid is part of the packet header so as soon as any data is received from the server you'll know what the active query spid is without needing to directly ask for it. It's probably possible to make that available through SqlConnection somehow but I don't know how you'd be query it in synchronous scenarios.

[edit]
and of course if you rely on getting a packet back then you have to wait for a response from the server

@roji
Copy link
Member

roji commented Jul 21, 2020

and of course if you rely on getting a packet back then you have to wait for a response from the server

That's a very good point :)

@Wraith2
Copy link
Contributor

Wraith2 commented Jul 21, 2020

It might be a good use-case for batches, first send the spid query then the user query and then process the spid response back into the ui then wait for user results. It's about as close a correlation as you can reasonably get until you see a packet from the user query. Might be too complicated though.

[edit] batching issue #19

@cheenamalhotra
Copy link
Member

Hi @Wraith2 @roji

As @Wraith2 wrote (I think), rather than debating this it's important to understand if the SPID is something that's automatically sent back with for every query, as part of the protocol. If so, it may make sense to expose that via an API.

Yes, SPID is available in Packet Header with every response packet after pre-login and corresponds to Connection's process ID on the server. There is no extra query required to retrieve this info. It would be transparently made available as connection's property as requested.

As it's server's process id, it will change with connection resiliency, but I don't think there's any concern here, but something good to be aware of and can be used productively at client side.

Also to mention, client should not send the same SPID when requesting a new connection implicitly. As it's server's process ID, it is possible that server allocates the process ID to some other connection in the mean time. It would be best to keep it simple and only provide as a readonly property for grabbing current connection/query's SPID when needed.

@Wraith2
Copy link
Contributor

Wraith2 commented Jul 21, 2020

Can the spid change if the internal connection changes due to reconnection or is it stable in the scenario? If it can't change even if the transport level changes then it's safe to present I to the user through a property as requested I think. If it can change then we'd need to be able to tell users that it had changed.

@cheenamalhotra
Copy link
Member

The value of SPID depends on server, to which ID SQL Server will allocate on reconnect. It may/may not be same.

I think that's also an expectation.
It should match SQL Server's allocated SPID. If it doesn't update to match server's SPID, then there's an issue.

I also just added a test to validate the same in my PR.

@cheenamalhotra cheenamalhotra added this to To do in SqlClient v2.1 via automation Jul 21, 2020
@cheenamalhotra cheenamalhotra moved this from To do to Review in progress in SqlClient v2.1 Jul 21, 2020
@Wraith2
Copy link
Contributor

Wraith2 commented Jul 21, 2020

Oh, there's a PR? that was fast.
If the connection spid changes due to resiliency the user has no way to know they may need to update the spid in the ui, a ServeRProcessIDChanged event would allow them to.

@shueybubbles
Copy link
Author

meh. The SSMS model is "run a query then check the spid and display it". If SqlConnection were to expose events, I'd much prefer something richer like "QueryStarted" and "QueryCompleted" and the like. Then any UI would update its state or log traces based on the arguments to those event handlers.
For an app like SSMS, global static events would be even nicer. I don't really want to have to create a trace listener to get all the events and filter them out if I want to log SQL queries to my Output window to help CSS and customers debug problems. But that will be a new issue :-)

@Wraith2
Copy link
Contributor

Wraith2 commented Jul 21, 2020

If it's in-proc then Activities would probably useful since they don't require all the work with listeners that you have with event tracing. There are some activities in the netcore build and If they aren't in netfx they can be backported as long as the dependency is acceptable.

@cheenamalhotra cheenamalhotra linked a pull request Jul 21, 2020 that will close this issue
@cheenamalhotra
Copy link
Member

I think raising events would need a better design from API perspective and can be taken up separately if someone really wants it.

As we know in the past, for BulkCopy "RowsCopied" had an associated event and it was not useful for most of the users. They used reflection to access the property directly all the time. It was a big trouble for many users and finally was solved recently.

So the audience is definitely moving away from event handling and wants direct handle of property to use it themselves.

@Wraith2
Copy link
Contributor

Wraith2 commented Jul 21, 2020

Fair enough 👍

SqlClient v2.1 automation moved this from Review in progress to Done Jul 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💡 Enhancement New feature request
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

5 participants