Skip to content

Commit d77dbba

Browse files
committed
perf(table): read a table and its latest job in one round trip
getTableById issued the table SELECT and then awaited latestJobForTable, so every table request paid two sequential round trips. With prepared statements disabled for PgBouncer transaction mode every await is a full round trip, and this loader is on essentially every table route. The job read cannot be skipped: a table's reported rowCount is the stored count minus the job's pendingDeleteRemaining, so dropping it would overstate the count during a pending delete and could wrongly reject inserts as over capacity. An opt-out flag would have made that a caller's trap. Instead the job is read in the same statement, as a correlated jsonb subquery in the select list — the select-list form of a LEFT JOIN LATERAL, which is what drizzle can type here. Output is unchanged for every input. latestJobForTable is deleted rather than left dangling: getTableById was its only caller, and keeping it would have carried a third copy of the exports-excluded / newest-started_at / limit-one rule. mapJobRow is now exported so the batch path and the lateral share one implementation of the doomedCount and pendingDeleteRemaining logic. The batch DISTINCT ON path used by the list endpoint is untouched. Verified to fail: dropping the export filter, reversing or re-keying the sort, dropping the limit, dropping the correlation, loosening either doomedCount condition, and removing the rowCount subtraction each turn tests red. Dropping the lateral from the projection initially survived, because the shared db mock returns queued rows regardless of predicate; a projection assertion now covers it.
1 parent 80ce6ae commit d77dbba

5 files changed

Lines changed: 352 additions & 34 deletions

File tree

apps/sim/lib/api/list-convention.test.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,8 @@ vi.mock('@/lib/table/billing', () => ({
3737
}))
3838
vi.mock('@/lib/table/jobs/service', () => ({
3939
EMPTY_JOB_FIELDS: {},
40-
latestJobForTable: vi.fn(async () => null),
40+
latestNonExportJobJson: vi.fn(() => null),
41+
mapJobRow: vi.fn(() => ({})),
4142
latestJobsForTables: vi.fn(async () => new Map()),
4243
}))
4344
vi.mock('@/lib/table/events', () => ({ appendTableEvent: vi.fn() }))
Lines changed: 110 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,110 @@
1+
/**
2+
* @vitest-environment node
3+
*/
4+
import { schemaMock } from '@sim/testing'
5+
import { describe, expect, it } from 'vitest'
6+
import {
7+
EMPTY_JOB_FIELDS,
8+
type LatestJobRow,
9+
latestNonExportJobJson,
10+
mapJobRow,
11+
} from '@/lib/table/jobs/service'
12+
13+
function job(overrides: Partial<LatestJobRow>): LatestJobRow {
14+
return {
15+
id: 'job-1',
16+
type: 'delete',
17+
status: 'running',
18+
rowsProcessed: 0,
19+
error: null,
20+
payload: null,
21+
...overrides,
22+
}
23+
}
24+
25+
describe('mapJobRow', () => {
26+
it('returns the empty fields when the table has no job row', () => {
27+
expect(mapJobRow(null)).toEqual(EMPTY_JOB_FIELDS)
28+
expect(mapJobRow(undefined)).toEqual(EMPTY_JOB_FIELDS)
29+
})
30+
31+
it('projects a running delete job and its remaining doomed rows', () => {
32+
expect(mapJobRow(job({ rowsProcessed: 4, payload: { doomedCount: 10 } }))).toEqual({
33+
jobStatus: 'running',
34+
jobId: 'job-1',
35+
jobType: 'delete',
36+
jobError: null,
37+
jobRowsProcessed: 4,
38+
pendingDeleteRemaining: 6,
39+
})
40+
})
41+
42+
it('ignores doomedCount once the delete job is terminal', () => {
43+
expect(
44+
mapJobRow(job({ status: 'ready', rowsProcessed: 4, payload: { doomedCount: 10 } }))
45+
.pendingDeleteRemaining
46+
).toBe(0)
47+
})
48+
49+
it('ignores doomedCount for a running job that is not a delete', () => {
50+
expect(
51+
mapJobRow(job({ type: 'import', rowsProcessed: 4, payload: { doomedCount: 10 } }))
52+
.pendingDeleteRemaining
53+
).toBe(0)
54+
})
55+
56+
it('treats a missing doomedCount as zero and never goes negative', () => {
57+
expect(mapJobRow(job({ rowsProcessed: 4 })).pendingDeleteRemaining).toBe(0)
58+
expect(
59+
mapJobRow(job({ rowsProcessed: 25, payload: { doomedCount: 10 } })).pendingDeleteRemaining
60+
).toBe(0)
61+
})
62+
63+
it('carries a failed job error through', () => {
64+
expect(mapJobRow(job({ status: 'failed', error: 'boom' }))).toMatchObject({
65+
jobStatus: 'failed',
66+
jobError: 'boom',
67+
})
68+
})
69+
})
70+
71+
/**
72+
* The lateral is a raw `sql` fragment, so the mocked drizzle `sql` tag is the only
73+
* place its text is observable — and the text IS the contract (`getTableById` would
74+
* otherwise silently return a different job than `latestJobsForTables` does).
75+
*/
76+
function renderLateral(): { text: string; values: unknown[] } {
77+
// double-cast-allowed: the mocked drizzle `sql` tag exposes the raw template parts
78+
const fragment = latestNonExportJobJson(schemaMock.userTableDefinitions.id) as unknown as {
79+
strings: string[]
80+
values: unknown[]
81+
}
82+
return { text: fragment.strings.join(' ? ').replace(/\s+/g, ' '), values: fragment.values }
83+
}
84+
85+
describe('latestNonExportJobJson', () => {
86+
it('excludes export jobs', () => {
87+
expect(renderLateral().text).toContain("<> 'export'")
88+
})
89+
90+
it('takes the single newest job by started_at', () => {
91+
const { text, values } = renderLateral()
92+
expect(text).toContain('order by ? desc')
93+
expect(text).toContain('limit 1')
94+
expect(values).toContain(schemaMock.tableJobs.startedAt)
95+
})
96+
97+
it('correlates the subquery to the outer table id', () => {
98+
const { text, values } = renderLateral()
99+
expect(text).toContain('where ? = ?')
100+
expect(values).toContain(schemaMock.tableJobs.tableId)
101+
expect(values).toContain(schemaMock.userTableDefinitions.id)
102+
})
103+
104+
it('projects every field mapJobRow reads', () => {
105+
const { text } = renderLateral()
106+
for (const key of ['id', 'type', 'status', 'rowsProcessed', 'error', 'payload']) {
107+
expect(text).toContain(`'${key}',`)
108+
}
109+
})
110+
})

apps/sim/lib/table/jobs/service.ts

Lines changed: 47 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -13,8 +13,8 @@
1313

1414
import { db } from '@sim/db'
1515
import { tableJobs, userTableDefinitions, userTableRows } from '@sim/db/schema'
16+
import type { Column, SQL } from 'drizzle-orm'
1617
import { and, asc, desc, eq, gt, inArray, ne, or, sql } from 'drizzle-orm'
17-
import type { DbOrTx } from '@/lib/db/types'
1818
import { pendingDeleteMask } from '@/lib/table/rows/pending-delete-mask'
1919
import type {
2020
RowData,
@@ -25,7 +25,7 @@ import type {
2525
} from '@/lib/table/types'
2626

2727
/** Job fields projected onto a {@link TableDefinition}, derived from its latest `table_jobs` row. */
28-
interface DerivedJobFields {
28+
export interface DerivedJobFields {
2929
jobStatus: TableDefinition['jobStatus']
3030
jobId: string | null
3131
jobType: TableDefinition['jobType']
@@ -49,18 +49,22 @@ export const EMPTY_JOB_FIELDS: DerivedJobFields = {
4949
pendingDeleteRemaining: 0,
5050
}
5151

52-
function mapJobRow(
53-
row:
54-
| {
55-
id: string
56-
type: string
57-
status: string
58-
rowsProcessed: number
59-
error: string | null
60-
payload: unknown
61-
}
62-
| undefined
63-
): DerivedJobFields {
52+
/**
53+
* The shape every latest-job read produces, whether it comes back as query columns
54+
* (the batch `DISTINCT ON`) or as one jsonb object (the correlated lateral folded
55+
* into the table SELECT). The single source of truth for the doomed-count rule is
56+
* {@link mapJobRow} — never re-derive `pendingDeleteRemaining` at a call site.
57+
*/
58+
export interface LatestJobRow {
59+
id: string
60+
type: string
61+
status: string
62+
rowsProcessed: number
63+
error: string | null
64+
payload: unknown
65+
}
66+
67+
export function mapJobRow(row: LatestJobRow | null | undefined): DerivedJobFields {
6468
if (!row) return EMPTY_JOB_FIELDS
6569
const doomedCount =
6670
row.type === 'delete' && row.status === 'running'
@@ -86,22 +90,36 @@ const JOB_PROJECTION = {
8690
} as const
8791

8892
/**
89-
* The latest job for one table (the running one if present, else the most recent terminal).
90-
* Exports are excluded: they're read-only, run concurrently with other jobs, and have their own
91-
* client surface — surfacing one here would clobber the import/delete/backfill status the tray
92-
* and SSE consumer derive from these fields.
93+
* The latest non-export job for one table, as a single jsonb value correlated to
94+
* `outerTableId` — i.e. a `LEFT JOIN LATERAL (... LIMIT 1) ON true` expressed in the
95+
* select list, which is the form drizzle can type without `leftJoinLateral`.
96+
*
97+
* It exists so {@link getTableById} stays ONE database round trip. With prepared
98+
* statements disabled (PgBouncer transaction mode) every extra `await` is a full
99+
* round trip, and `getTableById` is on essentially every table request. The job row
100+
* cannot simply be skipped: a table's reported `rowCount` is the stored count minus
101+
* this job's `pendingDeleteRemaining`, so the count and the job row are one read.
102+
*
103+
* Semantics match the batch {@link latestJobsForTables} exactly — exports excluded
104+
* (they run concurrently and have their own client surface), newest `started_at`
105+
* first, one row. `NULL` when the table has no such job; feed the result straight to
106+
* {@link mapJobRow}.
93107
*/
94-
export async function latestJobForTable(
95-
tableId: string,
96-
executor: DbOrTx = db
97-
): Promise<DerivedJobFields> {
98-
const [row] = await executor
99-
.select(JOB_PROJECTION)
100-
.from(tableJobs)
101-
.where(and(eq(tableJobs.tableId, tableId), ne(tableJobs.type, 'export')))
102-
.orderBy(desc(tableJobs.startedAt))
103-
.limit(1)
104-
return mapJobRow(row)
108+
export function latestNonExportJobJson(outerTableId: Column | SQL): SQL<LatestJobRow | null> {
109+
return sql<LatestJobRow | null>`(
110+
select jsonb_build_object(
111+
'id', ${tableJobs.id},
112+
'type', ${tableJobs.type},
113+
'status', ${tableJobs.status},
114+
'rowsProcessed', ${tableJobs.rowsProcessed},
115+
'error', ${tableJobs.error},
116+
'payload', ${tableJobs.payload}
117+
)
118+
from ${tableJobs}
119+
where ${tableJobs.tableId} = ${outerTableId} and ${tableJobs.type} <> 'export'
120+
order by ${tableJobs.startedAt} desc
121+
limit 1
122+
)`
105123
}
106124

107125
/** Latest non-export job per table for a batch of ids, via `DISTINCT ON (table_id)`. */

0 commit comments

Comments
 (0)