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

Introduce StatementCache #113

Merged
merged 46 commits into from
May 19, 2016
Merged

Introduce StatementCache #113

merged 46 commits into from
May 19, 2016

Conversation

jgallagher
Copy link
Contributor

Mostly #61, with fixes for the latest merged PRs.

@gwenn I made a couple of minor changes here; the only public-facing one is removing cacheable and instead having a consuming method discard(). I think this feels a little nicer; what do you think?

gwenn and others added 29 commits August 2, 2015 12:07
@@ -1,5 +1,7 @@
# Version UPCOMING (...)

* Adds a `cache` Cargo feature that provides `cache::StatementCache` for caching prepraed
Copy link
Collaborator

Choose a reason for hiding this comment

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

Typo here: prepared

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 fixed

@gwenn
Copy link
Collaborator

gwenn commented Jan 7, 2016

Maybe you should wait before merging this PR.
Look at this: http://sfackler.github.io/rust-postgres/doc/v0.11.0/postgres/struct.Connection.html#method.prepare_cached

@jgallagher
Copy link
Contributor Author

I don't think we can offer an API exactly like that. Their statement handles (the inner StatementInfo) are cloneable; it looks like they're basically just string names that are associated with some prepared query on the server side. We could modify Connection to have an internal StatementCache and then offer something like this API (leaving out lifetimes, not 100% sure what they'd be off the top of my head):

pub fn prepare_cached(&self, sql: &str) -> Result<CachedStatement>

but since we can't clone, we have to give something different than the Statement returned by prepare since we need to take the statement back when the caller is done with it.

@jgallagher
Copy link
Contributor Author

@gwenn Pushed some fairly significant changes to this branch, trying to present an API more like the one you linked from rust-postgres. The relevant user-facing bits are in the impl Connection inside cache.rs. Do you want to take a look?

@gwenn
Copy link
Collaborator

gwenn commented May 18, 2016

The column_count cannot be saved in memory because it can be invalidated by a DDL:

db.execute("CREATE TABLE test (x TEXT)").unwrap();
// ...
let stmt = db.prepare_cached("SELECT * FROM test").unwrap();
// ...
db.execute("ALTER TABLE test ADD COLUMN y TEXT").unwrap();
// ...
stmt.stmt.query_map(&[], |row| {
            let x = row.get(0);
            let y = row.get(1); // PANIC ?
            // ...
    }).unwrap();

More precisely, only the Rows/Row can kept the column_count in memory (not the Statement).
And the one to blame for introducing this issue is me...
jgallagher@f91db1b
Sorry.

@jgallagher
Copy link
Contributor Author

Nice catch! Added that test and a fix in 74b57ee. Notice any other issues?

@gwenn
Copy link
Collaborator

gwenn commented May 19, 2016

No other issue.
Having the cache embedded in the connection is nice.
Thanks.

@jgallagher jgallagher merged commit 432880c into master May 19, 2016
@jgallagher jgallagher deleted the gwenn-stmt-cache branch May 19, 2016 18:45
@jgallagher jgallagher mentioned this pull request May 19, 2016
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.

None yet

3 participants