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.

Sign up for GitHub

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

Fbaetens/bump sqlite limit length #2682

Merged
merged 1 commit into from
Sep 11, 2024
Merged

Fbaetens/bump sqlite limit length #2682

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion src/workerd/util/sqlite.c++
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -817,7 +817,11 @@ void SqliteDatabase::setupSecurity() {
// 2. Reduce limits
// We use the suggested limits from the web site. Note that sqlite3_limit() does NOT return an
// error code; it returns the old limit.
sqlite3_limit(db, SQLITE_LIMIT_LENGTH, 1000000);

// This limit is set highter than what is suggested on sqlite.org/security.html
// because we want to allow storing values of 1MiB, and we added some extra
// padding on top of that
sqlite3_limit(db, SQLITE_LIMIT_LENGTH, 2000000);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you document where we got this value from? Either in the commit message or in a comment?

I believe the reasoning is that we started with 1MB as per https://www.sqlite.org/security.html, but got a request for a higher value so that we could store a 1 MiB message as a value.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can confirm that reasoning is accurate

Copy link
Member

Choose a reason for hiding this comment

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

Heh, but I thought the reason that we wanted to store a 1 MB value (I thought the goal was 1 MB, not 1 MiB?) in the KV API was so that it would have the same limit as in the SQL API? It's kind of circular reasoning to then increase the SQL API size limit just so that the KV API can match the old sql limit (but not the new one).

Not that I have a problem with 2 MB values, but this doesn't solve the goal of allowing the KV API to store values of the same size as the SQL API.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the KV API matching the sql limit is orthogonal to this, and still needs to be done. I just wanted to bump this to accomodate Sid's request.

Copy link
Member

Choose a reason for hiding this comment

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

My point is that we won't really be able to have the KV API match the sql limit, will we? IIUC it'll now just be 1 MiB for KV vs 2 MB for SQL instead of 128 KB for KV and 1 MB for SQL.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I plan to just give the KV API 2MB as well, why wouldn't that be possible?

Copy link
Member

@a-robinson a-robinson Sep 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

If it is then I don't totally understand the reasoning for choosing 2 MB as the new limit (rather than just going up to 1 MiB or something). I thought the sql row limit needed to be at least somewhat bigger than the KV value limit, since each KV key-value pair is stored in a single row, and thus both the key and value combined have to fit into the sqlite row limit.

So if the sqlite row limit is 2 MB, you can't store a 2 MB value in the KV API because even just a 10 byte key would bring the row up to 200010 bytes, i.e. more than the limit.

But if this all works somehow, then just ignore me.

Copy link
Contributor Author

@Frederik-Baetens Frederik-Baetens Sep 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Well the plan is to just have the limits be enforced by sqlite directly, and only surface an error when sqlite surfaces an error.

There's not a terribly specific reason for 2MB, but we wanted to allow 1MiB keys + overhead for the key, and I figured we might as well jump up to 2MB, especially since the default limit is 1GB.

sqlite3_limit(db, SQLITE_LIMIT_SQL_LENGTH, 100000);
sqlite3_limit(db, SQLITE_LIMIT_COLUMN, 100);
sqlite3_limit(db, SQLITE_LIMIT_EXPR_DEPTH, 100);
Expand Down