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

release-23.1: sql: add prepared_statements_cache_size setting #99254

Merged
merged 1 commit into from
Mar 28, 2023

Conversation

michae2
Copy link
Collaborator

@michae2 michae2 commented Mar 22, 2023

Backport 1/1 commits from #98917.

/cc @cockroachdb/release

Release justification: high priority business need for the functionality (fix for customer).


Add a new circular doubly-linked list of prepared statements to prepStmtNamespace which tracks the least-recently-used prepared statement. When new setting prepared_statements_cache_size is set, use this LRU list to automatically deallocate prepared statements.

Fixes: #97866

Epic: None

Release note (sql change): Add a new prepared_statements_cache_size setting which, when set to a non-zero number of bytes, causes the least-recently-used prepared statements to be automatically deallocated when prepared statement memory usage goes above the cache size. This setting can be used to avoid prepared statement leaks from long-lived connections which never DEALLOCATE prepared statements.

@michae2 michae2 requested a review from a team as a code owner March 22, 2023 16:44
@blathers-crl
Copy link

blathers-crl bot commented Mar 22, 2023

Thanks for opening a backport.

Please check the backport criteria before merging:

  • Patches should only be created for serious issues or test-only changes.
  • Patches should not break backwards-compatibility.
  • Patches should change as little code as possible.
  • Patches should not change on-disk formats or node communication protocols.
  • Patches should not add new functionality.
  • Patches must not add, edit, or otherwise modify cluster versions; or add version gates.
If some of the basic criteria cannot be satisfied, ensure that the exceptional criteria are satisfied within.
  • There is a high priority need for the functionality that cannot wait until the next release and is difficult to address in another way.
  • The new functionality is additive-only and only runs for clusters which have specifically “opted in” to it (e.g. by a cluster setting).
  • New code is protected by a conditional check that is trivial to verify and ensures that it only runs for opt-in clusters.
  • The PM and TL on the team that owns the changed code have signed off that the change obeys the above rules.

Add a brief release justification to the body of your PR to justify this backport.

Some other things to consider:

  • What did we do to ensure that a user that doesn’t know & care about this backport, has no idea that it happened?
  • Will this work in a cluster of mixed patch versions? Did we test that?
  • If a user upgrades a patch version, uses this feature, and then downgrades, what happens?

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@michae2
Copy link
Collaborator Author

michae2 commented Mar 22, 2023

(Hold off on reviewing just yet.)

@michae2
Copy link
Collaborator Author

michae2 commented Mar 27, 2023

I don't feel strongly about enabling this by default, but I would like to get it into 23.1, so I think this is RFAL.

Copy link
Member

@yuzefovich yuzefovich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a strong opinion either about the default. Perhaps we'll wait to see whether we get another report where the unlimited memory usage of prepared statements is a problem, and if it happens, then we switch the default. At least now (assuming recent enough version) we should have decent observability in place to investigate this problem.

I do agree with you that we should get this into 23.1.0.

Also didn't review too carefully.

Reviewed 16 of 16 files at r1, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @mgartner)

@michae2 michae2 requested a review from rytaft March 28, 2023 17:03
@michae2
Copy link
Collaborator Author

michae2 commented Mar 28, 2023

I'm hoping to merge this to release-23.1 before #99663 (which hasn't yet been committed to master, but will be soon, I think).

@mgartner
Copy link
Collaborator

LGTM. I think leaving it off by default for v23.1 is good.

Add a new circular doubly-linked list of prepared statements to
`prepStmtNamespace` which tracks the least-recently-used prepared
statement. When new setting `prepared_statements_cache_size` is set,
use this LRU list to automatically deallocate prepared statements.

Fixes: cockroachdb#97866

Epic: None

Release note (sql change): Add a new `prepared_statements_cache_size`
setting which, when set to a non-zero number of bytes, causes the least-
recently-used prepared statements to be automatically deallocated when
prepared statement memory usage goes above the cache size. This setting
can be used to avoid prepared statement leaks from long-lived
connections which never `DEALLOCATE` prepared statements.
Copy link
Collaborator

@rytaft rytaft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @mgartner and @yuzefovich)

@michae2 michae2 merged commit a34af06 into cockroachdb:release-23.1 Mar 28, 2023
@michae2 michae2 deleted the backport23.1-98917 branch March 28, 2023 21:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants