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

Always discard mysql multi-statement results #52402

Merged
merged 2 commits into from
Jul 25, 2024

Conversation

jhawthorn
Copy link
Member

@jhawthorn jhawthorn commented Jul 23, 2024

We never want to connect or reconnect here, we just need to drop any results from a multi-statement query and that only makes sense to do on the same connection we'd already used for the query.

Previously when a query was run which hit the query cache on a fresh, non-sticky connection, the connection would perform its verfiy! before calling next_result, resulting in a network round trip.

https://gist.github.com/jhawthorn/a7210ff40d947b5f6c776045a2a8c59c

cc @byroot @matthewd

@jhawthorn jhawthorn added this to the 7.2.0 milestone Jul 23, 2024
@byroot
Copy link
Member

byroot commented Jul 23, 2024

So that makes sense however:

  • Don't we have a race condition by re-acquiring the lock
  • Don't we have an issue with Async queries?

This makes me think we should instead pass some sort of parameter to select, so that it is the one that calls next_result with the lock etc.

@matthewd
Copy link
Member

Ah, yeah, it looks I "handled" this for mysql2 by just wrapping everything and grabbing the connection ahead of schedule:

with_raw_connection do |conn|
result = if ExplainRegistry.collect? && prepared_statements
unprepared_statement { super }
else
super
end
conn.abandon_results!
end

... which may or may not have its own async worries, and surely has other disadvantages like missing the log rescue's set_query.

some sort of parameter to select

Hmmm... is there actually any time we wouldn't want to drain more_results_exist?/similar? 🤔

@composerinteralia
Copy link
Member

composerinteralia commented Jul 23, 2024

which may or may not have its own ... worries

Indeed ec7f5a1 (although GitHub-specific at least at the time)

@jhawthorn
Copy link
Member Author

Hmmm... is there actually any time we wouldn't want to drain more_results_exist?/similar? 🤔

I can't find any, so let's try that as that solves both this and the two existing issues Jean points out.

Previously we were trying to clear the extra results in a
with_raw_connection block, which caused some issues.

We never want to connect or reconnect here, we just need to drop any
results from a multi-statement query and that only makes sense to do on
the same connection we'd already used for the query.

Previously when a query was run which hit the query cache on a fresh,
non-sticky connection, the connection would perform its `verfiy!` before
calling next_result, resulting in a network round trip.
Same as Trilogy, using abandon_results! needs to be done on the same
original connection, and the best way to do that is inside raw_execute.
@jhawthorn jhawthorn force-pushed the avoid_extra_pings branch from 0364a7b to 4ff28e8 Compare July 25, 2024 06:05
@jhawthorn jhawthorn changed the title Use raw connection for Trilogy more_results Always discard mysql multi-statement results Jul 25, 2024
@jhawthorn jhawthorn merged commit 0db535f into rails:main Jul 25, 2024
3 checks passed
jhawthorn added a commit that referenced this pull request Jul 25, 2024
Use raw connection for Trilogy more_results
@jhawthorn jhawthorn deleted the avoid_extra_pings branch July 25, 2024 07:44
@jhawthorn
Copy link
Member Author

Backported to 7-2-stable as 229308d

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants