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

proposal: database/sql: add a sync.RWLock to Rows so we can invoke .Next() concurrently #21329

Closed
pjebs opened this issue Aug 6, 2017 · 5 comments

Comments

@pjebs
Copy link
Contributor

@pjebs pjebs commented Aug 6, 2017

Can we put a sync.RWLock inside Rows so we can call rows.Next concurrently?
That way we can process the results in separate goroutines instead of processing them sequentially.

Sample code of how it's done now:

rows, err := db.Query("SELECT name FROM users WHERE age=?", age)
if err != nil {
        log.Fatal(err)
}
defer rows.Close()
for rows.Next() { //Maybe we can do this concurrently instead of processing the stuff inside concurrently
        var name string
        if err := rows.Scan(&name); err != nil {
                log.Fatal(err)
        }
        fmt.Printf("%s is %d\n", name, age)
}
if err := rows.Err(); err != nil {
        log.Fatal(err)
}
@odeke-em odeke-em changed the title database/sql row.Next() proposal: database/sql: add a sync.RWLock to Rows so we can invoke .Close() concurrently Aug 7, 2017
@gopherbot gopherbot added this to the Proposal milestone Aug 7, 2017
@gopherbot gopherbot added the Proposal label Aug 7, 2017
@odeke-em
Copy link
Member

@odeke-em odeke-em commented Aug 7, 2017

@pjebs thanks for the issue! I've updated the title a bit to try to reflect your intent, but the body could use some examples in order to make it a proposal that can be reviewed easily.

You showed how rows are processed sequentially right now, what will the processing look like if the sync.RWLock is added and then rows can be processed concurrently?

@pjebs pjebs changed the title proposal: database/sql: add a sync.RWLock to Rows so we can invoke .Close() concurrently proposal: database/sql: add a sync.RWLock to Rows so we can invoke .Next() concurrently Aug 7, 2017
@pjebs
Copy link
Contributor Author

@pjebs pjebs commented Aug 7, 2017

I don't necessarily have a good idea. I just know that the key to doing it is a sync.RWLock.

In this example: https://stackoverflow.com/questions/33661442/how-to-concurrently-iterate-through-an-sql-result-set-in-golang

It shows that the best we can do is process the stuff inside the loop concurrently instead of doing everything concurrently.

@cznic
Copy link
Contributor

@cznic cznic commented Aug 7, 2017

Do the locking in your client code instead of slowing down all non-concurrent clients.

@rsc
Copy link
Contributor

@rsc rsc commented Aug 7, 2017

@pjebs:

It shows that the best we can do is process the stuff inside the loop concurrently instead of doing everything concurrently.

And why is that not good enough? Ultimately the SQL database is handing back results one at a time. Even if we went through the effort of allowing concurrent calls to Next, they'd have to coordinate with a Mutex (not an RWMutex) to get access to the actual rows. Doing that in your code is going to run just as well, without having to introduce API complexity for all users.

@rsc
Copy link
Contributor

@rsc rsc commented Aug 14, 2017

Closed in absence of a reason this must be inside database/sql (as opposed to outside, as I outlined in the last message).

@rsc rsc closed this Aug 14, 2017
@golang golang locked and limited conversation to collaborators Aug 14, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.