Conversation
pq's implementation of the PostgreSQL query protocol is fairly straight
forward and doesn't support running more than one query at a time.
This means that something like this:
tx, _ := db.Begin()
rows, _ := tx.Query(..)
for rows.Next() {
tx.Query(..) // Sends query while we're reading the first.
}
Will fail, as it sends a query while we're still reading the first
query.
We could use the [extended query protocol], which does allow running
more than one query without waiting for the previous one to complete.
It's an option, I guess, but reading the docs it also seems rather more
complex – I kind of like how straight-forward pq's implementation is
right now and anyone can step through it without much understanding of
PostgreSQL.
The "classic" solution for this is to just immediately read all rows in
memory, which is what many PostgreSQL drivers do. I guess we could add
an option for that (and there's a PR for it at #117). But idk, seems
easy enough to do manually? And we can't do it by default, since people
have written code against pq with the assumption that it won't read all
data in memory: so it will still catch people not aware of this.
So anyway, just change the error message to be clearer. It hasn't come
up that often in the last ~13 years so it's probably "good enough", and
it's the simplest way to improve this.
There was already inCopy to prevent other queries being run while COPY
was in progress, so expand and re-use that. We reset it when we get
ReadyForQuery from the server. Not sure if atomic.Bool is needed or just
bool will be fine? I couldn't trigger a race in my testing, but err is
protected with syncErr, and it doesn't hurt to be safe.
Note we can still do any of the above (extended query protocol, read all
in memory) later: this is not preventing that.
[extended query protocol]: https://www.postgresql.org/docs/current/protocol-flow.html#PROTOCOL-FLOW-EXT-QUERY
Fixes #81
Fixes #117
Probably fixes #855
Probably fixes #894
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
pq's implementation of the PostgreSQL query protocol is fairly straight forward and doesn't support running more than one query at a time.
This means that something like this:
Will fail, as it sends a query while we're still reading the first query.
We could use the extended query protocol, which does allow running more than one query without waiting for the previous one to complete. It's an option, I guess, but reading the docs it also seems rather more complex – I kind of like how straight-forward pq's implementation is right now and anyone can step through it without much understanding of PostgreSQL.
The "classic" solution for this is to just immediately read all rows in memory, which is what many PostgreSQL drivers do. I guess we could add an option for that (and there's a PR for it at #117). But idk, seems easy enough to do manually? And we can't do it by default, since people have written code against pq with the assumption that it won't read all data in memory: so it will still catch people not aware of this.
So anyway, just change the error message to be clearer. It hasn't come up that often in the last ~13 years so it's probably "good enough", and it's the simplest way to improve this.
There was already inCopy to prevent other queries being run while COPY was in progress, so expand and re-use that. We reset it when we get ReadyForQuery from the server. Not sure if atomic.Bool is needed or just bool will be fine? I couldn't trigger a race in my testing, but err is protected with syncErr, and it doesn't hurt to be safe.
Note we can still do any of the above (extended query protocol, read all in memory) later: this is not preventing that.
Fixes #81
Fixes #117
Probably fixes #855
Probably fixes #894