Skip to content

Commit

Permalink
sqlite: allow pragmas and statements needed to run the query optimizer
Browse files Browse the repository at this point in the history
https://www.sqlite.org/lang_analyze.html recommends a bunch of things
one can do to optimize the performance of the query planner.  Most
folks using Durable Objects will probably be best served by just
calling `PRAGMA optimize` after making schema changes.

A couple of notes about this commit:

* `PRAGMA optimize` may call `ANALYZE` on all tables, including the
  internal `_cf_` tables, so we have to allow `ANALYZE` even on
  otherwise disallowed table names.

* Calls to `ANALYZE`, either directly or via `PRAGMA optimize`, cause
  the creation of the `sqlite_stat1` table.  This caused some noise in
  sql-test.js as I had to update tests that depend on (a) the particular
  set of tables in the database and (b) the particular size of the
  database.
  • Loading branch information
justin-mp committed Feb 7, 2025
1 parent f26d5ed commit e2afa2f
Show file tree
Hide file tree
Showing 2 changed files with 55 additions and 10 deletions.
33 changes: 25 additions & 8 deletions src/workerd/api/sql-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -406,6 +406,23 @@ async function test(state) {
// But that private tables are again restricted
assert.throws(() => sql.exec('PRAGMA quick_check(_cf_KV)'), /not authorized/);

// PRAGMA optimize and ANALYZE are allowed.
//
// The following sequence of calls is mentioned by https://www.sqlite.org/lang_analyze.html as how
// one might optmize the query planner.
{
let info = [...sql.exec('PRAGMA optimize=0x10002;')];
assert.equal(info.length, 0);
}
{
let info = [...sql.exec('ANALYZE;')];
assert.equal(info.length, 0);
}
{
let info = [...sql.exec('PRAGMA optimize;')];
assert.equal(info.length, 0);
}

// Basic functions like abs() work.
assert.equal([...sql.exec('SELECT abs(-123)').raw()][0][0], 123);

Expand Down Expand Up @@ -617,16 +634,16 @@ async function test(state) {
)
)[0].data
);
assert.equal(jsonResult.length, 11);
assert.equal(jsonResult.length, 12);
assert.equal(
jsonResult.map((r) => r.name).join(','),
'myTable,documents,documents_fts,documents_fts_data,documents_fts_idx,documents_fts_content,documents_fts_docsize,documents_fts_config,documents_fts_v_col,documents_fts_v_row,documents_fts_v_instance'
'myTable,sqlite_stat1,documents,documents_fts,documents_fts_data,documents_fts_idx,documents_fts_content,documents_fts_docsize,documents_fts_config,documents_fts_v_col,documents_fts_v_row,documents_fts_v_instance'
);
assert.equal(jsonResult[0].columns.foo, 'TEXT');
assert.equal(jsonResult[0].columns.bar, 'INTEGER');
assert.equal(jsonResult[1].columns.id, 'INTEGER');
assert.equal(jsonResult[1].columns.title, 'TEXT');
assert.equal(jsonResult[1].columns.content, 'TEXT');
assert.equal(jsonResult[2].columns.id, 'INTEGER');
assert.equal(jsonResult[2].columns.title, 'TEXT');
assert.equal(jsonResult[2].columns.content, 'TEXT');
}

let assertValidBool = (name, val) => {
Expand Down Expand Up @@ -670,11 +687,11 @@ async function test(state) {
assertInvalidBool("'yes", 'unrecognized token');

// Test database size interface.
assert.equal(sql.databaseSize, 36864);
assert.equal(sql.databaseSize, 40960);
sql.exec(`CREATE TABLE should_make_one_more_page(VALUE text);`);
assert.equal(sql.databaseSize, 36864 + 4096);
assert.equal(sql.databaseSize, 40960 + 4096);
sql.exec(`DROP TABLE should_make_one_more_page;`);
assert.equal(sql.databaseSize, 36864);
assert.equal(sql.databaseSize, 40960);

storage.put('txnTest', 0);

Expand Down
32 changes: 30 additions & 2 deletions src/workerd/util/sqlite.c++
Original file line number Diff line number Diff line change
Expand Up @@ -409,6 +409,7 @@ enum class PragmaSignature {
BOOLEAN,
OBJECT_NAME,
OPTIONAL_OBJECT_NAME,
NULL_OR_NUMBER,
NULL_NUMBER_OR_OBJECT_NAME
};
struct PragmaInfo {
Expand Down Expand Up @@ -438,7 +439,10 @@ static constexpr PragmaInfo ALLOWED_PRAGMAS[] = {{"data_version"_kj, PragmaSigna
{"index_xinfo"_kj, PragmaSignature::OBJECT_NAME},

// Takes an argument of table name/index name OR a max number of results, or nothing
{"quick_check"_kj, PragmaSignature::NULL_NUMBER_OR_OBJECT_NAME}};
{"quick_check"_kj, PragmaSignature::NULL_NUMBER_OR_OBJECT_NAME},

// Takes a number representing a bit mask or nothing to use the default mask.
{"optimize"_kj, PragmaSignature::NULL_OR_NUMBER}};

} // namespace

Expand Down Expand Up @@ -899,13 +903,31 @@ bool SqliteDatabase::isAuthorized(int actionCode,
case SQLITE_DELETE: /* Table Name NULL */
case SQLITE_DROP_TABLE: /* Table Name NULL */
case SQLITE_INSERT: /* Table Name NULL */
case SQLITE_ANALYZE: /* Table Name NULL */
case SQLITE_CREATE_VIEW: /* View Name NULL */
case SQLITE_DROP_VIEW: /* View Name NULL */
case SQLITE_REINDEX: /* Index Name NULL */
KJ_ASSERT(param2 == kj::none);
return regulator.isAllowedName(KJ_ASSERT_NONNULL(param1));

case SQLITE_ANALYZE: /* Table Name NULL */
KJ_ASSERT(param2 == kj::none);
// We allow all names (including names where isAllowedName() would return false) because
// `PRAGMA optimize` issues an ANALYZE statement with no arguments and a SQLite ANALYZE
// statement with no parameters will analyze all tables, including otherwise restricted
// tables.
//
// The ANALYZE statement records information about the distribution of rows in each index in
// the database in a special sqlite_stat1 table. While the sqlite_stat1 table leaks metadata
// about restricted tables (like the names of indices and the sizes of those tables), the
// sqlite_stat1 does not contain data from the restricted tables. As such, it's OK to allow
// users to ANALYZE restricted tables.
//
// Note that users can *modify* the sqlite_stat1 table, which means that they can make the
// query planner work in suboptimal ways by writing bogus data to the table.
//
// See https://www.sqlite.org/fileformat2.html#stat1tab for more details.
return true;

case SQLITE_ALTER_TABLE: /* Table Name NULL (modified) */
return regulator.isAllowedName(KJ_ASSERT_NONNULL(param1));

Expand Down Expand Up @@ -1031,6 +1053,12 @@ bool SqliteDatabase::isAuthorized(int actionCode,
auto val = KJ_UNWRAP_OR(param2, return true);
return regulator.isAllowedName(val);
}
case PragmaSignature::NULL_OR_NUMBER: {
// Argument is not required
auto val = KJ_UNWRAP_OR(param2, return true);
// val is allowed if it parses to an integer
return val.tryParseAs<uint>() != kj::none;
}
case PragmaSignature::NULL_NUMBER_OR_OBJECT_NAME: {
// Argument is not required
auto val = KJ_UNWRAP_OR(param2, return true);
Expand Down

0 comments on commit e2afa2f

Please sign in to comment.