sqlite: allow pragmas and statements needed to run the query optimizer #3483
Conversation
justin-mp
force-pushed
the
jmp/sqlite-optimize
branch
from
aa67bb3 to
1e99152
Compare
src/workerd/util/sqlite.c++
Outdated
| case SQLITE_ANALYZE: /* Table Name NULL */ | ||
| KJ_ASSERT(param2 == kj::none); | ||
| // We allow all names (including names where isAllowedName() would return false) because a | ||
| // SQLite ANALYZE statement with no parameters will analyze all tables, including otherwise |
There was a problem hiding this comment.
If I understand correctly, ANALYZE doesn't return any information to the caller, it just makes notes for future query planning. Is that right? Therefore, it certainly doesn't reveal nor modify the contents of hidden tables, therefore there's no need to restrict it? Might be worth spelling that out in the comment here.
There was a problem hiding this comment.
Your understanding is correct and I've updated the comment.
One non-obvious thing is that the notes that ANALYZE makes are available in a table that users can modify. This means that customers can make the query planner do suboptimal things. It's likely that the worst thing a customer could do is waste CPU, but it is a rather large surface that we open up by allowing ANALYZE.
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.
justin-mp
force-pushed
the
jmp/sqlite-optimize
branch
from
1e99152 to
e2afa2f
Compare
I do think we want either D1 or DO to automatically run The pros of doing the automatic version now are (1) that we probably wouldn't need to allowlist The downsides are: (1) more work to figure out when we can call I'm happy to drop this in favor of the automatic approach if we think that's worth waiting for compared to getting something out sooner. |
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 optimizeafter making schema changes.A couple of notes about this commit:
PRAGMA optimizemay callANALYZEon all tables, including the internal_cf_tables, so we have to allowANALYZEeven on otherwise disallowed table names.Calls to
ANALYZE, either directly or viaPRAGMA optimize, cause the creation of thesqlite_stat1table. 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.