Skip to content

Weird behaviour with query cancellation using context #863

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

Closed
pjebs opened this issue Oct 2, 2018 · 19 comments
Closed

Weird behaviour with query cancellation using context #863

pjebs opened this issue Oct 2, 2018 · 19 comments

Comments

@pjebs
Copy link

pjebs commented Oct 2, 2018

Server: MySQL 5.6

Stored Proc:

DELIMITER //
CREATE PROCEDURE slowInsert(IN t int)
BEGIN       
       SELECT SLEEP(t);
       INSERT INTO `table_x` (message) VALUES (UUID());
END //
DELIMITER ;

Go Code:

package main

import (
    "context"
    "database/sql"
    "time"

    _ "github.com/go-sql-driver/mysql"
)

func main() {
    db, err := sql.Open("mysql", "<url>")
    if err != nil {
        panic(err)
    }
    db.SetConnMaxLifetime(9 * time.Second)
    db.SetMaxIdleConns(12)
    db.SetMaxOpenConns(12)

    ctx := context.Background()
    ctx, cancel := context.WithTimeout(ctx, time.Duration(3)*time.Second)
    defer cancel()

    _, err = db.ExecContext(ctx, "call slowInsert( 10 )") // context will cancel before insert occurs
    if err != nil {
        panic(err)
    }
}
@pjebs
Copy link
Author

pjebs commented Oct 2, 2018

We are encountering weird behaviour.
When we run slowInsert( 10 ) on a mysql client (eg. sequel pro), the 10 second wait is respected before insertion. We have no reason to believe that the stored proc is wrong or invalid.

When we run the Go code above, the context is cancelling after 3 seconds -- all good.
... But immediately the insertion occurs. (It doesn't wait 10 seconds).

@pjebs
Copy link
Author

pjebs commented Oct 2, 2018

Now, if we change the stored proc so that it's:

DELIMITER //
CREATE PROCEDURE slowInsert(IN t int)
BEGIN       
       SELECT SLEEP(t/2); // <---- The change
       SELECT SLEEP(t/2); // <---- The change
       INSERT INTO `table_x` (message) VALUES (UUID());
END //
DELIMITER ;

The Go code works perfectly and cancels the query (i.e. no insertion happens).
In both cases, our mysql client (sequel pro) respects the wait period hence both stored procs are equivalent.

But in our Go code, the second stored proc works but the first does not - leading to our belief that there is some weird bug in the driver.

@pjebs
Copy link
Author

pjebs commented Oct 2, 2018

@alessio-palumbo

@methane
Copy link
Member

methane commented Oct 2, 2018

It's not a bug. We don't support cancelling query yet.
And SLEEP() is very special query. It checks connection is alive or not.
In general case, MySQL doesn't check connection until query is finished.

@pjebs
Copy link
Author

pjebs commented Oct 2, 2018

The Readme: https://github.com/go-sql-driver/mysql#contextcontext-support

Go 1.8 added database/sql support for context.Context. This driver supports query timeouts and cancellation via contexts. See context support in the database/sql package for more details.

@methane
Copy link
Member

methane commented Oct 2, 2018

OK, README is misleading. It just cancelling "waiting" query result, not query execution.

MySQL protocol doesn't provide safe way to cancel query execution.
Some CLI supports cancelling, but it's not safe / stable on some environment.

@pjebs
Copy link
Author

pjebs commented Oct 2, 2018

thanks

@methane
Copy link
Member

methane commented Oct 2, 2018

What you should do is using transaction. If you don't commit, it's rollbacked.
So timeout while transaction is safe.

@pjebs
Copy link
Author

pjebs commented Oct 2, 2018

@pjebs pjebs closed this as completed Oct 2, 2018
@pjebs pjebs reopened this Oct 2, 2018
@pjebs
Copy link
Author

pjebs commented Oct 2, 2018

Why does first storedProc insert after timeout but second storedProc does not insert. I'd assume that based on your statement, it either inserts or it doesn't. If the driver doesn't support cancelling query execution, why does it seem to cancel the execution?

@methane
Copy link
Member

methane commented Oct 2, 2018

It's MySQL behavior. I don't know much. But SLEEP() has very strange behavior when connection is closed.

@pjebs
Copy link
Author

pjebs commented Oct 8, 2018

@julienschmidt what do you think of this idea: src-d/gitbase-web#241 (comment)

@julienschmidt
Copy link
Member

We might be able to use SHOW FULL PROCESSLIST and KILL to send a KILL to the right query.

Unfortunately that can't be implemented on the driver level easily. It would require a 2nd connection on which the driver executes these commands. That could be either a shared "management connection" or connections opened on demand.

First of all, this would make the driver much more complex. Right now all the handling of sessions and connections is done by the database/sql package, not the driver, which just provides the functions to open a new connection etc.

But much more problematic is, that we have actually no way to find the right server (unless there is just one). The driver is largely deployed with mysql load-balancing proxies. In that case, it is likely hat SHOW FULL PROCESSLIST is executed on the wrong server and we can't find the query.

@pjebs
Copy link
Author

pjebs commented Oct 8, 2018

Would you be able to make a Go 2 proposal to modify what ever is required to database/sql to make a query truly cancellable?

@julienschmidt
Copy link
Member

julienschmidt commented Oct 8, 2018

I honestly think that not database/sql or this driver is the problem here, but simple MySQL itself, which provides no simple way to cancel a running query.

A good start would be if the reply to queries sent to the server would contain the process id.

In the case of a single server setup, we could then cancel queries in a similar manner to Postgres: Open a new connection and immediately send the KILL command.

The case with some proxy is more difficult. Right now such proxies are completely transparent to the driver. What would be required is some sort of mechanism to guarantee that a connection is opened to the same backend as another existing connection.

@julienschmidt
Copy link
Member

We actually have an open PR for a similar approach (for the single server setup case) already: #791

@pjebs
Copy link
Author

pjebs commented Oct 8, 2018

I'm going to submit a feature request with MySQL. If I ask for queries to contain the process id, would that create a backwards-incompatible change that they will flat out refuse?

@methane
Copy link
Member

methane commented Oct 9, 2018

It's the not only problem about cancelling. There are some other issues.
There are no any "reliable cancel on remote" protocol on the earth.
It should be used only for saving some CPUs.

Anyway, what is the point of this issue?
Weird behavior is not avoided by implementing cancellation.
You can not know query " canceled by context" is executed or canceled in MySQL server. It's timing issue.
Only you can do is using transaction; all query will be rolled back.

If we really need issue for "Implement cancelling query", file an new issue and close this.

@methane methane closed this as completed Oct 21, 2018
@pjebs
Copy link
Author

pjebs commented Oct 31, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants