remove receiving EOF from backend after cancel …

[mahmoudbahaa]

since according to protocol the server closes the connection immediately once cancel is sent (connection reset exception is always thrown) and that was slowing the close considerably (the 1000 close used to take 22 seconds because of this now it takes 1-1.5 seconds on my machine. I also changed the timeout of the test from 20 seconds to 2 seconds.

quote:

The server will process this [cancel] request and then close the connection. For security reasons, no direct reply is made to the cancel request message

." https://www.postgresql.org/docs/current/protocol-flow.html

All Submissions:

• Have you followed the guidelines in our Contributing document?
• Have you checked to ensure there aren't other open Pull Requests for the same update/change?

New Feature Submissions:

1. Does your submission pass tests?
2. Does mvn checkstyle:check pass ?
3. Have you added your new test classes to an existing test suite in alphabetical order?

Changes to Existing Features:

• Does this break existing behaviour? If so please explain.
• Have you added an explanation of what your changes do and why you'd like us to include them?
• Have you written new tests for your core changes, as applicable?
• Have you successfully run tests with your changes locally?

[vlsi]

@mahmoudbahaa , this change breaks org.postgresql.test.jdbc2.StatementTest#testShortQueryTimeout

Can you please elaborate on why do you need the change?

TL;DR: I'm reverting this change (and aaccf43) in 48 hours unless there's a clarification.

---

Just in case: testShortQueryTimeout verifies the following case:

statementWithTimeout.setQueryTimeoutMs(1);

try { statementWithTimeout.executeQuery("select 1"); } catch (Throwable ignored) { }
statement.executeQuery("select 1");

Of course, statementWithTimeout might fail, however, statement must not fail with `timeout exception because the user has never set a timeout for it.

Unfortunately, we don't have query identifier in PostgreSQL, so the only thing we can do is to set cancel request to the backend, and wait for it to process.
Unfortunately^2 cancels in the backend are implemented via POSIX signals, so they are asynchronous, so noticeable time might pass until the backend receives the cancel.

cancelStream.receiveEOF(); was there to ensure backend does receive the cancel request. Of course, due to security reasons we don't know if something was canceled or not, but we do know we used the right session identifier for the cancel ticket, so we know backend tries to cancel the query.

We don't really want the following:

// start executing the query 
try { statementWithTimeout.executeQuery("select 1"); } catch (Throwable ignored) { }
// a background thread creates a timer to send `cancel` after `1ms`
// 1ms passes. The query is already completed, but the response is buffered somewhere
// background thread identifies that the query is still in progress, so it goes ahead and sends `cancel`. Note: cancel request is buffered
// query processing resumes. Client receives the result successfully
// Client sents `very_important` query
// `cancel` signal passes through the network, and it causes the backend to kill `very_important` query

So if you ever use cancel API, the connection is not really usable until the cancel is processed somehow.

If you want to quickly shut down the connection, then you need to call Connection#abort(...) instead.

[mahmoudbahaa]

@vlsi ok i got your point. but since

The server will process this request and then close the connection. For security reasons, no direct reply is made to the cancel request message.

so the cancel request never receive any EOF and even if it does it doesn't mean the request was processed. to actually make sure the request was processed we need to wait for the connection to be closed. NOT for a reply to the cancel request. that is the correct indication of whether the cancel request was processed. so if you want to you can wait for isClosed()==true or something like that

[vlsi]

cancel request never receive any EOF and even if it does it doesn't mean the request was processed

backend code does the following:

1. process cancel request
2. close the socket

Apparently, if pgjdbc detects EOF, it means the backend has processed the request.

[mahmoudbahaa]

well correct me if I am wrong but it only means the SIGINT was sent to the backend but whether it was processed by backend (SIGINT just set cancelRequested = true in backend) so actual handling of cancel can still be done after the cancel is returned.

never mind I got what you mean since sending new query reset cancelRequested to false so we will be sure that cancelRequested will not affect the next query since this way we are sure that cancelRequested is set to true before the query is sent setting it back to false.

so yes I think you should go ahead with the revert

[mahmoudbahaa]

although adding a comment to the revert so everybody would understand why it is there and don't remove it by mistake it in the future would also be a good idea