From 4cd0076ec65d51974dbe3efdd03be6ed5bf91c80 Mon Sep 17 00:00:00 2001 From: Theo Browne Date: Wed, 14 Oct 2026 01:14:00 -0700 Subject: [PATCH] perf(shared): cached SQLite statements no longer hold large parameters (#19305) Takeover of #17785 by @tris203. Executions binding more than 64 KiB of text or blob data run on a throwaway statement, in the writer and the reader worker, so cached statements no longer pin large payloads in native memory. Co-authored-by: tris203 Co-authored-by: Claude Opus 5.5 --- packages/shared/src/nodeSqliteClient.test.ts | 128 +++++++++++++++++++ packages/shared/src/nodeSqliteClient.ts | 63 ++++++++- 2 files changed, 188 insertions(+), 3 deletions(-) diff --git a/packages/shared/src/nodeSqliteClient.test.ts b/packages/shared/src/nodeSqliteClient.test.ts index 040baa7ba1304e7629a960b468729779d84a5b82..423e1ab6838b2581281dca9923ae602d9b2cee03 100644 --- a/packages/shared/src/nodeSqliteClient.test.ts +++ b/packages/shared/src/nodeSqliteClient.test.ts @@ -131,6 +131,102 @@ layer("NodeSqliteClient", (it) => { ); }); +const largeText = "x".repeat(64 * 1024 + 1); + +it.effect("keeps large text and blob parameters off cached statements", () => + Effect.gen(function* () { + const sql = yield* SqlClient.SqlClient; + const statements: Array = []; + const all = NodeSqlite.StatementSync.prototype.all; + yield* Effect.acquireRelease( + Effect.sync(() => + vi.spyOn(NodeSqlite.StatementSync.prototype, "all").mockImplementation(function ( + this: NodeSqlite.StatementSync, + ...params + ) { + statements.push(this); + return all.apply(this, params); + }), + ), + (spy) => Effect.sync(() => spy.mockRestore()), + ); + + const small = "small"; + const queries = [ + (value: string | Uint8Array | ArrayBuffer) => sql`SELECT length(${value}) AS size`, + (value: string | Uint8Array | ArrayBuffer) => + sql.unsafe("SELECT length($value) AS size", [{ $value: value }]), + (value: string | Uint8Array | ArrayBuffer) => + sql.unsafe("SELECT length($value) AS callable", [ + Object.assign(() => {}, { $value: value }), + ]), + ]; + const cached = new Set(); + const large = new Set(); + const largeBlobs = [new Uint8Array(64 * 1024 + 1), new ArrayBuffer(64 * 1024 + 1)]; + for (const value of [small, largeText, ...largeBlobs, small]) { + const expected = typeof value === "string" ? value.length : value.byteLength; + for (const query of queries) { + assert.deepEqual((yield* query(value)).map(Object.values), [[expected]]); + assert.deepEqual(yield* query(value).values, [[expected]]); + for (const statement of statements.splice(0)) { + (value === small ? cached : large).add(statement); + } + } + } + + // Small parameters keep reusing the one cached statement for each query. + assert.equal(cached.size, queries.length); + // Each large execution gets a statement that nothing holds on to afterwards. + assert.equal(large.size, queries.length * 6); + assert.isTrue(cached.isDisjointFrom(large)); + }).pipe(Effect.provide(SqliteClient.layer({ filename: ":memory:" }))), +); + +it.effect("reads integers on the values path as the caller asked, cached or not", () => + Effect.gen(function* () { + const sql = yield* SqlClient.SqlClient; + for (const value of ["small", largeText]) { + const values = sql`SELECT 1 AS id, length(${value}) AS size`.values; + assert.deepEqual(yield* values.pipe(Effect.provideService(SqlClient.SafeIntegers, true)), [ + [1n, BigInt(value.length)], + ]); + assert.deepEqual(yield* values, [[1, value.length]]); + } + }).pipe(Effect.provide(SqliteClient.layer({ filename: ":memory:" }))), +); + +it.effect("leaves a parameter it cannot measure for the statement to handle", () => + Effect.gen(function* () { + const sql = yield* SqlClient.SqlClient; + const detached = new DataView(new ArrayBuffer(8)); + structuredClone(detached.buffer, { transfer: [detached.buffer] }); + // What a detached view binds as varies by Node version; match the statement itself. + const native = new NodeSqlite.DatabaseSync(":memory:"); + const expected = native.prepare("SELECT length(?) AS size").all(detached); + native.close(); + assert.deepEqual(yield* sql`SELECT length(${detached}) AS size`, expected); + + const throwing = { + get $value(): string { + throw new Error("unreadable"); + }, + }; + const error = yield* sql.unsafe("SELECT length($value) AS size", [throwing]).pipe(Effect.flip); + assert.equal(error.reason.operation, "execute"); + + // Only the statement reads a getter, so it runs once as it would natively. + let reads = 0; + const counting = { + get $value(): number { + return ++reads; + }, + }; + assert.deepEqual(yield* sql.unsafe("SELECT $value AS value", [counting]), [{ value: 1 }]); + assert.equal(reads, 1); + }).pipe(Effect.provide(SqliteClient.layer({ filename: ":memory:" }))), +); + const makeTempDatabase = Effect.gen(function* () { const fs = yield* FileSystem.FileSystem; const path = yield* Path.Path; @@ -311,6 +407,38 @@ it.effect("keeps read-only work inside a write transaction on the writer", () => ), ); +it.effect("keeps large parameters off the reader worker's cached statements", () => + withReaderClient((sql) => + Effect.gen(function* () { + const read = (value: string) => + sql<{ size: number }>`SELECT length(${value}) AS size`.pipe( + Effect.map((rows) => rows[0]?.size), + ); + // Start the worker so its requests can be recorded. + yield* countEntries(sql).pipe(SqliteClient.readOnly); + const worker = startedWorkers.at(-1)!; + const cached: Array = []; + const postMessage = worker.postMessage; + yield* Effect.acquireRelease( + Effect.sync(() => + vi.spyOn(worker, "postMessage").mockImplementation(function ( + this: NodeWorkerThreads.Worker, + message: { readonly cached: boolean }, + ) { + cached.push(message.cached); + return postMessage.call(this, message); + }), + ), + (spy) => Effect.sync(() => spy.mockRestore()), + ); + + assert.equal(yield* read("small").pipe(SqliteClient.readOnly), 5); + assert.equal(yield* read(largeText).pipe(SqliteClient.readOnly), largeText.length); + assert.deepEqual(cached, [true, false]); + }).pipe(Effect.scoped), + ), +); + it.effect("fails a read-only transaction whose reader stops, then starts a new reader", () => withReaderClient((sql) => Effect.gen(function* () { diff --git a/packages/shared/src/nodeSqliteClient.ts b/packages/shared/src/nodeSqliteClient.ts index dd00bccd1e476342a63507b8ded590f60bb5c7b0..2749ff9cd1b6291f575c315e1a00ca5680efe815 100644 --- a/packages/shared/src/nodeSqliteClient.ts +++ b/packages/shared/src/nodeSqliteClient.ts @@ -5,6 +5,7 @@ * @module SqliteClient */ import * as NodeSqlite from "node:sqlite"; +import * as NodeUtil from "node:util"; import * as NodeWorkerThreads from "node:worker_threads"; import * as Cache from "effect/Cache"; @@ -126,6 +127,54 @@ const classifyError = (cause: unknown, message: string, operation: string) => { return classifySqliteError(cause, { message, operation }); }; +/** + * SQLite keeps its copy of a statement's text and blob parameters until the + * statement is bound again or finalized, and `node:sqlite` cannot clear them. + * A cached statement would hold its last payload for as long as it stays + * cached, so executions binding more than this many string code units and + * blob bytes, or a named parameter behind a getter, get a throwaway statement + * instead. + */ +const LARGE_BINDING_SIZE = 64 * 1024; + +// `node:sqlite` binds both buffer views and raw buffers as blobs. +const isBlob = (value: unknown): value is ArrayBufferView | ArrayBufferLike => + ArrayBuffer.isView(value) || NodeUtil.types.isAnyArrayBuffer(value); + +const bindingSize = (value: unknown): number => { + if (typeof value === "string") { + return value.length; + } + return isBlob(value) ? value.byteLength : 0; +}; + +const hasLargeBinding = (params: ReadonlyArray): boolean => { + try { + let size = 0; + for (const param of params) { + if ( + (typeof param === "object" || typeof param === "function") && + param !== null && + !isBlob(param) + ) { + // `node:sqlite` takes named parameters as one object, callable or not. + // Read data properties only: calling a getter here would run it twice. + for (const property of Object.values(Object.getOwnPropertyDescriptors(param))) { + if (!property.enumerable) continue; + if (!("value" in property)) return true; + size += bindingSize(property.value); + } + } else { + size += bindingSize(param); + } + } + return size > LARGE_BINDING_SIZE; + } catch { + // A parameter that cannot be measured is left for the statement to report. + return true; + } +}; + // Evaluated source instead of a worker file, so the reader also starts from // bundles and single-executable builds that have no module file on disk. const readerWorkerSource = ` @@ -327,8 +376,10 @@ const makeReader = Effect.fnUntraced(function* (options: SqliteClientConfig) { }, }); if (pending.size === 1) worker.ref(); + // The worker's statement cache would pin large parameters the same way. + const cached = input.cached && !hasLargeBinding(input.params); // oxlint-disable-next-line unicorn/require-post-message-target-origin -- Node workers do not accept a target origin. - worker.postMessage({ id, safeIntegers, ...input }); + worker.postMessage({ id, safeIntegers, ...input, cached }); }); }); @@ -487,6 +538,9 @@ const make = Effect.fn("makeWithDatabase")(function* ( Exit.isSuccess(exit) ? (options.prepareCacheTTL ?? Duration.minutes(10)) : Duration.zero, }); + const statementFor = (sql: string, params: ReadonlyArray) => + hasLargeBinding(params) ? prepare(sql) : Cache.get(prepareCache, sql); + const runStatement = ( statement: NodeSqlite.StatementSync, params: ReadonlyArray, @@ -510,7 +564,7 @@ const make = Effect.fn("makeWithDatabase")(function* ( }); const run = (sql: string, params: ReadonlyArray, raw = false) => - Effect.flatMap(Cache.get(prepareCache, sql), (s) => runStatement(s, params, raw)); + Effect.flatMap(statementFor(sql, params), (s) => runStatement(s, params, raw)); const runStatementValues = ( statement: NodeSqlite.StatementSync, @@ -521,6 +575,9 @@ const make = Effect.fn("makeWithDatabase")(function* ( (statement) => Effect.try({ try: () => { + statement.setReadBigInts( + Boolean(Context.get(Fiber.getCurrent()!.context, Client.SafeIntegers)), + ); if (hasRows(statement)) { statement.setReturnArrays(true); // Safe to cast to array after we've setReturnArrays(true) @@ -555,7 +612,7 @@ const make = Effect.fn("makeWithDatabase")(function* ( ); const runValues = (sql: string, params: ReadonlyArray) => - Effect.flatMap(Cache.get(prepareCache, sql), (statement) => + Effect.flatMap(statementFor(sql, params), (statement) => runStatementValues(statement, params), ); -- 2.54.0