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

Added cleanup task to start of every execute. #2272

Merged
merged 10 commits into from Jan 17, 2024
Merged

Conversation

Jeffery-Wasty
Copy link
Member

@Jeffery-Wasty Jeffery-Wasty commented Dec 11, 2023

Under certain conditions, prepared statement cache can end up being filled quicker than expected, while being cleaned less often than expected. This adds additional cache clean triggers making the cleaning much more frequent.

@ecki
Copy link
Contributor

ecki commented Dec 14, 2023

Îs using force=true the right thing here? doesn’t that add roundtrips? (Besides that the change seems to address the issue but I wonder if the logic can’t be in the place where the handles are orphaned - not sure where).

@Jeffery-Wasty
Copy link
Member Author

For sure this can be improved. I have only opened this PR so the user can see our proposed changes and try testing them out, and see if that resolves the issue. For an actual fix, we will look into a cleaner and more performance-friendly solution.

@ecki
Copy link
Contributor

ecki commented Dec 14, 2023

can see our proposed changes and try testing them out, and see if that resolves the issue.

ah yes, I understand, makes sense.

it will check if it fixes the handles accumulated in my reproducer test, but of course not sure if that is the exact problem of OP

@Jeffery-Wasty
Copy link
Member Author

No we're not, but from the information we have, this is the best guess.

@David-Engel
Copy link
Contributor

For the production fix, I'd set force to false. (Assuming this is the fix.) We don't need to be super aggressive on the cleanup. It's just that if an application is doing millions of executions of a PreparedStatement and some of them occasionally result in an invalidated prepared statement handle, we should clean up periodically to avoid continuously increasing memory usage. Current cleanup only happens on close of the PreparedStatement and connection close.

@Jeffery-Wasty Jeffery-Wasty marked this pull request as ready for review January 3, 2024 00:25
@Jeffery-Wasty Jeffery-Wasty added this to In progress in MSSQL JDBC via automation Jan 3, 2024
@Jeffery-Wasty Jeffery-Wasty added this to the 12.6.0 milestone Jan 3, 2024
@Jeffery-Wasty Jeffery-Wasty linked an issue Jan 3, 2024 that may be closed by this pull request
@Jeffery-Wasty Jeffery-Wasty moved this from In progress to Under Peer Review in MSSQL JDBC Jan 9, 2024
@Jeffery-Wasty Jeffery-Wasty merged commit 829321f into main Jan 17, 2024
22 checks passed
MSSQL JDBC automation moved this from Under Peer Review to Closed/Merged PRs Jan 17, 2024
@Jeffery-Wasty Jeffery-Wasty deleted the memleakissue branch January 17, 2024 21:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
MSSQL JDBC
  
Closed/Merged PRs
Development

Successfully merging this pull request may close these issues.

PreparedStatementHandle leak leading to OOM
5 participants