Fix sql.begin() sending BEGIN on an unreserved connection - #1218
Open
v0idpwn wants to merge 2 commits into
Open
Conversation
sql.begin() could send BEGIN to the server without reserving the connection. Depending on pool size, this errored with UNSAFE_TRANSACTION or TypeError. The transaction stayed open on the server either way, and the connection went back to the pool, so unrelated queries then ran inside it. With max_pipeline: 0 this error happened consistently. With the default max_pipeline of 100 the same thing happened only when BEGIN was the query that filled a connection's pipeline, which is why the failure was intermittent. The issue is in execute(): after writing the query it returns an && chain that states whether the connection can accept more queries, and one of its terms is sent.length < max_pipeline. The onexecute hook, which is how sql.begin() reserves the connection, was the last term of that chain. Whenever the capacity term was false the chain short-circuited and the hook never ran, even though BEGIN had already been written. With max_pipeline: 0 that term is false for every query, so the hook never ran at all. Move the hook ahead of the capacity terms so it runs whenever the query was written. Reserving the connection is a consequence of having sent BEGIN, unrelated to the query limit in the connection. Add tests for BEGIN at the pipeline boundary and with max_pipeline: 0. Fixes porsager#1189
execute() must keep returning a falsy value for BEGIN so the pool puts the connection in the full queue rather than busy. Otherwise a query issued while BEGIN is in flight is pipelined behind it and runs inside the transaction.
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.
sql.begin() could send BEGIN to the server without reserving the
connection.
Depending on pool size, this errored with UNSAFE_TRANSACTION or
TypeError. The transaction stayed open on the server either way, and
the connection went back to the pool, so unrelated queries then ran
inside it.
With max_pipeline: 0 this error happened consistently. With the default
max_pipeline of 100 the same thing happened only when BEGIN was the
query that filled a connection's pipeline, which is why the failure was
intermittent.
The issue is in execute(): after writing the query it returns an &&
chain that states whether the connection can accept more queries, and
one of its terms is sent.length < max_pipeline. The onexecute hook,
which is how sql.begin() reserves the connection, was the last term of
that chain. Whenever the capacity term was false the chain
short-circuited and the hook never ran, even though BEGIN had already
been written. With max_pipeline: 0 that term is false for every query,
so the hook never ran at all.
Move the hook ahead of the capacity terms so it runs whenever the query
was written. Reserving the connection is a consequence of having sent
BEGIN, unrelated to the query limit in the connection.
Add tests for BEGIN at the pipeline boundary and with max_pipeline: 0.
Fixes #1189 #1210