Skip to content

Commit 447f525

Browse files
committed
improvement(tables): cut the DB round trips a table read and write spend on protocol
The grid's first page spent more time on round trips than on work. Four of them were avoidable: - `pendingDeleteMask` probed `table_jobs` on every read, though the table a request just loaded already carries its latest non-export job, and the `one_active_per_table` unique index makes that row the running delete when one exists. Callers that hold a table across a long walk (the export stream, the snapshot builder) keep probing per page, so a delete starting mid-walk still begins masking. - The run-state sidecar was read for every table, including the ones that declare no workflow group and therefore cannot have a row — four chunked queries on a 1000-row page, all returning nothing. - The drain opened a transaction per batch. The guards are fixed for the call, so each extra batch paid `BEGIN` + `set_config` + `COMMIT` for nothing. - `setTableTxTimeouts` issued three `SET LOCAL` statements; `set_config(…, true)` is the same thing and fits in one round trip, as the read guards already do. A 1000-row page goes from 22 statements to 14, a 50-row page from 15 to 13, and every write transaction drops two.
1 parent acc6cbd commit 447f525

11 files changed

Lines changed: 361 additions & 91 deletions

File tree

‎apps/sim/app/api/v1/tables/[tableId]/rows/route.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -185,6 +185,8 @@ export const GET = withRouteHandler(async (request: NextRequest, context: TableR
185185
offset: validated.offset,
186186
includeTotal: validated.includeTotal,
187187
withExecutions: false,
188+
// `table` was loaded a few lines above, for this read.
189+
trustLoadedJob: true,
188190
},
189191
requestId
190192
)

‎apps/sim/lib/table/__tests__/service-filter-threading.test.ts‎

Lines changed: 88 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,7 @@ vi.mock('@/lib/table/rows/executions', () => ({
5050
})),
5151
loadExecutionsByRow: mockLoadExecutionsByRow,
5252
loadExecutionsForRow: vi.fn(async () => ({})),
53+
tableMayHaveRunState: vi.fn(() => true),
5354
writeExecutionsPatch: vi.fn(async () => 'wrote'),
5455
}))
5556

@@ -65,6 +66,7 @@ vi.mock('@/lib/table/validation', () => ({
6566
checkBatchUniqueConstraintsDb: vi.fn(async () => ({ valid: true, errors: [] })),
6667
}))
6768

69+
import { tableMayHaveRunState } from '@/lib/table/rows/executions'
6870
import {
6971
deleteRow,
7072
deleteRowsByFilter,
@@ -666,3 +668,89 @@ describe('queryRows byte budget', () => {
666668
})
667669
})
668670
})
671+
672+
/**
673+
* Two reads the row path used to make unconditionally, each one round trip on the grid's hot
674+
* page. Both are now answered from state the caller already holds; these pin that the query is
675+
* actually skipped rather than merely ignored, and that the fallbacks still run.
676+
*/
677+
describe('queryRows round-trip elision', () => {
678+
beforeEach(() => {
679+
vi.clearAllMocks()
680+
resetDbChainMock()
681+
vi.mocked(tableMayHaveRunState).mockReturnValue(true)
682+
})
683+
684+
const hydrated = (
685+
fields: Partial<Pick<TableDefinition, 'jobStatus' | 'jobType'>>
686+
): TableDefinition => ({ ...TABLE, jobStatus: null, jobType: null, ...fields })
687+
688+
it('skips the delete-job probe when the hydrated table shows no running delete', async () => {
689+
await queryRows(
690+
hydrated({}),
691+
{ limit: 5, includeTotal: false, withExecutions: false, trustLoadedJob: true },
692+
'req-1'
693+
)
694+
695+
// With no probe, the drain batch is the FIRST bounded query rather than the second.
696+
expect(dbChainMockFns.limit).toHaveBeenNthCalledWith(1, 6)
697+
})
698+
699+
it('still probes when the hydrated table shows a running delete job', async () => {
700+
await queryRows(
701+
hydrated({ jobStatus: 'running', jobType: 'delete' }),
702+
{ limit: 5, includeTotal: false, withExecutions: false, trustLoadedJob: true },
703+
'req-1'
704+
)
705+
706+
expect(dbChainMockFns.limit).toHaveBeenNthCalledWith(1, 1)
707+
expect(dbChainMockFns.limit).toHaveBeenNthCalledWith(2, 6)
708+
})
709+
710+
/** An unhydrated definition cannot rule the job out, so it keeps the lookup it always had. */
711+
it('still probes when the table carries no job fields at all', async () => {
712+
await queryRows(
713+
TABLE,
714+
{ limit: 5, includeTotal: false, withExecutions: false, trustLoadedJob: true },
715+
'req-1'
716+
)
717+
718+
expect(dbChainMockFns.limit).toHaveBeenNthCalledWith(1, 1)
719+
expect(dbChainMockFns.limit).toHaveBeenNthCalledWith(2, 6)
720+
})
721+
722+
/**
723+
* A caller that holds its table across a long walk (the export stream) must keep re-asking, so
724+
* a delete job starting mid-walk still begins masking its doomed rows.
725+
*/
726+
it('still probes for a caller that did not vouch for its table', async () => {
727+
await queryRows(hydrated({}), { limit: 5, includeTotal: false, withExecutions: false }, 'req-1')
728+
729+
expect(dbChainMockFns.limit).toHaveBeenNthCalledWith(1, 1)
730+
expect(dbChainMockFns.limit).toHaveBeenNthCalledWith(2, 6)
731+
})
732+
733+
it('skips the run-state read for a table that can hold none, still reporting empty executions', async () => {
734+
vi.mocked(tableMayHaveRunState).mockReturnValue(false)
735+
dbChainMockFns.limit.mockResolvedValueOnce([])
736+
dbChainMockFns.limit.mockResolvedValueOnce([
737+
{ id: 'row-1', data: {}, position: 0, orderKey: 'a0', createdAt: null, updatedAt: null },
738+
])
739+
740+
const result = await queryRows(TABLE, { limit: 5, includeTotal: false }, 'req-1')
741+
742+
expect(mockLoadExecutionsByRow).not.toHaveBeenCalled()
743+
expect(result.rows[0].executions).toEqual({})
744+
})
745+
746+
it('reads run state for a table that can hold it', async () => {
747+
dbChainMockFns.limit.mockResolvedValueOnce([])
748+
dbChainMockFns.limit.mockResolvedValueOnce([
749+
{ id: 'row-1', data: {}, position: 0, orderKey: 'a0', createdAt: null, updatedAt: null },
750+
])
751+
752+
await queryRows(TABLE, { limit: 5, includeTotal: false }, 'req-1')
753+
754+
expect(mockLoadExecutionsByRow).toHaveBeenCalledTimes(1)
755+
})
756+
})

‎apps/sim/lib/table/__tests__/update-row.test.ts‎

Lines changed: 33 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,31 @@ function findExecutedRawSql(substring: string): string | undefined {
6868
return undefined
6969
}
7070

71+
/** Every string reachable from a drizzle `sql` fragment — its literal chunks AND its bound values. */
72+
function collectStrings(node: unknown, out: string[] = []): string[] {
73+
if (typeof node === 'string') out.push(node)
74+
else if (Array.isArray(node)) for (const entry of node) collectStrings(entry, out)
75+
else if (node && typeof node === 'object')
76+
for (const entry of Object.values(node as Record<string, unknown>)) collectStrings(entry, out)
77+
return out
78+
}
79+
80+
/**
81+
* Whether one `set_config(<setting>, '<value>', true)` guard was executed.
82+
*
83+
* The guards bind their values as parameters, so the setting name lives in the statement's
84+
* literal chunks while the duration lives in its bound values — asserting on a rendered string
85+
* would only re-check the placeholder.
86+
*/
87+
function executedTxTimeout(setting: string, value: string): boolean {
88+
return dbChainMockFns.execute.mock.calls.some(([arg]) => {
89+
const strings = collectStrings(arg)
90+
return (
91+
strings.some((entry) => entry.includes(`set_config('${setting}'`)) && strings.includes(value)
92+
)
93+
})
94+
}
95+
7196
/**
7297
* The `data` payload of the last `.set(...)` row write. `updateRow` always writes a JSONB merge
7398
* (`data = data || {changed}::jsonb`), so this is a `sql` fragment exposing `{ strings, values }`.
@@ -420,11 +445,9 @@ describe('mutation paths — SET LOCAL timeouts', () => {
420445
insertRow({ tableId: 'tbl-1', data: { name: 'a' }, workspaceId: 'ws-1' }, TABLE, 'req-1')
421446
).rejects.toBeDefined()
422447

423-
expect(findExecutedRawSql("SET LOCAL statement_timeout = '10000ms'")).toBeDefined()
424-
expect(findExecutedRawSql("SET LOCAL lock_timeout = '3000ms'")).toBeDefined()
425-
expect(
426-
findExecutedRawSql("SET LOCAL idle_in_transaction_session_timeout = '5000ms'")
427-
).toBeDefined()
448+
expect(executedTxTimeout('statement_timeout', '10000ms')).toBe(true)
449+
expect(executedTxTimeout('lock_timeout', '3000ms')).toBe(true)
450+
expect(executedTxTimeout('idle_in_transaction_session_timeout', '5000ms')).toBe(true)
428451
})
429452

430453
it('batchInsertRows raises statement_timeout to 60s', async () => {
@@ -436,7 +459,7 @@ describe('mutation paths — SET LOCAL timeouts', () => {
436459
)
437460
).rejects.toBeDefined()
438461

439-
expect(findExecutedRawSql("SET LOCAL statement_timeout = '60000ms'")).toBeDefined()
462+
expect(executedTxTimeout('statement_timeout', '60000ms')).toBe(true)
440463
})
441464

442465
it('replaceTableRows scales statement_timeout with (existing + new) row count', async () => {
@@ -450,7 +473,7 @@ describe('mutation paths — SET LOCAL timeouts', () => {
450473
)
451474

452475
// (100_000 + 50_000) × 3ms/row = 450_000ms; above 120_000 floor, below 600_000 cap
453-
expect(findExecutedRawSql("SET LOCAL statement_timeout = '450000ms'")).toBeDefined()
476+
expect(executedTxTimeout('statement_timeout', '450000ms')).toBe(true)
454477
})
455478

456479
it('replaceTableRows caps scaled timeout at 10 minutes for very large tables', async () => {
@@ -459,7 +482,7 @@ describe('mutation paths — SET LOCAL timeouts', () => {
459482
await replaceTableRows({ tableId: 'tbl-1', workspaceId: 'ws-1', rows: [] }, hugeTable, 'req-1')
460483

461484
// 10M × 3ms = 30M ms, capped at 600_000ms (10 min)
462-
expect(findExecutedRawSql("SET LOCAL statement_timeout = '600000ms'")).toBeDefined()
485+
expect(executedTxTimeout('statement_timeout', '600000ms')).toBe(true)
463486
})
464487

465488
it('replaceTableRows uses the 120s floor on small tables', async () => {
@@ -472,7 +495,7 @@ describe('mutation paths — SET LOCAL timeouts', () => {
472495
)
473496

474497
// 12 × 3ms = 36ms → floored at 120_000ms
475-
expect(findExecutedRawSql("SET LOCAL statement_timeout = '120000ms'")).toBeDefined()
498+
expect(executedTxTimeout('statement_timeout', '120000ms')).toBe(true)
476499
})
477500

478501
it('renameColumn is metadata-only — no per-row JSONB rewrite regardless of row count', async () => {
@@ -491,7 +514,7 @@ describe('mutation paths — SET LOCAL timeouts', () => {
491514
await deleteColumn({ tableId: 'tbl-1', columnName: 'age' }, 'req-1')
492515

493516
// 100 × 2ms = 200ms → floored at 60_000ms
494-
expect(findExecutedRawSql("SET LOCAL statement_timeout = '60000ms'")).toBeDefined()
517+
expect(executedTxTimeout('statement_timeout', '60000ms')).toBe(true)
495518
})
496519

497520
it('replaceTableRows acquires the per-table advisory lock to serialize concurrent replaces', async () => {

‎apps/sim/lib/table/application/rows.test.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -769,6 +769,7 @@ describe('row query and upsert application semantics', () => {
769769
includeTotal: false,
770770
withExecutions: false,
771771
runStateBudgetBytes: TABLE_LIMITS.MAX_ROW_RUN_STATE_BYTES,
772+
trustLoadedJob: true,
772773
},
773774
expect.any(String)
774775
)

‎apps/sim/lib/table/application/rows.ts‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -436,6 +436,8 @@ export const listTableRows = defineAuthorizedTableUseCase({
436436
includeTotal: false,
437437
withExecutions: input.includeRunState ?? false,
438438
runStateBudgetBytes: TABLE_LIMITS.MAX_ROW_RUN_STATE_BYTES,
439+
// `context.table` was loaded by this use case's own resolver, for this read.
440+
trustLoadedJob: true,
439441
},
440442
requestId(input)
441443
)
@@ -573,6 +575,8 @@ export const queryTableRows = defineAuthorizedTableUseCase({
573575
withExecutions: input.includeRunState ?? false,
574576
runStateBudgetBytes: TABLE_LIMITS.MAX_ROW_RUN_STATE_BYTES,
575577
columnIds,
578+
// `context.table` was loaded by this use case's own resolver, for this read.
579+
trustLoadedJob: true,
576580
},
577581
requestId(input),
578582
readProvenance

‎apps/sim/lib/table/rows/executions.test.ts‎

Lines changed: 42 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,8 +4,12 @@
44
import { dbChainMock, dbChainMockFns, resetDbChainMock } from '@sim/testing'
55
import { beforeEach, describe, expect, it, vi } from 'vitest'
66
import type { DbOrTx } from '@/lib/db/types'
7-
import { loadExecutionsByRow, writeExecutionsPatch } from '@/lib/table/rows/executions'
8-
import type { RowExecutionMetadata } from '@/lib/table/types'
7+
import {
8+
loadExecutionsByRow,
9+
tableMayHaveRunState,
10+
writeExecutionsPatch,
11+
} from '@/lib/table/rows/executions'
12+
import type { RowExecutionMetadata, TableSchema } from '@/lib/table/types'
913

1014
const EXECUTION_STATE: RowExecutionMetadata = {
1115
status: 'running',
@@ -258,3 +262,39 @@ describe('loadExecutionsByRow', () => {
258262
expect(byRow.size).toBe(750)
259263
})
260264
})
265+
266+
describe('tableMayHaveRunState', () => {
267+
const column = (overrides: Partial<TableSchema['columns'][number]> = {}) => ({
268+
id: 'col_1',
269+
name: 'title',
270+
type: 'string' as const,
271+
...overrides,
272+
})
273+
274+
it('is false for a schema that declares no group', () => {
275+
expect(tableMayHaveRunState({ columns: [column()] })).toBe(false)
276+
expect(tableMayHaveRunState({ columns: [column()], workflowGroups: [] })).toBe(false)
277+
})
278+
279+
it('is true once the schema declares a group', () => {
280+
expect(
281+
tableMayHaveRunState({
282+
columns: [column()],
283+
workflowGroups: [{ id: 'group-1' }] as TableSchema['workflowGroups'],
284+
})
285+
).toBe(true)
286+
})
287+
288+
/**
289+
* A column still pointing at a group is group state whatever the group list says, so an
290+
* unexpected schema shape must keep the sidecar read rather than silently drop run state.
291+
*/
292+
it('is true for a column that still names a group the list has lost', () => {
293+
expect(
294+
tableMayHaveRunState({
295+
columns: [column({ workflowGroupId: 'group-1' })],
296+
workflowGroups: [],
297+
})
298+
).toBe(true)
299+
})
300+
})

‎apps/sim/lib/table/rows/executions.ts‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,24 @@ interface LoadExecutionsOptions {
4040
budgetBytes?: number
4141
}
4242

43+
/**
44+
* Whether a table can have any run-state sidecar at all.
45+
*
46+
* `tableRowExecutions` is keyed by `(rowId, groupId)`, and every writer takes its `groupId` from
47+
* a group on the table's own schema. Group and column deletes strip the matching sidecar rows in
48+
* the same transaction that removes the group ({@link stripGroupExecutions}), so a schema that
49+
* declares no group cannot have a surviving row — the sidecar read would return nothing, and the
50+
* caller would fill in the same empty map it gets by skipping.
51+
*
52+
* Both signals are checked rather than just `workflowGroups`: a column still carrying a
53+
* `workflowGroupId` means the table has group state whatever the group list looks like, so an
54+
* unexpected schema shape keeps the query instead of silently dropping run state.
55+
*/
56+
export function tableMayHaveRunState(schema: TableSchema): boolean {
57+
if (schema.workflowGroups && schema.workflowGroups.length > 0) return true
58+
return schema.columns.some((column) => column.workflowGroupId !== undefined)
59+
}
60+
4361
/**
4462
* Loads `tableRowExecutions` rows for the given row ids and groups them into a
4563
* `Map<rowId, RowExecutions>` suitable for plugging into `TableRow.executions`.

‎apps/sim/lib/table/rows/pending-delete-mask.ts‎

Lines changed: 42 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,43 @@ import type { TableDefinition, TableDeleteJobPayload } from '@/lib/table/types'
99

1010
const logger = createLogger('TablePendingDeleteMask')
1111

12+
/**
13+
* Whether {@link PendingDeleteMaskOptions.trustLoadedJob} can rule a running delete job out.
14+
*
15+
* Every table loaded through `getTableById` / `listTables` already carries its latest non-export
16+
* job, folded into that same SELECT as a lateral, so a caller that loaded its table for this very
17+
* read already holds the answer and the lookup is pure overhead on the hot path.
18+
*
19+
* The derivation is exact, not a heuristic. `table_jobs_one_active_per_table` is unique on
20+
* `table_id WHERE status = 'running' AND type <> 'export'` — the same predicate the lateral
21+
* filters on — so a running delete job is the ONLY running non-export job on its table, and no
22+
* further non-export job can be inserted while it holds that slot. It is therefore the newest
23+
* non-export job by `started_at`, which is exactly the row the lateral returns.
24+
*
25+
* `jobStatus === undefined` means the fields were never hydrated (a `TableDefinition` assembled
26+
* by some other path), which is indistinguishable from "no job" in the shape alone — so that case
27+
* falls back to the query rather than assuming. A hydrated table with no job has
28+
* `jobStatus: null`.
29+
*/
30+
function hydratedJobRulesOutDelete(table: TableDefinition): boolean {
31+
if (table.jobStatus === undefined) return false
32+
return !(table.jobStatus === 'running' && table.jobType === 'delete')
33+
}
34+
35+
export interface PendingDeleteMaskOptions {
36+
/**
37+
* Answer from `table`'s own latest-job fields when they rule a running delete out, instead of
38+
* querying for one.
39+
*
40+
* Only for a caller whose `table` was loaded for this read: the fields are then as fresh as the
41+
* query would have been. A caller that loads a table once and then pages for a while — the
42+
* export runner, the snapshot builder — must NOT set this, because a delete job starting
43+
* mid-walk would never appear in its snapshot and its later pages would stop masking doomed
44+
* rows. Those callers keep re-asking per page, which is what makes the mask appear mid-walk.
45+
*/
46+
trustLoadedJob?: boolean
47+
}
48+
1249
/**
1350
* Visibility mask for a running delete job: returns a clause keeping only rows the job will NOT
1451
* delete, or `undefined` when no delete job is running. The job's persisted scope
@@ -20,7 +57,11 @@ const logger = createLogger('TablePendingDeleteMask')
2057
* `(doomed) IS NOT TRUE` rather than `NOT (doomed)`: JSONB predicates evaluate to NULL on missing
2158
* cells, and those rows are NOT selected for deletion (NULL ≠ TRUE) — they must stay visible.
2259
*/
23-
export async function pendingDeleteMask(table: TableDefinition): Promise<SQL | undefined> {
60+
export async function pendingDeleteMask(
61+
table: TableDefinition,
62+
options?: PendingDeleteMaskOptions
63+
): Promise<SQL | undefined> {
64+
if (options?.trustLoadedJob && hydratedJobRulesOutDelete(table)) return undefined
2465
const [job] = await db
2566
.select({ payload: tableJobs.payload })
2667
.from(tableJobs)

0 commit comments

Comments
 (0)