Skip to content

Commit

Permalink
Next() should wait while BUSY or LOCKED because return value is bool
Browse files Browse the repository at this point in the history
  • Loading branch information
mattn committed Mar 19, 2015
1 parent 3080677 commit 5253daf
Showing 1 changed file with 17 additions and 9 deletions.
26 changes: 17 additions & 9 deletions sqlite3.go
Expand Up @@ -463,12 +463,14 @@ func (r *SQLiteResult) RowsAffected() (int64, error) {
func (s *SQLiteStmt) Exec(args []driver.Value) (driver.Result, error) {
if err := s.bind(args); err != nil {
C.sqlite3_reset(s.s)
C.sqlite3_clear_bindings(s.s)
return nil, err
}
rv := C.sqlite3_step(s.s)
if rv != C.SQLITE_ROW && rv != C.SQLITE_OK && rv != C.SQLITE_DONE {
err := s.c.lastError()
C.sqlite3_reset(s.s)
C.sqlite3_clear_bindings(s.s)
return nil, err
}

Expand Down Expand Up @@ -507,16 +509,22 @@ func (rc *SQLiteRows) Columns() []string {

// Move cursor to next.
func (rc *SQLiteRows) Next(dest []driver.Value) error {
rv := C.sqlite3_step(rc.s.s)
if rv == C.SQLITE_DONE {
return io.EOF
}
if rv != C.SQLITE_ROW {
rv = C.sqlite3_reset(rc.s.s)
if rv != C.SQLITE_OK {
return rc.s.c.lastError()
for {
rv := C.sqlite3_step(rc.s.s)
if rv == C.SQLITE_DONE {
return io.EOF
}
if rv == C.SQLITE_ROW {
break
}
if rv != C.SQLITE_BUSY && rv != C.SQLITE_LOCKED {

This comment has been minimized.

Copy link
@gwenn

gwenn Mar 20, 2015

http://sqlite.org/c3ref/step.html:
SQLITE_BUSY means that the database engine was unable to acquire the database locks it needs to do its job. If the statement is a COMMIT or occurs outside of an explicit transaction, then you can retry the statement. If the statement is not a COMMIT and occurs within an explicit transaction then you should rollback the transaction before continuing.

This comment has been minimized.

Copy link
@mattn

mattn Mar 21, 2015

Author Owner

Could you please send me a PR?

This comment has been minimized.

Copy link
@gwenn

gwenn Mar 21, 2015

There is no PR needed: the bug is in the example (#185).
When I fix the gist and revert this patch, I can see this log:

thread21: query gave error "database is locked"
err := rc.s.c.lastError()
C.sqlite3_reset(rc.s.s)
if rc.nc > 0 {
C.sqlite3_clear_bindings(rc.s.s)
}
return err
}
return nil
}

if rc.decltype == nil {
Expand Down

10 comments on commit 5253daf

@hallyn
Copy link
Contributor

@hallyn hallyn commented on 5253daf Mar 21, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gwenn, I think he meant a PR to revert this commit. (Too bad about the extra boilerplate in all callers, but indeed it looks like this commit isn't needed or appropriate then - at least unless/until sqlite3 offers automatic retry-until-not-locked-with-timeout).

@mattn
Copy link
Owner Author

@mattn mattn commented on 5253daf Mar 21, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But I'm thinking go-sqlite3 can retry if query parameter in DSN is set.

@gwenn
Copy link

@gwenn gwenn commented on 5253daf Mar 21, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I am wrong but the driver should not retry indefinitely by default.
You can increase the busy timeout (by calling sqlite3_busy_timeout).
Or you can attach a custom sqlite3_busy_handler which never returns zero.
For SQLITE_LOCKED, the problem still remains.

@mattn
Copy link
Owner Author

@mattn mattn commented on 5253daf Mar 21, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In few days ago, I received e-mail from someone. He says: 'we must write code to retry for sqlite3'. So we decide to change.

@gwenn
Copy link

@gwenn gwenn commented on 5253daf Mar 21, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Retries are already done by sqlite3_busy_timeout...

@mattn
Copy link
Owner Author

@mattn mattn commented on 5253daf Mar 21, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, okay. I'll provide way to change the value of sqlite3_busy_timeout.

@gwenn
Copy link

@gwenn gwenn commented on 5253daf Mar 21, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

http://sqlite.1065341.n5.nabble.com/sqlite3-busy-timeout-tp2711p2712.html

But if you set up sqlite3_busy_timeout, SQLite will automatically retry your
operation several times before erroring out. Hopefully the writing
transaction completes before the timeout has expired, and your select is
allowed to proceed.

@mattn
Copy link
Owner Author

@mattn mattn commented on 5253daf Mar 21, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do it works against SQLITE_LOCKED also?

@mattn
Copy link
Owner Author

@mattn mattn commented on 5253daf Mar 21, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll revert this.

@gwenn
Copy link

@gwenn gwenn commented on 5253daf Mar 21, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, sqlite_busy_* doesn't work against SQLITE_LOCKED. But SQLITE_LOCKED should not happen with database/sqlexcept in shared cache mode.

Please sign in to comment.