Skip to content
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

Use golang's context to cancel query, rather than proactively check a killed/canceled flag #58102

Open
lance6716 opened this issue Dec 9, 2024 · 1 comment
Labels
type/enhancement The issue or PR belongs to an enhancement.

Comments

@lance6716
Copy link
Contributor

Enhancement

as title.

pros:

  • golang runtime and libraries have adopted the style that context.Context is the first parameters in function arguments. We don't need to proactively check the query is killed/canceled frequently. In many functions from official SDK or 3-rd party libraries, invalid context will cause the function return error and break the workflow. In fact, we often forget to proactively check the killed/canceled flag and have caused many problems like can't kill query for CHECK_CONSTRAINTS #58080 non-dist-task fast-reorg ADD INDEX can't be canceled timely #56017
  • after golang 1.20, we can return specific error to https://pkg.go.dev/context#CancelCauseFunc , so the reasons of
    func (killer *SQLKiller) getKillError(status killSignal) error {
    switch status {
    case QueryInterrupted:
    return exeerrors.ErrQueryInterrupted.GenWithStackByArgs()
    case MaxExecTimeExceeded:
    return exeerrors.ErrMaxExecTimeExceeded.GenWithStackByArgs()
    case QueryMemoryExceeded:
    return exeerrors.ErrMemoryExceedForQuery.GenWithStackByArgs(killer.ConnID.Load())
    case ServerMemoryExceeded:
    return exeerrors.ErrMemoryExceedForInstance.GenWithStackByArgs(killer.ConnID.Load())
    case RunawayQueryExceeded:
    return exeerrors.ErrResourceGroupQueryRunawayInterrupted.FastGenByArgs("runaway exceed tidb side")
    default:
    }
    return nil
    can be passed in context way

cons:

  • the builtin style of CancelCauseFunc requires to use https://pkg.go.dev/context#Cause to find the error, but we are more familiar with ctx.Err(). There should be some work to rewrite the usage of ctx.Err()
  • the builtin https://pkg.go.dev/context#WithCancelCause uses a lock to guard the cancel cause. So if multiple goroutines access it, it's slower than the killed/canceled flag which is an atomic variable. Given these 2 drawbacks, we may want to implement our own WithCancelCauseFast function
@lance6716 lance6716 added the type/enhancement The issue or PR belongs to an enhancement. label Dec 9, 2024
@lance6716
Copy link
Contributor Author

lance6716 commented Dec 10, 2024

tidb/pkg/server/server.go

Lines 959 to 965 in d6b313f

conn.mu.RLock()
cancelFunc := conn.mu.cancelFunc
conn.mu.RUnlock()
if cancelFunc != nil {
cancelFunc()
}

context is already canceled, but there still be many reports of query stuck. Maybe they didn't use correct context

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/enhancement The issue or PR belongs to an enhancement.
Projects
None yet
Development

No branches or pull requests

1 participant