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

database/sql: No way to tell before use if sql.Stmt is closed #20993

Closed
ncapdevi opened this issue Jul 12, 2017 · 9 comments
Closed

database/sql: No way to tell before use if sql.Stmt is closed #20993

ncapdevi opened this issue Jul 12, 2017 · 9 comments
Assignees

Comments

@ncapdevi
Copy link

@ncapdevi ncapdevi commented Jul 12, 2017

What version of Go are you using (go version)?

1.8.3

What did you do?

Performing a query on a statement that may/may not have been previously closed() can result in sql.go line 1883-1885 being hit, returning error "sql: statement is closed" it would be much more efficient if there were some way to check stmt.IsClosed() which returned stmt.closed and if it was, the user could re-prepare the statement on their own. As it stands, you would need to perform a query, check the error coming back via a string compare, then re-perform the query.

@bradfitz bradfitz changed the title No way to tell before use if sql.Stmt is closed database/sql: No way to tell before use if sql.Stmt is closed Jul 12, 2017
@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Jul 12, 2017

@kardianos kardianos self-assigned this Jul 12, 2017
@kardianos
Copy link
Contributor

@kardianos kardianos commented Jul 12, 2017

That sounds reasonable. @ncapdevi Could you post your use case where it is difficult to determine if the stmt is closed or not?

@ncapdevi
Copy link
Author

@ncapdevi ncapdevi commented Jul 12, 2017

@kardianos Sure. The general situation is that we are doing a bunch of batched DB queries/exec. We have a helper Prepare(query string) that takes in the string, correctly prepares the statement for our use case, and then puts that prepared statement into a map var preparedStatements map[string]*sql.Stmt = make(map[string]*sql.Stmt).
So every time that function is called, it first checks if it has that statement prepared, and if it does, just returns that, saving us an extra prepare, otherwise it creates it, stores it, and returns the new instance. Doing it this way has lead to 2x speed increases for operations where we need to do a bunch of the same queries over and over again. The problem is if somewhere in a function where this stored statement is being used, a stmt.Close() is called. That stored statement in the map is now unusable. This would mean that we would want to change our Prepare function to check if it's in the map, and if it IS in the map, we want to make sure it hasn't been closed, if it HAS been closed, we still want to prepare it and store that new, ready to go version.
This would be easy to do if we could just check if the stmt.closed was set to true (as far as I can tell, the boolean is only set whenstmt.Close() is called). This gives the functionality for devs to close a prepared statement that we might not be using again for awhile, while still being able to always call our same Prepare helper method.

I hope this is clear, if you need any more clarification, please let me know.

@kardianos
Copy link
Contributor

@kardianos kardianos commented Jul 13, 2017

That all sounds good. Out of curiosity, why would there by a stmt.Close() call at all?

@ncapdevi
Copy link
Author

@ncapdevi ncapdevi commented Jul 13, 2017

There shouldn't be, or at least I can't imagine that there would be, but it's a project with multiple people working on it and as you can imagine, things sneak in there sometimes. It's more precautionary than anything. My thoughts were that it should be reasonably quick to implement (would be happy to do a PR myself, but am not entirely familiar with the sql.go file so didn't want to make assumptions), so it'd be nice to have in there as a safe guard.

@kardianos
Copy link
Contributor

@kardianos kardianos commented Jul 13, 2017

I'm less thrilled with adding a "IsClosed()" type of call for a just-in-case use. But, we have pleanty of time before the go1.10 freeze to think about it.

I'd be interested in hearing other use cases from you or other organizations too. in the mean time.

@rsc
Copy link
Contributor

@rsc rsc commented Jul 13, 2017

This seems like a mistake. If you don't know whether the statement is closed, you also don't know whether someone else is going to close the statement a nanosecond after you confirm that it's open. We shouldn't encourage programs not to know fundamental details like "has someone closed this statement?" There's no equivalent for open files, for example.

@ncapdevi
Copy link
Author

@ncapdevi ncapdevi commented Jul 13, 2017

@rsc that's a fair point, but it greatly, greatly, reduces the likelihood of a problem, and even if it DOES get closed in that nanosecond in between, you'd get the same "sql: statement is closed" that is and always has existed. So you're taking care of 99.99% use cases and if not, falling back to what is already currently implemented.

@kardianos
Copy link
Contributor

@kardianos kardianos commented Sep 9, 2017

I'm going to decline this proposal. I think @rsc 's comment was appropriate. If this is a problem you can easily audit your code base for any instances of Stmt.Close with common tools.

@kardianos kardianos closed this Sep 9, 2017
@golang golang locked and limited conversation to collaborators Sep 9, 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.