Skip to content

Fix #4591: serialize concurrent DCB tag-boundary appends via side table#4595

Merged
jeremydmiller merged 2 commits into
masterfrom
fix/4591-dcb-tag-version-side-table
Jun 1, 2026
Merged

Fix #4591: serialize concurrent DCB tag-boundary appends via side table#4595
jeremydmiller merged 2 commits into
masterfrom
fix/4591-dcb-tag-version-side-table

Conversation

@jeremydmiller

@jeremydmiller jeremydmiller commented Jun 1, 2026

Copy link
Copy Markdown
Member

Closes #4591.

⚠️ Required schema migration in 9.4

This PR adds a new schema object: mt_dcb_tag_version in your event-store schema, created automatically whenever any DCB tag types are registered. The new fetch and save paths reference it, so users on AutoCreate.None must run db-patch / db-apply before deploying 9.4 or saves with tagged events will fail with relation "<schema>.mt_dcb_tag_version" does not exist. Auto-apply users (default) get the table on first save, no action needed. Full migration write-up at docs/migration-guide.md → 9.4.0 section on this branch.

This lands as a required migration in a point release because it's a correctness fix — the racy SELECT-then-INSERT pattern affected both DcbStorageMode.HStore and DcbStorageMode.TagTables (the predicate shape differed; the race didn't). Per-save cost: one extra INSERT … ON CONFLICT DO UPDATE per distinct (tag_type, tag_value) tuple referenced by the batch — typically one or two rows per tagged save.

The race

AssertDcbConsistency emitted a SELECT EXISTS(... FROM mt_events ... WHERE seq_id > :lastSeen AND <tag predicate>) as a separate non-locking statement before the INSERTs at READ COMMITTED. Two truly-concurrent fetch→save sessions both ran the EXISTS check before either committed, both saw no conflict, and both committed when only one should have. The reproduction in #4594 routinely observed 6 of 8 racers commit when the contract demands exactly one.

The constraint Jeremy set was no SERIALIZABLE, no advisory locks.

The fix — mt_dcb_tag_version side table

A new side table keyed by (tag_table, tag_value, tenant_id) whose version column converts the predicate read into a row-level write conflict.

  • Producer-side bump. Every tagged event append (boundary or not) queues a DcbTagVersionBumpOperation that inserts the row at version=1 first time or ON CONFLICT DO UPDATE SET version = mt_dcb_tag_version.version + 1 every subsequent time. This is what makes the side table reflect every commit, not only boundary saves — a plain session.Events.Append(streamId, taggedEvent) correctly invalidates any in-flight boundary that captured the prior version.
  • Capture at fetch. FetchForWritingByTags adds a second batched SELECT that reads each tag's current version (or 0 if no row yet) and stamps it onto the returned EventBoundary via DcbTagVersionAssertion.
  • Optimistic check at save. The assertion emits INSERT … ON CONFLICT DO UPDATE SET version = version + 1 WHERE version = $captured RETURNING 1 per captured tag, sorted by (tag_table, tag_value) for deterministic lock-acquisition order. The row-level write lock plus the captured-version predicate is the serialization point: any save with a stale captured value matches zero rows on RETURNING and surfaces DcbConcurrencyException.

Works on READ COMMITTED for both DcbStorageMode.HStore and DcbStorageMode.TagTables. The AssertDcbConsistency class is gone. EventStore.Dcb.FetchForWritingByTags is now a thin delegate over FetchForWritingByTagsHandler so the two entry points (direct and BatchedQuery) can't drift on the boundary semantics.

Regression test

Bug_4591_truly_concurrent_dcb_tag_appends_both_commit is now a [Theory] over DcbStorageMode (HStore + TagTables). 8 racers, each fetches via its own session, barrier-syncs on a TaskCompletionSource so every fetch captures the same boundary, then races into SaveChangesAsync. Asserts exactly one commit + Racers - 1 DcbConcurrencyException throws. Both variants pass.

Follow-ups (tracked separately, not in this PR)

  • Test location. Per Jeremy's note, the regression test should move to its own project alongside DaemonTests so it doesn't bloat the EventSourcingTests run (it spins concurrent sessions). Left in EventSourcingTests for this PR.
  • Heap fillfactor. DcbTagVersionTable does not yet set WITH (fillfactor = N) on the heap — Weasel.Postgresql's Table type doesn't currently expose that. Tracked as a Weasel follow-up; the column list leaves version unindexed so HOT updates remain eligible regardless.
  • Cleanup. The side table is never truncated. Growth is bounded by the count of distinct (tag_table, tag_value, tenant_id) tuples — docs/events/dcb.md documents the long-lived-tag guidance.

🤖 Generated with Claude Code

jeremydmiller and others added 2 commits May 31, 2026 21:56
Pre-fix, `AssertDcbConsistency` emitted a `SELECT EXISTS(... FROM mt_events
... WHERE seq_id > :lastSeen AND <tag predicate>)` as a separate
non-locking statement before the INSERTs at READ COMMITTED. Two
truly-concurrent fetch→save sessions both ran the EXISTS check before
either committed, both saw no conflict, and both committed when only one
should have. The reproduction in PR #4594 routinely observed 6 of 8
racers commit when the contract demands exactly one.

The fix is per the prior research-write-up: a new `mt_dcb_tag_version`
side table keyed by `(tag_table, tag_value, tenant_id)` whose `version`
column converts the predicate read into a row-level write conflict.

How it serializes (READ COMMITTED, no SERIALIZABLE, no advisory locks):
- Every tagged event append (boundary or not) queues a
  `DcbTagVersionBumpOperation` that INSERTs the row at version=1 (first
  time) or `ON CONFLICT DO UPDATE SET version = mt_dcb_tag_version.version
  + 1` (every subsequent time). This is the *producer* side — it makes the
  side table reflect every commit, not only boundary saves.
- `FetchForWritingByTags` adds a second batched SELECT that captures each
  tag's current `version` (or 0 if no row yet) and stamps it onto the
  returned `EventBoundary` via `DcbTagVersionAssertion`.
- At `SaveChangesAsync` time, the assertion emits
  `INSERT … ON CONFLICT DO UPDATE SET version = version + 1
  WHERE version = $captured RETURNING 1` per captured tag, sorted by
  `(tag_table, tag_value)` for deterministic lock-acquisition order. The
  row-level write lock plus the captured-version predicate is the
  serialization point: any save with a stale captured value matches zero
  rows on RETURNING and surfaces `DcbConcurrencyException`.

The `AssertDcbConsistency` class is gone — the side-table mechanism
replaces it for both `DcbStorageMode.HStore` and `DcbStorageMode.TagTables`.
`EventStore.Dcb.FetchForWritingByTags` is now a thin delegate over
`FetchForWritingByTagsHandler` so the two entry points (direct and
`BatchedQuery`) can't drift on the boundary semantics.

Regression test:
- `Bug_4591_truly_concurrent_dcb_tag_appends_both_commit` is now a
  `[Theory]` over `DcbStorageMode` (HStore + TagTables). 8 racers, each
  fetches via its own session, barrier-syncs on a TCS so every fetch
  captures the same boundary, then races into SaveChangesAsync. Asserts
  exactly one commit + `Racers - 1` `DcbConcurrencyException` throws.
  Both variants pass.

Caveats / known follow-ups (separate task):
- Test currently lives in `EventSourcingTests`; it's slow (~100 ms per
  Theory case, runs concurrent sessions) and per the user's note should
  move to its own project alongside `DaemonTests` in a follow-up so it
  doesn't bloat the EventSourcingTests run.
- `DcbTagVersionTable` does not yet set heap `fillfactor` (Weasel.Postgresql
  doesn't expose `WITH (fillfactor = N)` on the `Table` type). Tracked as
  a Weasel follow-up; column-list keeps `version` unindexed so HOT
  updates remain eligible regardless.
- The side table is never truncated. Growth is bounded by the count of
  distinct (tag_table, tag_value, tenant_id) tuples — see
  docs/events/dcb.md for the long-lived-tag guidance.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Per Jeremy's review: the side-table fix lands as a required, mandatory
schema migration in a point release, so the migration callout needs to
be impossible to miss for users on AutoCreate.None / db-patch pipelines.

- migration-guide.md: new "Key Changes in 9.4.0" section at the top with
  an action-required Badge, the exact db-patch / db-apply commands, and
  a section explaining why the fix lands in 9.4 rather than waiting for
  10.0 (it's a correctness bug affecting both DcbStorageMode.HStore and
  DcbStorageMode.TagTables — the racy SELECT-then-INSERT pattern was
  identical in both branches of the old AssertDcbConsistency).
- docs/events/dcb.md: warning callout at the head of the "How the
  boundary check serializes" section pointing back at the migration
  guide. Also corrected the SQL shape to match what shipped
  (INSERT … ON CONFLICT DO UPDATE … WHERE … RETURNING 1, not a bare
  UPDATE) and added the producer-bump paragraph that explains why
  non-boundary appends now also bump the row.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@jeremydmiller jeremydmiller merged commit 9eac963 into master Jun 1, 2026
8 checks passed
@jeremydmiller jeremydmiller deleted the fix/4591-dcb-tag-version-side-table branch June 1, 2026 10:48
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.

DCB: AssertDcbConsistency is a non-locking check — truly-concurrent appends sharing a tag both commit (no DcbConcurrencyException)

1 participant