Fbaetens/bump sqlite kv limit & improve error reporting #2724

Frederik-Baetens · 2024-09-17T16:03:05Z

No description provided.

src/workerd/io/actor-cache.c++

Frederik-Baetens · 2024-09-17T16:38:28Z

This also has an internal PR

justin-mp · 2024-09-17T21:16:34Z

src/workerd/api/actor-state.c++

    kj::Array<byte> buffer = serializeV8Value(js, field.value);
-    ActorStorageLimits::checkMaxValueSize(field.name, buffer);


Hmm, by doing this means that if we get a giant value we're going to keep it around until the check kicks in later. I guess this is probably safe, if only because if the value were too huge, the isolate would have run out of memory before we got here.

src/workerd/api/actor-state.c++

src/workerd/io/actor-cache.c++

src/workerd/util/sqlite-kv.h

justin-mp · 2024-09-17T21:57:00Z

src/workerd/util/sqlite-kv.h

+// of KJ exceptions which become internal errors.
+class SqliteKvRegulator: public SqliteDatabase::Regulator {
+  void onError(kj::StringPtr message) const override {
+    JSG_ASSERT(false, Error, message);


This implementation will throw JSG errors for all possible SQLite errors. Almost everything about this class is under our control so every error EXCEPT the TOOBIG error should be an internal error. Is it possible to report only TOOBIG errors this way?

Yes, but not without either doing matching on the string message (bad), or changing the onError API (out of scope for now).

What I'll do now is only pass the regulator to the put query, so only sql errors for put get passed through. Or would you prefer I do string matching as a temporary kludge here?

Changed it to only report errors on the put query. Let me know your thoughts on that. I acknowledge it's a bit of a hack, but I think its a low consequence change here. Reporting an SQLite error that's actually our fault to the user doesn't seem like a massive issue for sqlite here.

Hmm, including the error code in the onError doesn't seem that difficult actually. I'll try to get something out based on that.

Pushed a fixup commit passing the error code to onError. Let me know what you think of that, and I'll squash it if you think it's a good approach here.

Frederik-Baetens · 2024-09-18T10:47:42Z

src/workerd/api/pyodide/pyodide-test.c++

@@ -3,6 +3,7 @@
 //     https://opensource.org/licenses/Apache-2.0

 #include "pyodide.h"
+


This got in from running the formatter.

justin-mp · 2024-09-18T11:59:49Z

src/workerd/util/sqlite-kv.h

@@ -15,8 +17,22 @@ namespace {
 // In this case we just customize the error reporting to emit JSG user visible errors instead
 // of KJ exceptions which become internal errors.
 class SqliteKvRegulator: public SqliteDatabase::Regulator {
-  void onError(kj::StringPtr message) const override {
-    JSG_ASSERT(false, Error, message);
+  void onError(kj::Maybe<int> sqliteErrorCode, kj::StringPtr message) const override {


I like this approach of taking in the error code. However, it would be better if you put the definition of onError in the C++ file. Doing so means that we wouldn't need to #include <sqlite3.h> in this header file, which means that other headers won't have to include sqlite3.h, which means @sqlite3 can remain an implementation_dep, which means builds should not slow down.

Done this now, I had put it in the header file to be able to put it in an anonymous namespace, but I suppose that doesn't matter much.

FYI for future reference, don't put anonymous namespaces in headers. This causes everything that includes the header to compile its own copy of the stuff in the anonymous namespace. This bloats the binary and could lead to ODR violations.

src/workerd/util/sqlite-kv.h

justin-mp

I think the code itself is in good shape, but we're missing a test in sqlite-kv-test.c++.

Or is there a reason that we shouldn't include a test?

Frederik-Baetens · 2024-09-18T18:01:31Z

I tested this in the internal PR, but I'll add a test there too then.

Frederik-Baetens requested review from a team as code owners September 17, 2024 16:03

Frederik-Baetens requested review from mikea and mar-cf September 17, 2024 16:03

Frederik-Baetens commented Sep 17, 2024

View reviewed changes

src/workerd/io/actor-cache.c++ Outdated Show resolved Hide resolved

Frederik-Baetens force-pushed the fbaetens/bump_sqlite_kv_limit branch from 336f99c to 5c476b5 Compare September 17, 2024 16:06

Frederik-Baetens force-pushed the fbaetens/bump_sqlite_kv_limit branch from 5c476b5 to 321fab6 Compare September 17, 2024 16:42

Frederik-Baetens requested a review from justin-mp September 17, 2024 18:00

justin-mp reviewed Sep 17, 2024

View reviewed changes

Frederik-Baetens force-pushed the fbaetens/bump_sqlite_kv_limit branch 2 times, most recently from 9ef86fa to 46c544d Compare September 18, 2024 08:59

move actor storage limit checks to actor-cache

457c7e1

Frederik-Baetens force-pushed the fbaetens/bump_sqlite_kv_limit branch 2 times, most recently from ab70a64 to 3453abb Compare September 18, 2024 10:46

Frederik-Baetens commented Sep 18, 2024

View reviewed changes

justin-mp reviewed Sep 18, 2024

View reviewed changes

Frederik-Baetens force-pushed the fbaetens/bump_sqlite_kv_limit branch from 3453abb to 8935dba Compare September 18, 2024 12:40

Frederik-Baetens added 2 commits September 18, 2024 13:41

improve sqlite kv error reporting

74e5d50

bump sqlite limit length to 2.2 MB

2983b60

Frederik-Baetens force-pushed the fbaetens/bump_sqlite_kv_limit branch from 8935dba to 2983b60 Compare September 18, 2024 12:41

justin-mp reviewed Sep 18, 2024

View reviewed changes

add test for sqlite-kv key size

e63c2ba

Frederik-Baetens force-pushed the fbaetens/bump_sqlite_kv_limit branch from 327f419 to e63c2ba Compare September 18, 2024 21:14

justin-mp approved these changes Sep 18, 2024

View reviewed changes

Frederik-Baetens merged commit 183b548 into main Sep 18, 2024
12 of 13 checks passed

Frederik-Baetens deleted the fbaetens/bump_sqlite_kv_limit branch September 18, 2024 21:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fbaetens/bump sqlite kv limit & improve error reporting #2724

Fbaetens/bump sqlite kv limit & improve error reporting #2724

Frederik-Baetens commented Sep 17, 2024

Frederik-Baetens commented Sep 17, 2024

justin-mp Sep 17, 2024

justin-mp Sep 17, 2024

Frederik-Baetens Sep 17, 2024 •

edited

Loading

Frederik-Baetens Sep 17, 2024

Frederik-Baetens Sep 18, 2024

Frederik-Baetens Sep 18, 2024

Frederik-Baetens Sep 18, 2024

justin-mp Sep 18, 2024

Frederik-Baetens Sep 18, 2024 •

edited

Loading

justin-mp Sep 18, 2024

justin-mp left a comment

Frederik-Baetens commented Sep 18, 2024

		kj::Array<byte> buffer = serializeV8Value(js, field.value);
		ActorStorageLimits::checkMaxValueSize(field.name, buffer);

		@@ -3,6 +3,7 @@
		// https://opensource.org/licenses/Apache-2.0

		#include "pyodide.h"

Fbaetens/bump sqlite kv limit & improve error reporting #2724

Fbaetens/bump sqlite kv limit & improve error reporting #2724

Conversation

Frederik-Baetens commented Sep 17, 2024

Frederik-Baetens commented Sep 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Frederik-Baetens Sep 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Frederik-Baetens Sep 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justin-mp left a comment

Choose a reason for hiding this comment

Frederik-Baetens commented Sep 18, 2024

Frederik-Baetens Sep 17, 2024 •

edited

Loading

Frederik-Baetens Sep 18, 2024 •

edited

Loading