Fbaetens/bump sqlite limit length #2682
Conversation
Frederik-Baetens
force-pushed
the
fbaetens/bump_sqlite_limit_length
branch
from
3889162 to
f422ff1
Compare
| @@ -817,7 +817,7 @@ 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); | |||
| sqlite3_limit(db, SQLITE_LIMIT_LENGTH, 2000000); | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Can confirm that reasoning is accurate
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I plan to just give the KV API 2MB as well, why wouldn't that be possible?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Frederik-Baetens
force-pushed
the
fbaetens/bump_sqlite_limit_length
branch
2 times, most recently
from
657c0bf to
d189a20
Compare
| @@ -817,7 +817,7 @@ 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); | |||
| sqlite3_limit(db, SQLITE_LIMIT_LENGTH, 2000000); | |||
There was a problem hiding this comment.
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.
Frederik-Baetens
force-pushed
the
fbaetens/bump_sqlite_limit_length
branch
from
d189a20 to
6a864b7
Compare
No description provided.