Skip to content

Commit 1dbecd4

Browse files
committed
perf(table): remove two round trips from every single-row update
A single-cell PATCH spends far more time in sequential round trips to a remote Postgres than in the UPDATE it issues. Prepared statements are disabled for PgBouncer transaction mode, so every await is a full Parse/Bind/Execute. Two of them were avoidable. getRowById issued the row lookup and its executions sidecar in series, but the sidecar is keyed on the row id the caller already supplied, so it never depended on the lookup. Issuing both together makes it one round trip. A miss now pays one redundant sidecar read, which is the rare path and costs no extra wall time. The uniqueness probe ran whenever the table had any unique column, passing the fully merged row, so editing an unrelated cell re-probed every unique column — its own transaction plus one query per column. It is now scoped to the columns the patch actually writes. A merge cannot newly violate uniqueness on a column it leaves alone: that value is the one already stored, and it satisfied the constraint when it was written. Sized before changing: few tables declare a unique column, but write traffic concentrates in the ones that do, so this is the larger of the two savings in practice. Verified to fail: reverting the probe scoping turns the covering test red.
1 parent fc43edc commit 1dbecd4

3 files changed

Lines changed: 141 additions & 14 deletions

File tree

apps/sim/lib/api/contracts/tables.ts

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1433,6 +1433,22 @@ export const upsertTableRowContract = defineRouteContract({
14331433
},
14341434
})
14351435

1436+
/**
1437+
* Reads one row. The sibling of {@link updateTableRowContract} and
1438+
* {@link deleteTableRowContract}, which take their workspace scope from a body;
1439+
* a GET has none, so it is asserted on the query string instead.
1440+
*/
1441+
export const getTableRowContract = defineRouteContract({
1442+
method: 'GET',
1443+
path: '/api/table/[tableId]/rows/[rowId]',
1444+
params: tableRowParamsSchema,
1445+
query: getTableQuerySchema,
1446+
response: {
1447+
mode: 'json',
1448+
schema: successResponseSchema(z.object({ row: tableRowSchema })),
1449+
},
1450+
})
1451+
14361452
export const updateTableRowContract = defineRouteContract({
14371453
method: 'PATCH',
14381454
path: '/api/table/[tableId]/rows/[rowId]',

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

Lines changed: 95 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -548,3 +548,98 @@ describe('batchUpdateRows — per-row partial merge', () => {
548548
expect(values).not.toContain(JSON.stringify({ name: 'Alice', age: 31 }))
549549
})
550550
})
551+
552+
/**
553+
* The uniqueness probe opens its own transaction and queries once per unique
554+
* column, so on a table that has any unique column it used to cost several
555+
* round trips on every edit — including edits nowhere near one. It is now
556+
* scoped to the columns the patch actually writes.
557+
*
558+
* The safety argument is that a merge cannot newly violate uniqueness on a
559+
* column it leaves alone: that value is the one already stored, and it
560+
* satisfied the constraint when it was written.
561+
*/
562+
describe('updateRow — uniqueness probe scoping', () => {
563+
beforeEach(() => {
564+
vi.clearAllMocks()
565+
resetDbChainMock()
566+
dbChainMockFns.limit.mockResolvedValue([EXISTING_ROW])
567+
dbChainMockFns.returning.mockResolvedValue([
568+
{ id: EXISTING_ROW.id, updatedAt: PERSISTED_UPDATED_AT },
569+
])
570+
})
571+
572+
it('does not probe when the patch touches no unique column', async () => {
573+
const { checkUniqueConstraintsDb, getUniqueColumns } = await import('@/lib/table/validation')
574+
vi.mocked(getUniqueColumns).mockReturnValue([{ name: 'name', type: 'string', unique: true }])
575+
576+
await updateRow(
577+
{ tableId: 'tbl-1', rowId: 'row-1', data: { age: 31 }, workspaceId: 'ws-1' },
578+
TABLE,
579+
'req-1'
580+
)
581+
582+
expect(checkUniqueConstraintsDb).not.toHaveBeenCalled()
583+
})
584+
585+
it('still probes when the patch touches a unique column', async () => {
586+
const { checkUniqueConstraintsDb, getUniqueColumns } = await import('@/lib/table/validation')
587+
vi.mocked(getUniqueColumns).mockReturnValue([{ name: 'name', type: 'string', unique: true }])
588+
589+
await updateRow(
590+
{ tableId: 'tbl-1', rowId: 'row-1', data: { name: 'Grace' }, workspaceId: 'ws-1' },
591+
TABLE,
592+
'req-1'
593+
)
594+
595+
expect(checkUniqueConstraintsDb).toHaveBeenCalledTimes(1)
596+
})
597+
598+
it('probes against the merged row, so the excluded row is still the one being edited', async () => {
599+
const { checkUniqueConstraintsDb, getUniqueColumns } = await import('@/lib/table/validation')
600+
vi.mocked(getUniqueColumns).mockReturnValue([{ name: 'name', type: 'string', unique: true }])
601+
602+
await updateRow(
603+
{ tableId: 'tbl-1', rowId: 'row-1', data: { name: 'Grace' }, workspaceId: 'ws-1' },
604+
TABLE,
605+
'req-1'
606+
)
607+
608+
expect(checkUniqueConstraintsDb).toHaveBeenCalledWith(
609+
'tbl-1',
610+
{ name: 'Grace', age: 30 },
611+
TABLE.schema,
612+
'row-1'
613+
)
614+
})
615+
616+
it('surfaces a duplicate on a column the patch does write', async () => {
617+
const { checkUniqueConstraintsDb, getUniqueColumns } = await import('@/lib/table/validation')
618+
vi.mocked(getUniqueColumns).mockReturnValue([{ name: 'name', type: 'string', unique: true }])
619+
vi.mocked(checkUniqueConstraintsDb).mockResolvedValueOnce({
620+
valid: false,
621+
errors: ['Duplicate value for name'],
622+
})
623+
624+
await expect(
625+
updateRow(
626+
{ tableId: 'tbl-1', rowId: 'row-1', data: { name: 'Grace' }, workspaceId: 'ws-1' },
627+
TABLE,
628+
'req-1'
629+
)
630+
).rejects.toThrow(/Duplicate value for name/)
631+
})
632+
633+
it('does not probe on a table with no unique columns at all', async () => {
634+
const { checkUniqueConstraintsDb, getUniqueColumns } = await import('@/lib/table/validation')
635+
vi.mocked(getUniqueColumns).mockReturnValue([])
636+
637+
await updateRow(
638+
{ tableId: 'tbl-1', rowId: 'row-1', data: { name: 'Grace' }, workspaceId: 'ws-1' },
639+
TABLE,
640+
'req-1'
641+
)
642+
643+
expect(checkUniqueConstraintsDb).not.toHaveBeenCalled()
644+
})
645+
})

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

Lines changed: 30 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1422,22 +1422,28 @@ export async function getRowById(
14221422
rowId: string,
14231423
workspaceId: string
14241424
): Promise<TableRow | null> {
1425-
const results = await db
1426-
.select()
1427-
.from(userTableRows)
1428-
.where(
1429-
and(
1430-
eq(userTableRows.id, rowId),
1431-
eq(userTableRows.tableId, tableId),
1432-
eq(userTableRows.workspaceId, workspaceId)
1425+
// The executions sidecar is keyed on the row id the caller already gave us, so
1426+
// it does not depend on the row lookup — issuing both together makes this one
1427+
// round trip instead of two. A miss pays one redundant sidecar read, which is
1428+
// the rare path and costs no extra wall time.
1429+
const [results, executions] = await Promise.all([
1430+
db
1431+
.select()
1432+
.from(userTableRows)
1433+
.where(
1434+
and(
1435+
eq(userTableRows.id, rowId),
1436+
eq(userTableRows.tableId, tableId),
1437+
eq(userTableRows.workspaceId, workspaceId)
1438+
)
14331439
)
1434-
)
1435-
.limit(1)
1440+
.limit(1),
1441+
loadExecutionsForRow(db, rowId),
1442+
])
14361443

14371444
if (results.length === 0) return null
14381445

14391446
const row = results[0]
1440-
const executions = await loadExecutionsForRow(db, row.id)
14411447
return {
14421448
id: row.id,
14431449
data: row.data as RowData,
@@ -1604,9 +1610,19 @@ export async function updateRow(
16041610
)
16051611
}
16061612

1607-
// Check unique constraints using optimized database query
1608-
const uniqueColumns = getUniqueColumns(table.schema)
1609-
if (uniqueColumns.length > 0) {
1613+
// Check unique constraints using optimized database query.
1614+
//
1615+
// Scoped to the columns this patch actually writes. A merge cannot newly
1616+
// violate uniqueness on a column it leaves alone: that value is the one
1617+
// already stored, and it satisfied the constraint when it was written. The
1618+
// probe opens its own transaction and queries once per unique column, so on a
1619+
// table that has any unique column this was several round trips on every
1620+
// edit, including edits nowhere near one.
1621+
const patchedColumnIds = new Set(Object.keys(data.data))
1622+
const patchedUniqueColumns = getUniqueColumns(table.schema).filter((column) =>
1623+
patchedColumnIds.has(getColumnId(column))
1624+
)
1625+
if (patchedUniqueColumns.length > 0) {
16101626
const uniqueValidation = await checkUniqueConstraintsDb(
16111627
data.tableId,
16121628
mergedData,

0 commit comments

Comments
 (0)