Fix timeout method

[martinmacko47]

Based on PR #2527 - rebased and added the fix for postgres too:

KILL commands have to be sent on a seprate connection, and transaction Clients override acquireConnection to always return the same connection.

Furthermore, it is plausible that some applications may never be able to Acquire a pooled connection for cancellation becuase a query which is Exceeding the timeout may coincidentally exhaust the connection pool.

For example, if a DDL operation is blocking writes, a connection pool May be quickly exhaused by write operations.

Timeout cancellation during transaction is fixed by using acquireRawConnection and _query to bypass the transaction connection and cancel the query from a separate connection.

fixes #2211

[martinmacko47]

This test is a copy of ".timeout(ms, {cancel: true}) should throw TimeoutError and cancel slow query" test with just the query enveloped into a transaction.

You can use this test to confirm that canceling a timeouted query does not work inside a transaction on current master. The test will fail there. I've made as little as possible changes to the test, to make sure the cause is the transaction.

[martinmacko47]

We go around the connection pool for the cancellation query by purpose. As the pool will likely to be exhausted when queries start timeouting, which would prevent us from cancelling the query in the first place.

Therefore this test is contradictory and will fail, as an exhausted pool should not prevent the cancellation.

[kibertoad]

We can remove the test then

[kibertoad]

Do we ignore the pool limits for all dialects now, or test still makes sense for some of them?

[martinmacko47]

@kibertoad

Do we ignore the pool limits for all dialects now, or test still makes sense for some of them?

Query cancelling is supported ony for mysql and postgres, so this test was skipped for other dialects anyway:

      // Only mysql/postgres query cancelling supported for now
      if (!isMysql(knex) && !isPostgreSQL(knex)) {
        return this.skip();
      }

We can remove the test then

Maybe I'd rather turn the test arround that the query is indeed cancelled even if pool is exhausted.

[kibertoad]

Great idea!

[martinmacko47]

@kibertoad Test refactored in 944e868

[martinmacko47]

There is a bit of semantic difference between MySQL and Postgress:

• KILL QUERY ? in MySQL stops just the current query, but keeps the transaction active. So the user can send further queries to the same transaction and commit it eventually.
• SELECT pg_cancel_backend($1) in Postress stops the current query, and also automaticaly rejects the transaction. So any further queries sent to the same transaction will fail and the transaction will be eventually rolled back.

Should we keep this semantinc difference? Or should we rather manually roll back the transaction in MySQL as well. If we decide either way, it won't be a breaking change, as the query cancellation didn't work in transactions so far anyway.

[kibertoad]

@elhigu @maximelkin @briandamaged This needs a careful review.

[martinmacko47]

The CI error seems unrelated. When I run the worflow on my fork, CI passed. Perhaps just hit the re-run button (I don't have access to it, so I can't re-run the workflow for the PR).

[kibertoad]

Released in 0.95.3.