Support passing AbortSignal in query() #2774
Comments
|
That would be a pretty nice way to cancel a query. Canceling a query with postgres is kinda a gross operation under the hood which is why support isn't great in this library. You have to create a new connection and then tell postgres to cancel a query from another connection - the active connection running the query is "unresponsive" from our app's perspective while its waiting for a query to execute, and the backend doesn't read any network data once it receives a command to execute a query. It doesn't even check the socket to make sure it's still connected (which is why if your process dies and you're running a 30 min query it will still run in postgres). So while DX the semantics of wanting to kill a stream and have it tell postgres to cancel the query are really nice - what would need to happen under the hood is actually spinning an entirely new connection to the backend, going through the connection handshake, and then telling the backend to cancel the other connection's query. Then the existing query will throw an error saying 'terminiated by backend.' It's a whole bunch of chaos. I do want to get better support for canceling queries in the near future though because yah killing your program and having a rouge query running for 30 min in the background without any way to stop it outside of going to |
|
Wow I had no idea it was so complicated!
I guess you can’t just kill the tcp connection on abort?
—
Matt Bishop
… On Jul 20, 2022, at 11:26 AM, Brian C ***@***.***> wrote:
That would be a pretty nice way to cancel a query. Canceling a query with postgres is kinda a gross operation under the hood which is why support isn't great in this library. You have to create a new connection and then tell postgres to cancel a query from another connection - the active connection running the query is "unresponsive" from our app's perspective while its waiting for a query to execute, and the backend doesn't read any network data once it receives a command to execute a query. It doesn't even check the socket to make sure it's still connected (which is why if your process dies and you're running a 30 min query it will still run in postgres). So while DX the semantics of wanting to kill a stream and have it tell postgres to cancel the query are really nice - what would need to happen under the hood is actually spinning an entirely new connection to the backend, going through the connection handshake, and then telling the backend to cancel the other connection's query. Then the existing query will throw an error saying 'terminiated by backend.' It's a whole bunch of chaos. I do want to get better support for canceling queries in the near future though because yah killing your program and having a rouge query running for 30 min in the background without any way to stop it outside of going to psql and finding and kiling it is annoying.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you authored the thread.
|
|
@brianc Hi Brian, I assume what you're talking is that being said, it would be quite nice to have such capability baked in the library. |
Yeah the async function cancel(client: pg.Client): void {
// note: need to use the same exact connection config to connect to the same backend
const newClient = new pg.Client({
host: client.host,
user: client.user,
// copy every single optional connection config for ssl and so-forth
})
await newClient.connect()
// note: this may or may-not cancel your query...you don't really have total control over that
// https://www.postgresql.org/docs/current/functions-admin.html#FUNCTIONS-ADMIN-SIGNAL
await newClient.query('SELECT PG_CANCEL_BACKEND($1)', [client.processId])
await client.end()
}Putting it on I'd need to test whether the newly created client doing the canceling needs to connect over SSL - it doesn't look like it does according to the protocol docs so all I need from the original client is Anyways...yeah the top level |
That's exactly what I would propose. The other issue I see is strange error propagation: So, ok we're cancelling a query on one side but now the client that actually initiated the query errors out with |
|
Maybe: Yuck.. but at least it .. doesn't surprise I suppose.. |
Definitely open to suggestions on a different api! |
|
An other issue with cancellation is that it can only target the connection, not a specific query (there is nothing like a query ID in the protocol).
This issue has been discussed on the different postgres mailing lists for over 20 years. Here are some relevant threads from 2003 to 2020:2020: https://postgrespro.com/list/thread-id/2515803 What I got from them is that you should lock down the client until the cancel request connection is closed by the server. And if you already have multiple queries in-flight (query pipelining?) it's not predictable which - if any - query will be canceled. Once all the in-flight queries have resolved and the cancel request connection has been closed by the server you can be sure there won't be any surprise errors caused by race conditions. |
|
Thanks @boromisp! |
@boromisp That's only true for I think there should be some pragmatism here. On the other hand, I am perplexed that so few people (relative to how many use this lib), ask for this. It's critical to have it, especially for those few times you messed up the architecture/design and have to compensate with long-running queries.
Yes - it is full of gotchas. But someone with a need to cancel queries would likely be in a position where the vast majority of the cancellations hit a running query. You don't go looking on ways to cancel a query unless you're talking about long, really, really long queries. That's an anecdote from me, sure. Btw language here is important; you say cancel but I'm not sure if you mean cancel or kill? Because they are different. Errant long queries have bitten us multiple times in prod; user streams something, it's taking more than 5 seconds, he refreshes because.. because user, the query becomes errant and a potential blocker of subsequent queries. Just a tiny bit of traffic spike and it eventually snowballs into a cascading failure. Issues like that masquerade themselves - you're lucky in a sense if you get it to fail catastrophically because you know it's there; the worse cases is where you paid tens of thousands more per year for a database you never really needed just to bring p95/99 times down. And there's other use cases, people who want to allow aborting requests as a product feature of some sort. I think it should at least be documented, in the style: look we won't do this in the library but here's a recipe for it and a couple of gotchas. There are, of course, other solutions; a connection-level |
|
I'm not against, I have the same issues with runaway queries. My current workaround simply closes the connections from the client side if the request has been aborted, and that's not ideal. More edge cases to review:
Some of this might be solvable by code (target the same IP, use the same SSL options), and the rest just needs to be called out in documentation to reduce confusion. |
There is a specific 57014 query_canceled error code (https://www.postgresql.org/docs/current/errcodes-appendix.html) you could check for in your exception handler. I'm not sure, how supressing it would work on a promise based API however. I might even introduce a new error for queries aborted before reaching the server, since And maybe add something like an To be honest, a simple, low-level And possibly an other new client API: node-postgres/packages/pg/lib/client.js Line 72 in 0096856 The application would have to be careful not to release the client back to the pool while the cancellation is pending All the other fluff could be added later if needed. |
|
That's some excellent resources and pointers. There's edge cases i wasn't aware of.
I would say so as well, right off the bat
Yes that's a must; I have to go an extra step further and put a
I don't use PGBouncer but I'm aware it's a default standard that a lot of people use; so yes certainly any implementation should support it.
I wouldn't be picky about vendor compatibility, especially if they are outliers.
No comment here, I only have 1 read replica and I point at it directly. |
By far, the most peculiar and painful handling has to be streams. I'm on my 3rd day of evaluating different strategies on how to handle streams that error, cleanup correctly and produce some OK response when they error out mid-stream. Just putting this out there. I wouldn't expect it to trigger errors when you're cancelling a query. This would be very surprising and perplexing. |
|
@nicholaswmin By streaming, do you mean something like I don't have a lot of experience with the node stream API, but a configurable error handler might work: public _read(size: number) {
this.cursor.read(size, (err: Error, rows: any[]) => {
if (err) {
if (this.errorIsEOF?.(err) this.push(null)
else this.destroy(err)
} else {
for (const row of rows) this.push(row)
if (rows.length < size) this.push(null)
}
})
}new QueryStream('SELECT ...', [], {
/* query_canceled, eg. statement_timeout or user request */
errorIsEOF: (err) => err?.code === '57014'
})I think it would be a lot more difficult to suppress errors in the core pg library without breaking things. That would involve emitting fake postgres protocol messages to simulate the end of a successful response and not confuse the likes of |
Ah man, thanks again for the pointers, so much appreciated - but I'll open another issue for that as I don't want to divert this conversation to my own issues. It's not about ending the DB stream but rather how to properly handle the errors downstream to
I see. In that case how would you handle the error that will occur in the initial query? Conditionally? By sniffing it's error code and ignoring it as an error? And is there an accurate picture of the use cases that this is requested for? Or maybe that enough bike shedding; I can jump in next week and give it a go? |
Node offers an AbortSignal API that gives async operations a way to cancel their progress if aborted by the caller. Common use cases are web apps that see their request closed before completion, or in a local app, the user hits Ctrl-C to kill a running program. node-postgres's query() command could accept an AbortSignal instance and listen to it for abort events. When an abort occurs, it would close the Postgres query immediately.
A decent writeup can be found in this logrocket blog: https://blog.logrocket.com/complete-guide-abortcontroller-node-js/
Node Stream offers an option to pass an AbortSignal instance to close streams when an abort occurs. https://nodejs.org/dist/latest-v18.x/docs/api/all.html#all_stream_streamaddabortsignalsignal-stream
The text was updated successfully, but these errors were encountered: