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

Cache prepared statements #98

Closed
sgrif opened this Issue Jan 15, 2016 · 3 comments

Comments

Projects
None yet
1 participant
@sgrif
Member

sgrif commented Jan 15, 2016

Currently we're never caching a prepared statement (though we are preparing them). We should put these in an LRU cache automatically, with a configurable size. Beyond the ability to specify the size, this should be a completely opaque change.

@sgrif

This comment has been minimized.

Member

sgrif commented Jan 18, 2016

Ok so I've started looking into this a little bit, and my first pass worked similarly to Rails, where I calculated the statement name from the hash of the SQL string. I actually started to wonder if this was even worth it, as to my knowledge PG will perform query planner caching automatically regardless.

However, I've come to realize that due to how our query builder works, we can do much better than that. As long as an AST contains no SqlLiteral nodes (in which case we should not prepare the statement), TypeId::of<QueryType>() is a sufficient stand-in for a hashing function. (As an aside, if we wrote #[derive(Hash)] for every Expression, any AST which does not contain SqlLiteral would result in the same hash if we hashed the query rather than the resulting SQL string).

We can also determine whether an AST contains a SqlLiteral node as a compile time constant. As such, an implementation should do the following:

  • Change our AST builder to be 2-pass, once for constructing the query, once for collecting the bind params.
    • This means QueryFragment/Query/Expression change to roughly the following (Note: The error types might change or be able to be removed from to_sql)
pub trait QueryFragment {
    fn is_preparable() -> bool;
    fn to_sql(&self, out: &mut QueryBuilder) -> BuildQueryResult;
    fn collect_bind_params(&self, out: &mut BindParamCollector) -> BuildQueryResult;
}
  • Change connection to operate on PQprepare and PQexecPrepared when is_preparable() returns true
  • Cache prepared statements in a BTreeMap<u64, CString> where the key is the type id, and the value is the statement name. This can be in an LRU cache that wraps the same type if we deem it appropriate
    • I'm unsure if we actually need to do LRU caching here. Would like to see some numbers on what we can expect a server to handle in terms of number of concurrently prepared statements

While to_sql will continue to incur the same costs that it does today, is_preparable and collect_bind_params should be absurdly optimizable, where is_preparable should compile to a literal, and collect_bind_params should eliminate the call for all nodes that aren't Bound, leaving only the to_sql calls inline.

@sgrif

This comment has been minimized.

Member

sgrif commented Jan 18, 2016

This might not actually work for our case, since TypeId::of<T> requires that T: 'static. Now the only type for which that doesn't hold true is Bound<T, U> where U might not be 'static. Ironically, that's the only case where we don't want the ID to change, but presumably if we did remove the 'static bound fromTypeId::of, then &'a i32 and &'b i32 would have different types. We actually want Bound to have the same type for ID here for any U, so that Bound<VarChar, String> was the same as Bound<VarChar, &str>. That said, if we just have one additional prepared statement for users.filter(name.eq("Sean")) and users.filter(name.eq("Sean".to_string())), it's not the end of the world. We should explore this further.

At the end of the day, if we do end up having to compute the hash of the SQL string, that's fine too. I'd still like to change our query builder to be 2 pass, as that will potentially be helpful for SQLite support, and will make this optimization easier in the future.

@sgrif

This comment has been minimized.

Member

sgrif commented Dec 10, 2016

Did I really forget to close this? This was done by #279 and #280

@sgrif sgrif closed this Dec 10, 2016

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