Skip to content

improvement(tables): bump rows version once at commit and serialize same-value unique writes - #8335

Merged
waleedlatif1 merged 7 commits into
stagingfrom
improvement/table-rows-version-at-commit
Sep 28, 2026
Merged

waleedlatif1 merged 7 commits into
stagingfrom
improvement/table-rows-version-at-commit

Conversation

@waleedlatif1

@waleedlatif1 waleedlatif1 commented Sep 26, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

  • bump_user_table_rows_version ran as a statement-level trigger inside every row-update statement, so each writer locked its table's user_table_definitions row across the rest of its transaction (executions patch, provenance write and capture, synchronous commit), and concurrent writers to one table queued behind each other. Provenance-only updates bumped it too, even though they don't change the table's content
  • The UPDATE bump is now a row-level DEFERRABLE INITIALLY DEFERRED constraint trigger AFTER UPDATE OF data, order_key, with a WHEN on real changes. It bumps once per table per transaction at commit, deduped through one transaction-local setting. INSERT/DELETE statement bumps are unchanged. The only readers (the CSV snapshot cache and mount safety) read outside write transactions, so a commit-time bump keeps their read, scan, re-read checks intact
  • user_table_rows.table_id → user_table_definitions FK becomes DEFERRABLE INITIALLY DEFERRED (catalog-only). The write path updates a row twice in one transaction (data, then the provenance marker, which the rolling-deploy demote trigger requires to be separate), and the second update re-checked the FK, taking a key-share lock on the definition row that the commit-time bump then waited behind. ON DELETE CASCADE stays immediate, and nothing catches this FK's error inline
  • The trigger swap runs inside an explicit BEGIN, so it stays atomic when an earlier migration in the same deploy used a COMMIT breakpoint
  • CREATE STATISTICS (dependencies) ON workspace_id, table_id plus ANALYZE, so the planner stops underestimating per-table row counts and serves data @> filters from the tenant GIN
  • Unique columns are an application-level check, so two concurrent writes of the same value could both pass it and both commit. Every write path now takes transaction-scoped advisory locks (tag user_table_unique_value) before its check: the table's unique lock shared, then an exclusive lock per value, keyed on the same normalization the check uses (so '8' and 8 share a key). Writers of different values, on any column, never wait on each other
    • Each transaction takes all its locks at once, in one sorted order. The global order is schema lock, then unique locks, then row-order lock, then definition row
    • A json object or array value, or a transaction that would take more than 64 value locks, takes the table's unique lock exclusively instead. Whole-table replace and every import batch always do. A transaction holds at most 1 + 64 unique locks, however many unique columns the table has. 64 is the default per-connection budget of Postgres's shared lock table, so wide schemas and large batches can't exhaust it and fail unrelated queries
    • The unique check for updateRow, batchUpdateRows, and single-row filter updates now runs inside the write transaction, under the locks. batchUpdateRows also rejects two updates in one batch that set the same value, and it locks and checks only the unique columns each update changes
    • Tables without unique columns take no extra round trips

Type of Change

  • Improvement

Testing

  • A/B against staging on identical fresh DBs, real service functions, runs alternated:

    • 20 concurrent updateRow writers on one table: 943–1071 → 1585–1606 ops/s, p99 77–107 → ~18 ms, lock wait 521–525 → 4–8 backend-s
    • 50 writers: 886–937 → 1398–1450 ops/s, p99 274–292 → 124–159 ms
    • Provenance-only writes: 5185–5482 → 7979–8713 ops/s, p99 ~15 → ~4 ms
    • Inserts with a unique column and executions-only writes: within noise
    • Large single-transaction rewrites are ~20% faster, with ~0.6s of commit-time trigger work at 1M rows
  • Filter queries on 4,000 tables with skewed sizes: large-table data @> filters 1.7–5.7 → 0.5 ms

  • Migration-during-writes: 0 missed bumps across 3 runs (without the BEGIN, 40/40 missed)

  • row-writes.integration.ts: data and reorder edits bump once per transaction, including two tables in one transaction; provenance-only, timestamp-only, no-op, and executions-only writes don't bump; a second writer commits while the first is uncommitted; the snapshot re-keys when a writer commits mid-materialization. These fail on the old trigger

  • Fresh DB migrate plus replay, secret-provenance and table integration suites, table unit suites, type-check, lint, check:audits, and check:migrations origin/staging pass

  • Unique races (deterministic, driven by pg_locks wait states), all ending with one row:

    • same-value single inserts
    • '8' vs 8
    • batch vs single insert
    • updateRow vs insert
    • import batch vs insert
    • a batch over the value-lock cap vs a per-value writer
    • a replace adding the first unique column vs a writer on the older schema (no deadlock)
    • a 100-unique-column table holding a single lock
    • two updates in one batch setting the same value

    Different values don't wait on each other. Each race test fails on the pre-fix code

  • Unique-column throughput vs staging is within noise: distinct-value inserts, batch inserts, and batch upserts at 10 and 50 writers. updateRow keeps the gains above

  • Migration is 0390 on top of staging

Checklist

  • Code follows project style guidelines
  • Self-reviewed my changes
  • Tests added/updated and passing
  • No new warnings introduced
  • I confirm that I have read and agree to the terms outlined in the Contributor License Agreement (CLA)

@vercel

vercel Bot commented Sep 26, 2026 •

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated
docs Skipped Skipped Sep 28, 2026 4:04am UTC

Request Review

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 9 files

Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.

Fix all with cubic | Re-trigger cubic

Comment thread apps/sim/lib/table/rows/service.ts Outdated
@greptile-apps

greptile-apps Bot commented Sep 26, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 5/5

[Critical risk] Restructures row-write locking and adds database migration.

The PR appears safe to merge; no new actionable issue or outstanding previous finding remains.

Summary

The PR defers row-update version bumps until commit and adds transaction-scoped locks around application-level unique checks.

  • UPDATEs that change row data or order now bump each table’s version once per transaction; provenance-only writes do not.
  • Unique-value writes serialize before checking for conflicts, with a table-wide lock for large batches and imports.
  • The latest changes canonicalize JSON object keys for batch duplicate detection and release a test writer when its lock-wait assertion fails.
Diagram
%%{init: {'theme': 'neutral'}}%%
flowchart LR
  A[Begin row-write transaction] --> B[Acquire unique-value or table-wide lock]
  B --> C[Check uniqueness]
  C --> D[Acquire row-order lock if needed]
  D --> E[Write rows]
  E --> F[Commit]
  F --> G[Deferred UPDATE trigger bumps rows_version once per table]
Loading

Reviews (10) · Last reviewed commit: "fix(tables): key unique JSON values by c..."

Comment thread apps/sim/lib/table/rows/row-writes.integration.ts Outdated
Comment thread apps/sim/lib/table/rows/service.ts Outdated
@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cubic-dev-ai review this PR

@cubic-dev-ai

cubic-dev-ai Bot commented Sep 26, 2026

Copy link
Copy Markdown
Contributor

@cubic-dev-ai review this PR

@waleedlatif1 I have started the AI code review. It will take a few minutes to complete.

Comment thread apps/sim/lib/table/rows/row-writes.integration.ts
Comment thread apps/sim/lib/table/rows/row-writes.integration.ts Outdated

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 9 files

Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.

Fix all with cubic | Re-trigger cubic

Comment thread apps/sim/lib/table/rows/row-writes.integration.ts Outdated
@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cubic-dev-ai review this PR

@cubic-dev-ai

cubic-dev-ai Bot commented Sep 26, 2026

Copy link
Copy Markdown
Contributor

@cubic-dev-ai review this PR

@waleedlatif1 I have started the AI code review. It will take a few minutes to complete.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 9 files

Confidence score: 5/5

  • Automated review surfaced no issues in the provided summaries.
  • No files require special attention.

Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.

Re-trigger cubic

Comment thread apps/sim/lib/table/rows/row-writes.integration.ts Outdated
@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

1 similar comment
@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cubic-dev-ai review this PR

@cubic-dev-ai

cubic-dev-ai Bot commented Sep 26, 2026

Copy link
Copy Markdown
Contributor

@cubic-dev-ai review this PR

@waleedlatif1 I have started the AI code review. It will take a few minutes to complete.

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 8 files

Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.

Fix all with cubic | Re-trigger cubic

Comment thread apps/sim/lib/table/rows/row-writes.integration.ts Outdated
Comment thread apps/sim/lib/table/rows/row-writes.integration.ts
@greptile-apps

This comment has been minimized.

@waleedlatif1
waleedlatif1 force-pushed the improvement/table-rows-version-at-commit branch from dba8085 to ecb119e Compare September 26, 2026 23:29
@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cubic-dev-ai review this PR

@cubic-dev-ai

cubic-dev-ai Bot commented Sep 26, 2026

Copy link
Copy Markdown
Contributor

@cubic-dev-ai review this PR

@waleedlatif1 I have started the AI code review. It will take a few minutes to complete.

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile re the concurrent-unique-insert risk in the summary: this PR doesn't change unique-constraint behavior. insertRow, batch insert, and upsert's insert branch are byte-identical to staging. An earlier revision moved the unique check under the per-table append lock, which closed the race but serialized every insert into a table with a unique column (2.5–10× slower under concurrency). That revision was reverted, so this PR doesn't touch the race either way. The race exists on staging today; fixing it properly needs per-value locks (table, column, normalized value), and that goes in a separate PR with its own benchmark. This PR only changes the rows_version trigger, the row FK's deferrability, and the planner statistic.

@greptile-apps

greptile-apps Bot commented Sep 27, 2026

Copy link
Copy Markdown
Contributor

You’re right: the concurrent unique-insert race is not introduced or changed by this PR. The changed application code contains no insert, batch-insert, or upsert-branch edits, and the migration only changes the rows-version triggers, FK deferrability, and planner statistics.

I’m withdrawing that critical-risk finding from this review; it should be addressed separately with the per-value locking design and its own concurrency benchmarks.

…ck unique columns on every import batch

Lower the per-transaction value-lock cap to 64 so concurrent writers cannot
exhaust the shared lock table; larger writes keep the exclusive column-lock
fallback. Import batches now take the unique-column locks and run their unique
check inside their transaction, batchUpdateRows rejects two updates writing the
same unique value and locks and checks only the unique columns each update
changes, and the 0390 comment describes the mechanism only.
@waleedlatif1
waleedlatif1 force-pushed the improvement/table-rows-version-at-commit branch from 1d520f6 to 97cb317 Compare September 28, 2026 03:34
@waleedlatif1 waleedlatif1 changed the title improvement(tables): bump a table's rows version once at commit improvement(tables): bump rows version once at commit and serialize same-value unique writes Sep 28, 2026
@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cubic-dev-ai review this PR

@cubic-dev-ai

cubic-dev-ai Bot commented Sep 28, 2026

Copy link
Copy Markdown
Contributor

@cubic-dev-ai review this PR

@waleedlatif1 I have started the AI code review. It will take a few minutes to complete.

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 13 files

You've manually re-run cubic several times on this PR. Each manual re-review checks the full PR again and counts toward your usage quota. To preserve your usage limits, we recommend letting cubic automatically review new commits.
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.

Fix all with cubic | Re-trigger cubic

Comment thread apps/sim/lib/table/rows/row-writes.integration.ts
Comment thread apps/sim/lib/table/rows/unique-locks.ts Outdated
Comment thread packages/db/migrations/0390_table_rows_version_at_commit.sql
@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cubic-dev-ai review this PR

@cubic-dev-ai

cubic-dev-ai Bot commented Sep 28, 2026

Copy link
Copy Markdown
Contributor

@cubic-dev-ai review this PR

@waleedlatif1 I have started the AI code review. It will take a few minutes to complete.

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 13 files

You've manually re-run cubic several times on this PR. Each manual re-review checks the full PR again and counts toward your usage quota. To preserve your usage limits, we recommend letting cubic automatically review new commits.
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.

Fix all with cubic | Re-trigger cubic

Comment thread apps/sim/lib/table/rows/service.ts
Comment thread apps/sim/lib/table/rows/row-writes.integration.ts Outdated
@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cubic-dev-ai review this PR

@cubic-dev-ai

cubic-dev-ai Bot commented Sep 28, 2026

Copy link
Copy Markdown
Contributor

@cubic-dev-ai review this PR

@waleedlatif1 I have started the AI code review. It will take a few minutes to complete.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 13 files

Confidence score: 5/5

  • Automated review surfaced no issues in the provided summaries.
  • No files require special attention.

You've manually re-run cubic several times on this PR. Each manual re-review checks the full PR again and counts toward your usage quota. To preserve your usage limits, we recommend letting cubic automatically review new commits.
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.

Re-trigger cubic

@waleedlatif1
waleedlatif1 merged commit 1321ee9 into staging Sep 28, 2026
32 checks passed
@waleedlatif1
waleedlatif1 deleted the improvement/table-rows-version-at-commit branch September 28, 2026 17:27

This branch was previously deployed

1 inactive deployment
Preview — 1c00cf55 Deployed Sep 28, 2026 by vercel[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant