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

driver can drop server-side query timeout error #1075

Closed
acharis opened this issue Mar 24, 2020 · 9 comments
Closed

driver can drop server-side query timeout error #1075

acharis opened this issue Mar 24, 2020 · 9 comments

Comments

@acharis
Copy link

acharis commented Mar 24, 2020

If a query takes too long in the sense of exceeding max_execution_time then the driver should report an error. I can reproduce lacking this error.

package main

import (
 "database/sql"
 "fmt"
 "time"
 _ "github.com/go-sql-driver/mysql"
)

func main() {
 db, err := sql.Open("mysql", "username:password@tcp(1.2.3.4:3306)/dbname")
 if err != nil {
    fmt.Println(err)
    return
 }

  defer db.Close()
  start := time.Now()
  rows, err := db.Query("SELECT count(*) FROM foo")
  if err != nil {
   fmt.Println(err.Error())
   return
  }

  t := time.Now()
  elapsed := t.Sub(start)
  fmt.Println(elapsed)

  var count int64
  for rows.Next() {
    err := rows.Scan(&count)
    if err != nil {
      fmt.Println(err)
      return
    }
    fmt.Printf("count: %d\n", count)
  }
}

I expect an error after db.Query

Built against go-sql-driver/mysql commit 681ffa8

go version
go version go1.12.14 linux/amd64

mysql version: 5.7.25-28-log Percona Server (GPL), Release 28, Revision c335905

uname -a
Linux pause-pod 5.4.20-hs768.el6.x86_64 #1 SMP Tue Mar 17 16:38:55 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux
@acharis
Copy link
Author

acharis commented Mar 24, 2020

I have also reproduced this against:

go version
go version go1.13.6 linux/amd64

@methane
Copy link
Member

methane commented Mar 24, 2020

Have you checked rows.Err()?

@acharis
Copy link
Author

acharis commented Mar 24, 2020

checking that now, via:

package main

import (
 "database/sql"
 "fmt"
 "time"
 _ "github.com/go-sql-driver/mysql"
)

func main() {
 db, err := sql.Open("mysql", "vt_hsmysql_read:T}MLBFCNroXKFzR;oPZUJ2Qve@tcp(172.18.153.145:3306)/VitessTest")
 if err != nil {
    fmt.Println(err)
    return
 }

  defer db.Close()
  start := time.Now()
  rows, err := db.Query("SELECT count(*) FROM foo")
  if err != nil {
   fmt.Println(err.Error())
   return
  }

  t := time.Now()
  elapsed := t.Sub(start)
  fmt.Println(elapsed)

   err = rows.Err()
   if err != nil {
           fmt.Println(err)
           return
   }

  var count int64
  for rows.Next() {
    err := rows.Scan(&count)
    if err != nil {
      fmt.Println(err)
      return
    }
    fmt.Printf("count: %d\n", count)
  }
}

i still don't see any error.

@acharis
Copy link
Author

acharis commented Mar 24, 2020

i've also now reproduced this in v1.2.0, v1.3.0, v1.4.0 as well as the original v1.5.0
(all those reproductions against golang 1.13.6)

@acharis
Copy link
Author

acharis commented Mar 24, 2020

is that only populated upon call to Next ?

@acharis
Copy link
Author

acharis commented Mar 24, 2020

ah yeah, so we've got to call next then check for error

@acharis
Copy link
Author

acharis commented Mar 24, 2020

x-ref: github/gh-ost#822

@acharis acharis closed this as completed Mar 24, 2020
@acharis
Copy link
Author

acharis commented Mar 24, 2020

i'm closing this, b/c it seems like the user (here the gh-ost application) isn't checking for the error in the way that go-sql-driver expects, but i'll note that it's surprising to me that we'd need to call Next to be able to see this error. that feels unergonomic to me. i'd expect the error in the rows, err := db.Query line to have this timeout error.

@methane
Copy link
Member

methane commented Mar 25, 2020

but i'll note that it's surprising to me that we'd need to call Next to be able to see this error. that feels unergonomic to me.

This is low level library. It is not designed for ergonomic.
This library return error when server returned error. If you get error from rows.Err() instead of db.Query(), your server sent the error while sending result rows instead of result headers.

You can use some wrapper library if you want "ergonomic" design.
For example, see sqlx.Select.

ajm188 added a commit to ajm188/gh-ost that referenced this issue Mar 31, 2020
Closes github#822.

In go-sql-driver/mysql#1075, @acharis notes
that the way the go-sql driver is written, query timeout errors don't
get set in `rows.Err()` until _after_ a call to `rows.Next()` is made.

Because this kind of error means there will be no rows in the result
set, the `for rows.Next()` will never enter the for loop, so we must
check the value of `rows.Err()` after the loop, and surface the error up
appropriately.
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

2 participants