Skip to content

fix(prepared_statements): make the old PREPARE unused and allow memory reclaim - #1342

Merged
meskill merged 1 commit into
mainfrom
meskill-2026-08-10-fix-prepared_statements---nspk
Aug 13, 2026
Merged

fix(prepared_statements): make the old PREPARE unused and allow memory reclaim#1342
meskill merged 1 commit into
mainfrom
meskill-2026-08-10-fix-prepared_statements---nspk

Conversation

@meskill

@meskill meskill commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Make sure the overwrite the PREPARED doesn't leak the statement in global cache.

And other refactors of global_cache to make it more explicit how it works for simple queries

@codecov

codecov Bot commented Aug 11, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 98.07692% with 2 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
...lient/query_engine/test/rewrite_simple_prepared.rs 97.05% 1 Missing ⚠️
...g/src/frontend/prepared_statements/global_cache.rs 98.11% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

///
/// SQL `PREPARE` gets a key of its own, so it is never handed to
/// a second client. It is tracked and evicted like any other statement.
pub fn insert_anyway(&mut self, parse: &Parse) -> String {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

You may want to rename this method accordingly. insert_anyway implies bypassing the cache, it does not imply handling prepared statements sent over the simple protocol only.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

renamed to insert_prepared

///
/// 1. Mapping between unique prepared statement identifiers (query and result data types),
/// and the global unique prepared statement name used in all server connections.
/// Statements created by SQL `PREPARE` carry a key of their own, so they are

@levkk levkk Aug 11, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This is not a great implementation on our part. We should make those re-usable between clients.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@meskill
meskill force-pushed the meskill-2026-08-10-fix-prepared_statements---nspk branch from 18a3c97 to bea37e8 Compare August 11, 2026 18:21
@meskill
meskill force-pushed the meskill-2026-08-10-fix-prepared_statements---nspk branch 2 times, most recently from eb2b53d to 6d4743b Compare August 11, 2026 18:55
Base automatically changed from meskill-2026-08-08-fix-prepared_statements---wsp to main August 12, 2026 10:53
@meskill
meskill force-pushed the meskill-2026-08-10-fix-prepared_statements---nspk branch 2 times, most recently from c8ba50f to 76aef9b Compare August 12, 2026 10:55
@meskill
meskill force-pushed the meskill-2026-08-10-fix-prepared_statements---nspk branch from 76aef9b to 20be5ee Compare August 13, 2026 05:35
@meskill
meskill merged commit 376acd1 into main Aug 13, 2026
28 checks passed
@meskill
meskill deleted the meskill-2026-08-10-fix-prepared_statements---nspk branch August 13, 2026 06:31
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.

2 participants