Repository navigation
fix(tables): log row writes to the change log instead of locking the table definition row - #8765
TheodoreSpeaks wants to merge 1 commit into
Conversation
…table definition row
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
We detected this is a high-risk PR and are running a free ultrareview. An ultrareview is a deeper, multi-pass review that catches hard-to-find bugs a standard review can miss. We'll post the findings when it completes. This PR appears to change a database schema or migrate data, where a missed bug can corrupt or lose records, so a deeper multi-pass review is worth running. Want an ultrareview on every high-risk PR? Set up automated ultrareviews. |
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| UPDATE user_table_definitions d | ||
| SET row_count = actual.n | ||
| SET row_count = actual.n - actual.tail | ||
| FROM ( | ||
| SELECT d2.id, count(r.id)::int AS n | ||
| SELECT d2.id, | ||
| (SELECT count(*) FROM user_table_rows r WHERE r.table_id = d2.id)::int AS n, | ||
| (SELECT coalesce(sum(c.row_delta), 0) FROM user_table_row_changes c | ||
| WHERE c.table_id = d2.id)::int AS tail |
There was a problem hiding this comment.
If the script runs during a fold, this query can read the pending log before waiting for the definition row's lock. The fold then removes that log and commits, but the query writes actual.n - actual.tail from its earlier snapshot. Ten real rows with a pending +3 can end up with a saved count of seven and an empty log. Later folds cannot recover that lost count.
Lock the definition rows first, then read the counts in a fresh statement while keeping those locks.
| data: { key: 'd', name: 'd' }, | ||
| conflictTarget: 'key', |
There was a problem hiding this comment.
This call uses key d, but only keys a, b, and c exist at this point. It tests another insert, not an upsert that changes an existing row. The test could pass even if that separate write path still waits on the held definition row.
Add an upsert using an existing key and a changed value while the lock is held.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
We detected this is a high-risk PR and ran a free ultrareview. An ultrareview is a deeper, multi-pass review that catches hard-to-find bugs a standard review can miss.
This PR appears to change a database schema or migrate data, where a missed bug can corrupt or lose records, so a deeper multi-pass review is worth running.
Want an ultrareview on every high-risk PR? Set up automated ultrareviews.
4 issues found across 9 files
Confidence score: 3/5
apply-dev-table-triggers.tscan permanently undercount if reconciliation uses a pre-foldactualvalue after the fold commits. Make reconciliation safe against a concurrent fold.row-writes.integration.tsonly detects the 0390 deferred UPDATE trigger, so on 0390/0396 it exercises the old definition-row triggers rather than the lock-free triggers installed by 0400. Ensure this test only runs against a database with the intended triggers.row-writes.integration.tsaccepts a lower bound that can miss a missing filter update;replaceTableRowsshould advance the version by exactlywrites.length + 1. Assert that exact expected version.row-writes.integration.tsusesd, which has not been inserted, so the upsert case only covers insertion and leaves conflict updates untested. Use an existing key with a changed value.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/sim/lib/table/rows/row-writes.integration.ts">
<violation number="1" location="apps/sim/lib/table/rows/row-writes.integration.ts:1388">
P2: `migrated` only detects the 0390 deferred UPDATE trigger, while the lock-free row-change triggers tested here are installed by 0400. On a 0390/0396 database, this block runs against the old definition-row triggers and times out under the holder instead of skipping; gate it on 0400's change-log trigger/table too.</violation>
<violation number="2" location="apps/sim/lib/table/rows/row-writes.integration.ts:1441">
P3: Use an existing key with a changed value here; `d` has not been inserted, so this tests only the insert side of the upsert and leaves the conflict-update path untested.</violation>
<violation number="3" location="apps/sim/lib/table/rows/row-writes.integration.ts:1521">
P3: Use an exact expected version here: `replaceTableRows` emits two statement-level entries (DELETE and INSERT), so the seven paths should advance by `writes.length + 1`. The current lower bound allows a missing filter-update zero-delta bump to pass.</violation>
</file>
<file name="packages/db/scripts/apply-dev-table-triggers.ts">
<violation number="1" location="packages/db/scripts/apply-dev-table-triggers.ts:73">
P1: This reconciliation can permanently undercount after racing with the fold: the UPDATE may use a pre-fold `actual` value after the fold has already deleted the log tail and committed its definition-row update. Reconcile under the same per-table lock/transaction, or re-read the count and tail after acquiring that lock.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Turn on auto-fix | Re-trigger cubic
| const reconciled = await sql` | ||
| UPDATE user_table_definitions d | ||
| SET row_count = actual.n | ||
| SET row_count = actual.n - actual.tail |
There was a problem hiding this comment.
P1: This reconciliation can permanently undercount after racing with the fold: the UPDATE may use a pre-fold actual value after the fold has already deleted the log tail and committed its definition-row update. Reconcile under the same per-table lock/transaction, or re-read the count and tail after acquiring that lock.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/db/scripts/apply-dev-table-triggers.ts, line 73:
<comment>This reconciliation can permanently undercount after racing with the fold: the UPDATE may use a pre-fold `actual` value after the fold has already deleted the log tail and committed its definition-row update. Reconcile under the same per-table lock/transaction, or re-read the count and tail after acquiring that lock.</comment>
<file context>
@@ -82,14 +70,15 @@ try {
const reconciled = await sql`
UPDATE user_table_definitions d
- SET row_count = actual.n
+ SET row_count = actual.n - actual.tail
FROM (
- SELECT d2.id, count(r.id)::int AS n
</file context>
| }) | ||
| }) | ||
|
|
||
| describe.skipIf(!migrated)('a held definition row', () => { |
There was a problem hiding this comment.
P2: migrated only detects the 0390 deferred UPDATE trigger, while the lock-free row-change triggers tested here are installed by 0400. On a 0390/0396 database, this block runs against the old definition-row triggers and times out under the holder instead of skipping; gate it on 0400's change-log trigger/table too.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At apps/sim/lib/table/rows/row-writes.integration.ts, line 1388:
<comment>`migrated` only detects the 0390 deferred UPDATE trigger, while the lock-free row-change triggers tested here are installed by 0400. On a 0390/0396 database, this block runs against the old definition-row triggers and times out under the holder instead of skipping; gate it on 0400's change-log trigger/table too.</comment>
<file context>
@@ -1382,6 +1385,143 @@ describe('table row writes against real PostgreSQL', () => {
})
})
+ describe.skipIf(!migrated)('a held definition row', () => {
+ it('lets every row write path commit while another session holds the definition row', async () => {
+ const table = await createTable([
</file context>
| FROM user_table_rows WHERE table_id = ${table.id}` | ||
| expect(count).toBe(1) | ||
| expect((await getTableById(table.id))?.rowCount).toBe(count) | ||
| expect(await rowsVersion(table.id)).toBeGreaterThanOrEqual(versionBefore + writes.length) |
There was a problem hiding this comment.
P3: Use an exact expected version here: replaceTableRows emits two statement-level entries (DELETE and INSERT), so the seven paths should advance by writes.length + 1. The current lower bound allows a missing filter-update zero-delta bump to pass.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At apps/sim/lib/table/rows/row-writes.integration.ts, line 1521:
<comment>Use an exact expected version here: `replaceTableRows` emits two statement-level entries (DELETE and INSERT), so the seven paths should advance by `writes.length + 1`. The current lower bound allows a missing filter-update zero-delta bump to pass.</comment>
<file context>
@@ -1382,6 +1385,143 @@ describe('table row writes against real PostgreSQL', () => {
+ FROM user_table_rows WHERE table_id = ${table.id}`
+ expect(count).toBe(1)
+ expect((await getTableById(table.id))?.rowCount).toBe(count)
+ expect(await rowsVersion(table.id)).toBeGreaterThanOrEqual(versionBefore + writes.length)
+ })
+ })
</file context>
| expect(await rowsVersion(table.id)).toBeGreaterThanOrEqual(versionBefore + writes.length) | |
| expect(await rowsVersion(table.id)).toBe(versionBefore + writes.length + 1) |
| { | ||
| tableId: table.id, | ||
| workspaceId, | ||
| data: { key: 'd', name: 'd' }, |
There was a problem hiding this comment.
P3: Use an existing key with a changed value here; d has not been inserted, so this tests only the insert side of the upsert and leaves the conflict-update path untested.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At apps/sim/lib/table/rows/row-writes.integration.ts, line 1441:
<comment>Use an existing key with a changed value here; `d` has not been inserted, so this tests only the insert side of the upsert and leaves the conflict-update path untested.</comment>
<file context>
@@ -1382,6 +1385,143 @@ describe('table row writes against real PostgreSQL', () => {
+ {
+ tableId: table.id,
+ workspaceId,
+ data: { key: 'd', name: 'd' },
+ conflictTarget: 'key',
+ secretProvenance: undefined,
</file context>
| data: { key: 'd', name: 'd' }, | |
| data: { key: 'a', name: 'a-updated' }, |
This branch was successfully deployed
1 participant
Summary
user_table_rowstriggers append touser_table_row_changesinstead of updating the table'suser_table_definitionsrow, so a row write no longer locks shared per-table state. A commit stalled on synchronous replication no longer makes every other writer to that table queue behind it and fail at the 3 slock_timeout+n/-nrow per table per statement — that row is both the row-count delta and the version bump, replacing the row-count triggers (0224) and the version insert/delete triggers (0240, dropped). The deferred update trigger (0390) keeps its column filter and per-transaction dedupe and logs a zero-delta rowapply-dev-table-triggers.tsinstalls the same functions and reconciles stored + tail against real counts; stale comments about the trigger lock updatedType of Change
Testing
row-writes.integration.tscase: a second session holds the definition rowFOR NO KEY UPDATE(what a commit stuck in SyncRep holds) while insert, batch insert, upsert, update by id, update by filter, delete by filter, and replace each run. All seven commit (56 ms total) and live count =COUNT(*). Against the pre-change triggers the first insert fails at the 3 s lock timeoutrows_versionand TTL helpers now read stored + tailbun run lint,check:audits,check:migrations origin/staging, block-registry check,docs-manifest:check, drizzle generate clean, 1,011 table unit tests; rootbun run testgreen except one desktop terminal test that passes in isolationChecklist
test-auditauthoring gate)🤖 Generated with Claude Code
https://claude.ai/code/session_01CJxC2fgucGVpT59RV5Z7xv