Conversation
AndrewJackson2020
force-pushed
the
kill_client
branch
2 times, most recently
from
2766e3d to
634daf6
Compare
AndrewJackson2020
force-pushed
the
kill_client
branch
from
fb6eb34 to
872ae62
Compare
Added some documentation in the usage.md file. Mostly boilerplate from the |
|
@JelteF I believe that all of your feedback should be addressed in the last commit. Let me know if there is anything else you notice. |
|
While playing around with this a bit I found that pointer reuse is very common. i.e. when you connect and disconnect, both clients will have the same pointer. This makes sense because we use a That does make pointers a rather bad identifier for clients over time though. This has the downside for this feature that if you're unlucky, and the client disconnects on its own while you are trying to kill it, it's pretty likely that you will kill the next client that tries to authenticate by accident. @eulerto do you have any thoughts on this? I'm leaning towards merging this PR as-is. Creating a whole new identifier for clients (e.g. an incrementing uint64, or even a uuid) seems a bit excessive just for the functionality of killing them. Especially since people will most likely kill clients that won't disconnect on their own. And if the race condition does happen, chances are it is not the end of the world that the other new connection is killed instead. |
That's a weak identifier. I'm afraid you are killing legit client if when you are typing, the app closed that client connection and what you killed was indeed another new client. :(
I agree that it is excessive design. However, last add a caveat saying that ptr is regularly reused and you might kill another connection if the previous one terminated between SHOW CLIENTS and KILL_CLIENT. If we get various complains about this decision, it is good sign that we need to rethink this decision. Please add a comment (FIXME) about it in the code so we are constantly reminded that it can be an issue. I didn't review this PR but AFAICS it seems in good shape. |
|
Added a TODO comment above the kill_client command. Should a warning about this issue also be included in the usage.md documentation as well? |
Yeah, let's do that |
|
@JelteF I added some language in usage.md that explains the risk due to the pointer reuse. Let me know you want any other changes before this can be merged. |
|
Just opened #1172 to add a unique identifier to clients if we decide that this is something we want to address instead of relying on pointer addresses now or in the future. |
Adds a numeric identifier to PgSocket. The main reason is to use it as an unique identifier for KILL_CLIENT command (commit 1dbde96). The current identifier is the PgSocket pointer but due to the reuse nature of slab allocation, there might cancel another client (if the client connection ends between SHOW CLIENTS and KILL_CLIENT). Let's not wait for complaints as we discussed in #1147 and closes the gap now.
3 participants
This PR adds a kill_client command to the admin console. This command is the equivalent of the Postgres pg_terminate_backend() function. Similar functionality also exists in other Postgres connection poolers.
In general this functionality is a useful last resort to undisciplined users opening a lot of connections and which block out other users. This is especially prominent with front end GUI DB applications that open a new session for every SQL editor tab.