Return row counts for SQL ingestion (storage.sql.ingest()).

[smerritt]

D1 bills based on rows read and written, but sql.ingest() didn't return those. Now it does.

This required changing the return type of sql.ingest(), but it's an experimental API so that should be okay.

let sqlCode = "INSERT INTO tbl (col) VALUES (123); INSERT I";
let result = sql.ingest(sqlCode);

Old:

result == " INSERT I"

New:

result.remainder == " INSERT I"
result.meta.rowsRead == 0
result.meta.rowsWritten == 1

This "meta" attribute also gives us a convenient spot to stick additional information. For example, if we wanted to count how many SQL statements ingest() executed, we could easily add "result.meta.statementsExecuted" without breaking any existing users.

Implementation-wise, there might be an optimization opportunity around SqlStorage::IngestResult::getMeta(). Right now, it allocates a new Meta every time it's called, so if you do the obvious thing like so:

totalRead += result.meta.rowsRead
totalWritten += result.meta.rowsWritten
totalRedFish += result.meta.redFish // and so on

then that might end up allocating and discarding multiple Meta objects, and some caching could fix that. On the other hand, I'm not very familiar[1] with JSG internals, so maybe that's already happening somehow. I have no idea.

[1] This is a terrible understatement.

[geelen]

Thanks for looking at this @smerritt! I think this would actually work fine, but interested in following up on this idea (from internal chat):

At the SQLite level, we've got a pair of DB-wide row counters (read and written), which would be really handy here.
workerd's API has SqliteDatabase::Query() resetting the row counters, though, which sort of makes sense and sort of doesn't. I'd almost prefer if the DB-wide counters just kept on going up† and Query computed deltas.

Part of me wants DB-wide non-resetting counters to make it easier to do stuff like this, and part of me thinks that it'd also make it easy to compute garbage values, like if you had a function that was synchronous but got an async step wedged in the middle later on, your before/after row counts could end up garbage.

We already make some fairly strict assumptions around the SQL api being complete synchronous, in particular calling sql.databaseSize and executing SELECT total_changes() as changes, last_insert_rowid() as last_row_id before and after each D1 query (which could be a batch with multiple SQL queries inside) in order to calculate the meta param of the return object.

I don't think there's any real worry in adding sql.totalRowsRead and sql.totalRowsWritten as ever-incrementing counters (accessible from JS), then making Query read the value before and after to produce the value for getRowsRead() and getRowsWritten(). Then APIs like ingest (and whatever a future DO + SQL user might need) can simply read the value before and after each logical operation and return it to the user in whatever form makes sense.

At the very least, this removes any concept of meta from the .ingest API and makes the change fully backwards-compatible.

[kentonv]

I think we intentionally didn't add DB-wide counters because queries can be running concurrently (you can have multiple cursors) and so these counters felt like a footgun.

Regarding the interface change to ingest(), my nitpick is I would not create a meta property, just flatten the counters into result itself. This is closer to what exec() returns -- a cursor which has rowsRead and rowsWritten as top-level properties.

[smerritt]

Flattening the counters into result sounds good to me. It's still the case that we can add more properties and methods later on, so I'm happy.

[smerritt]

Fixup: f583865

[smerritt]

This is ready for review.

ingest's return type is now flat: it's got three properties: remainder, rowsRead, and rowsWritten.

As far as the DB-wide counters, Kenton's right: they're a footgun. If you compute deltas around a chunk of purely synchronous code, you'll get correct numbers, but if any asynchrony creeps in between the queries across which you are computing deltas, your deltas can be too large. Summing row counts across queries is much less error-prone. I think it's better to take the interface change now, while it's still experimental, and just deal with a bit of jank in the D1 worker for a few weeks.

[smerritt]

479edba is a rebase on main.

[geelen]

@smerritt ok, I'm happy with this. I can tweak the d1-worker stuff as necessary. Can I ask to add one more counter: numQueries? It turns out that was the one bit we used to show to users in Wrangler, that we've now lost due to no longer parsing the SQL locally:

[smerritt]

@geelen Commit 0cc33b7 adds the statement count.

[jasnell]

Code changes LGTM