Use sqlite3_limit instead of hard-coded SQLITE_MAX_VARIABLE_NUMBER (#371) (#288) r=rnewman

* Part 1: added limits feature to rusqlite dependencies.
* Part 2: replace references to SQLITE_MAX_VARIABLE_NUMBER with sqlite3_limit.
* Move assertion check for correct number of variables in repeat_values to before call as this is where the variable is defined.
* Part 3: add tests
This commit is contained in:
Emily Toop 2017-03-13 16:39:19 +00:00 committed by Richard Newman
parent 6109a63249
commit fddc57d548
5 changed files with 27 additions and 12 deletions

View file

@ -28,7 +28,7 @@ time = "0.1.35"
[dependencies.rusqlite] [dependencies.rusqlite]
version = "0.10.1" version = "0.10.1"
# System sqlite might be very old. # System sqlite might be very old.
features = ["bundled"] features = ["bundled", "limits"]
[dependencies.edn] [dependencies.edn]
path = "edn" path = "edn"

View file

@ -13,7 +13,7 @@ time = "0.1.35"
[dependencies.rusqlite] [dependencies.rusqlite]
version = "0.10.1" version = "0.10.1"
# System sqlite might be very old. # System sqlite might be very old.
features = ["bundled"] features = ["bundled", "limits"]
[dependencies.edn] [dependencies.edn]
path = "../edn" path = "../edn"

View file

@ -21,6 +21,7 @@ use itertools;
use itertools::Itertools; use itertools::Itertools;
use rusqlite; use rusqlite;
use rusqlite::types::{ToSql, ToSqlOutput}; use rusqlite::types::{ToSql, ToSqlOutput};
use rusqlite::limits::Limit;
use ::{repeat_values, to_namespaced_keyword}; use ::{repeat_values, to_namespaced_keyword};
use bootstrap; use bootstrap;
@ -613,7 +614,8 @@ impl MentatStoring for rusqlite::Connection {
// produce the map [a v] -> e. // produce the map [a v] -> e.
// //
// TODO: `collect` into a HashSet so that any (a, v) is resolved at most once. // TODO: `collect` into a HashSet so that any (a, v) is resolved at most once.
let chunks: itertools::IntoChunks<_> = avs.into_iter().enumerate().chunks(::SQLITE_MAX_VARIABLE_NUMBER / 4); let max_vars = self.limit(Limit::SQLITE_LIMIT_VARIABLE_NUMBER) as usize;
let chunks: itertools::IntoChunks<_> = avs.into_iter().enumerate().chunks(max_vars / 4);
// We'd like to `flat_map` here, but it's not obvious how to `flat_map` across `Result`. // We'd like to `flat_map` here, but it's not obvious how to `flat_map` across `Result`.
// Alternatively, this is a `fold`, and it might be wise to express it as such. // Alternatively, this is a `fold`, and it might be wise to express it as such.
@ -642,7 +644,9 @@ impl MentatStoring for rusqlite::Connection {
// TODO: query against `datoms` and UNION ALL with `fulltext_datoms` rather than // TODO: query against `datoms` and UNION ALL with `fulltext_datoms` rather than
// querying against `all_datoms`. We know all the attributes, and in the common case, // querying against `all_datoms`. We know all the attributes, and in the common case,
// where most unique attributes will not be fulltext-indexed, we'll be querying just // where most unique attributes will not be fulltext-indexed, we'll be querying just
// `datoms`, which will be much faster. // `datoms`, which will be much faster.ˇ
assert!(bindings_per_statement * count < max_vars, "Too many values: {} * {} >= {}", bindings_per_statement, count, max_vars);
let values: String = repeat_values(bindings_per_statement, count); let values: String = repeat_values(bindings_per_statement, count);
let s: String = format!("WITH t(search_id, a, v, value_type_tag) AS (VALUES {}) SELECT t.search_id, d.e \ let s: String = format!("WITH t(search_id, a, v, value_type_tag) AS (VALUES {}) SELECT t.search_id, d.e \
FROM t, all_datoms AS d \ FROM t, all_datoms AS d \
@ -731,7 +735,8 @@ impl MentatStoring for rusqlite::Connection {
fn insert_non_fts_searches<'a>(&self, entities: &'a [ReducedEntity<'a>], search_type: SearchType) -> Result<()> { fn insert_non_fts_searches<'a>(&self, entities: &'a [ReducedEntity<'a>], search_type: SearchType) -> Result<()> {
let bindings_per_statement = 6; let bindings_per_statement = 6;
let chunks: itertools::IntoChunks<_> = entities.into_iter().chunks(::SQLITE_MAX_VARIABLE_NUMBER / bindings_per_statement); let max_vars = self.limit(Limit::SQLITE_LIMIT_VARIABLE_NUMBER) as usize;
let chunks: itertools::IntoChunks<_> = entities.into_iter().chunks(max_vars / bindings_per_statement);
// We'd like to flat_map here, but it's not obvious how to flat_map across Result. // We'd like to flat_map here, but it's not obvious how to flat_map across Result.
let results: Result<Vec<()>> = chunks.into_iter().map(|chunk| -> Result<()> { let results: Result<Vec<()>> = chunks.into_iter().map(|chunk| -> Result<()> {
@ -768,6 +773,7 @@ impl MentatStoring for rusqlite::Connection {
}).collect(); }).collect();
// TODO: cache this for selected values of count. // TODO: cache this for selected values of count.
assert!(bindings_per_statement * count < max_vars, "Too many values: {} * {} >= {}", bindings_per_statement, count, max_vars);
let values: String = repeat_values(bindings_per_statement, count); let values: String = repeat_values(bindings_per_statement, count);
let s: String = if search_type == SearchType::Exact { let s: String = if search_type == SearchType::Exact {
format!("INSERT INTO temp.exact_searches (e0, a0, v0, value_type_tag0, added0, flags0) VALUES {}", values) format!("INSERT INTO temp.exact_searches (e0, a0, v0, value_type_tag0, added0, flags0) VALUES {}", values)
@ -797,7 +803,8 @@ impl MentatStoring for rusqlite::Connection {
// TODO: only update changed partitions. // TODO: only update changed partitions.
pub fn update_partition_map(conn: &rusqlite::Connection, partition_map: &PartitionMap) -> Result<()> { pub fn update_partition_map(conn: &rusqlite::Connection, partition_map: &PartitionMap) -> Result<()> {
let values_per_statement = 2; let values_per_statement = 2;
let max_partitions = ::SQLITE_MAX_VARIABLE_NUMBER / values_per_statement; let max_vars = conn.limit(Limit::SQLITE_LIMIT_VARIABLE_NUMBER) as usize;
let max_partitions = max_vars / values_per_statement;
if partition_map.len() > max_partitions { if partition_map.len() > max_partitions {
bail!(ErrorKind::NotYetImplemented(format!("No more than {} partitions are supported", max_partitions))); bail!(ErrorKind::NotYetImplemented(format!("No more than {} partitions are supported", max_partitions)));
} }
@ -1011,4 +1018,16 @@ mod tests {
let transactions = value.as_vector().unwrap(); let transactions = value.as_vector().unwrap();
assert_transactions(&conn, &mut db.partition_map, &mut db.schema, transactions); assert_transactions(&conn, &mut db.partition_map, &mut db.schema, transactions);
} }
#[test]
fn test_sqlite_limit() {
let conn = new_connection("").expect("Couldn't open in-memory db");
let initial = conn.limit(Limit::SQLITE_LIMIT_VARIABLE_NUMBER);
// Sanity check.
assert!(initial > 500);
// Make sure setting works.
conn.set_limit(Limit::SQLITE_LIMIT_VARIABLE_NUMBER, 222);
assert_eq!(222, conn.limit(Limit::SQLITE_LIMIT_VARIABLE_NUMBER));
}
} }

View file

@ -54,9 +54,6 @@ pub use types::{
use edn::symbols; use edn::symbols;
// TODO: replace with sqlite3_limit. #288.
pub const SQLITE_MAX_VARIABLE_NUMBER: usize = 999;
pub fn to_namespaced_keyword(s: &str) -> Result<symbols::NamespacedKeyword> { pub fn to_namespaced_keyword(s: &str) -> Result<symbols::NamespacedKeyword> {
let splits = [':', '/']; let splits = [':', '/'];
let mut i = s.split(&splits[..]); let mut i = s.split(&splits[..]);
@ -84,7 +81,6 @@ pub fn to_namespaced_keyword(s: &str) -> Result<symbols::NamespacedKeyword> {
pub fn repeat_values(values_per_tuple: usize, tuples: usize) -> String { pub fn repeat_values(values_per_tuple: usize, tuples: usize) -> String {
assert!(values_per_tuple >= 1); assert!(values_per_tuple >= 1);
assert!(tuples >= 1); assert!(tuples >= 1);
assert!(values_per_tuple * tuples < SQLITE_MAX_VARIABLE_NUMBER, "Too many values: {} * {} >= {}", values_per_tuple, tuples, SQLITE_MAX_VARIABLE_NUMBER);
// Like "(?, ?, ?)". // Like "(?, ?, ?)".
let inner = format!("({})", repeat("?").take(values_per_tuple).join(", ")); let inner = format!("({})", repeat("?").take(values_per_tuple).join(", "));
// Like "(?, ?, ?), (?, ?, ?)". // Like "(?, ?, ?), (?, ?, ?)".

View file

@ -9,7 +9,7 @@ error-chain = "0.9.0"
[dependencies.rusqlite] [dependencies.rusqlite]
version = "0.10.1" version = "0.10.1"
# System sqlite might be very old. # System sqlite might be very old.
features = ["bundled"] features = ["bundled", "limits"]
[dependencies.mentat_core] [dependencies.mentat_core]
path = "../core" path = "../core"