feat(repl): use bufferedMap for all subscriptions#823
Merged
Conversation
…erywhere # Conflicts: # pkg/repl/subscription_buffered.go # pkg/repl/subscription_buffered_test.go
Backfills four scenarios from the deleted subscription_queue_test.go that were not already covered by the queue-mode tests in subscription_buffered_test.go: - TestBufferedMapQueueFlushEmpty: Flush on an empty queue is a no-op. - TestBufferedMapQueueFlushBatchSizeLimit: targetBatchSize=2 with 5 same-type events forces flushQueueLocked to split a same-type segment across multiple applier calls; end state must still have all rows applied. - TestBufferedMapQueueFlushUnderLock: queue flush with a held TableLock, exercising the cutover path. - TestBufferedMapQueueConcurrentFlush: interleaves HasChanged from a goroutine with repeated Flush calls to catch races between queue append and queue drain. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds the new flag to docs/migrate.md (full description) and docs/move.md (short cross-reference). Both note the default is the safer FIFO-queue-full-time behaviour for non-memory-comparable PKs and that enabling the optimization is experimental and relies on the post-cutover checksum for correctness. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Removes the tableCompatibilityCheck gate that rejected source tables with VARCHAR / collation-sensitive primary keys. With deltaQueue gone and bufferedMap routing those subscriptions through its FIFO queue mode, the replication path is correct for non-memory-comparable PKs in moves too — the queue replays binlog events in order, and the target's collation-aware uniqueness produces the correct end state. Verified by TestMoveWithVarcharPK: a real move of a VARCHAR(64) PK table with utf8mb4_0900_ai_ci collation under 4 concurrent INSERT/UPDATE/DELETE writers, run twice — once with the default (queue full-time) and once with --force-enable-buffered-map (map during copy, queue post-copy). Both modes complete successfully and source/target row counts match. Also tidies a stylistic redundancy in pkg/repl/client.go. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Dead code left over from the deltaQueue removal — golangci-lint's unused linter caught it on CI. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Briefly explains that the codebase previously had separate deltaMap and deltaQueue subscription types, and that they were unified into a single bufferedMap implementation as part of the fix for issue block#746 (binlog vs. visibility race in the deltaMap's SELECT-at-flush path). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR unifies replication change tracking around a single subscription implementation (bufferedMap) for all tables, removing the old deltaQueue type and extending bufferedMap with an internal FIFO mode for non-memory-comparable primary keys. It also propagates a new --force-enable-buffered-map flag through migration/move flows, and updates watermark toggling to be context-aware and error-returning because toggling may now drain pending writes inline.
Changes:
- Remove
deltaQueueand route all subscriptions throughbufferedMap, adding an internal FIFO queue mode for non-memory-comparable PKs. - Change
SetWatermarkOptimizationtoSetWatermarkOptimization(ctx, enabled) erroracross repl/migration/move, since toggling may drain pending data via the applier. - Add and document
--force-enable-buffered-map(Migration/Move) and lift the move table compatibility gate that previously rejected non-memory-comparable PKs.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/repl/utils.go | Removes deltaQueue-specific SQL helper no longer needed. |
| pkg/repl/subscription.go | Updates subscription contract/docs for unified bufferedMap and new watermark toggle signature. |
| pkg/repl/subscription_queue.go | Deletes the old deltaQueue implementation. |
| pkg/repl/subscription_queue_test.go | Removes deltaQueue tests (coverage moved to buffered tests). |
| pkg/repl/subscription_buffered.go | Adds internal FIFO queue mode, routing rules, drain-on-toggle logic, and updated flush behavior. |
| pkg/repl/subscription_buffered_test.go | Expands tests to cover queue routing, FIFO semantics, batch splitting, under-lock flush, and transitions. |
| pkg/repl/README.md | Updates replication documentation to describe bufferedMap-only design and FIFO fallback. |
| pkg/repl/client.go | Adds ForceEnableBufferedMap config plumb-through; changes SetWatermarkOptimization to ctx+error and updates subscription selection logic. |
| pkg/repl/client_test.go | Updates tests for the new watermark toggle signature and removes deltaQueue-specific coverage. |
| pkg/move/runner.go | Passes ForceEnableBufferedMap into repl client and handles watermark toggle errors. |
| pkg/move/runner_test.go | Adds an end-to-end move test for VARCHAR PK tables under concurrent writes (both routing modes). |
| pkg/move/move.go | Adds --force-enable-buffered-map flag for move. |
| pkg/move/check/table_compatibility.go | Removes “memory-comparable PK” requirement; keeps “must have PK”. |
| pkg/move/check/table_compatibility_test.go | Updates tests to accept VARCHAR PKs for move compatibility checks. |
| pkg/migration/runner.go | Handles watermark toggle errors and passes ForceEnableBufferedMap into repl client. |
| pkg/migration/resume_test.go | Updates resume test for new watermark toggle signature. |
| pkg/migration/migration.go | Adds --force-enable-buffered-map flag for migrate with detailed rationale. |
| docs/move.md | Documents --force-enable-buffered-map for move. |
| docs/migrate.md | Documents --force-enable-buffered-map for migrate with experimental warning and rationale. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…yWithConcurrentWrites PR block#821 dropped the composite_unbuffered and composite_buffered subtests when deltaQueue was retired, since the unified bufferedMap implementation hadn't shipped yet. Restore them now: the composite (id, x_token VARCHAR) PK is non-memory-comparable, which routes the bufferedMap subscription through its FIFO queue mode for the entire run — exactly the path that needs cutover-atomicity coverage under concurrent writes. We deliberately skip a force-enable-buffered-map=true variant: with the default (false), the queue runs full-time across copy and post-copy phases, which exercises the queue path strictly more than the optimization. The default is the higher-coverage variant. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two threads: - pkg/repl/subscription.go: rewrite the Subscription doc comment to describe both routing policies for non-memory-comparable PKs (queue full-time vs. map-during-copy). The previous wording suggested the queue was only used "once watermark optimization is disabled", which is only true for forceEnableBufferedMap=true. - pkg/repl/client.go: snapshot the subscription set under c.Lock and release the lock before calling sub.SetWatermarkOptimization on each one. The drain inside the toggle can do synchronous applier writes, and holding c.Lock across that would block processRowsEvent for unrelated tables (relevant for moves with many subscriptions). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
aparajon
approved these changes
May 5, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Replaces the
deltaQueuesubscription type with a unifiedbufferedMapfor all replication subscriptions. The bufferedMap handles both memory-comparable and non-memory-comparable PKs:deltaQueueperformance characteristics and keeps the queue path hot in CI). Opt-in via--force-enable-buffered-mapto use map-during-copy / queue-post-copy.SetWatermarkOptimizationnow takes acontext.Contextand returns an error because toggling can drain the outgoing store inline.pkg/migration/runner.goandpkg/move/runner.gowere updated accordingly.The old
pkg/repl/subscription_queue.goand its tests are removed; their coverage is folded intosubscription_buffered_test.go.Fixes
Fixes #746 — every replication change now captures a full row image from the binary log via the bufferedMap path. The previous deltaMap/deltaQueue split could produce partial-image deltas for some PK shapes; with bufferedMap-everywhere, the row image is the source of truth on both the insert and delete sides of an LWW write.
Fixes #607 —
moveoperations now accept tables with non-memory-comparable primary keys (e.g.VARCHARwith a CI collation). ThetableCompatibilityCheckgate that rejected those tables has been removed, since the bufferedMap FIFO queue mode handles them correctly. Verified byTestMoveWithVarcharPK, which moves aVARCHAR(64)PK table withutf8mb4_0900_ai_cicollation under 4 concurrent INSERT/UPDATE/DELETE writers — once with the default (queue full-time) and once with--force-enable-buffered-map(map during copy, queue post-copy). Both modes complete successfully and source/target row counts match.Docs
docs/migrate.mdanddocs/move.mddocument the new--force-enable-buffered-mapflag, including the rationale (collation-sensitive PK ≠ map-key equality) and the experimental warning (correctness relies on the post-cutover checksum).Test plan
go build ./...passesgo vet ./...passesgo test ./pkg/repl/...passes (including merged-in upstream testsTestBufferedMapConcurrentHasChanged,TestBufferedMapKeyOverwriteDedupes,TestBufferedMapHasChangedNilAndEmpty,TestBufferedMapKeyAboveWatermarkCounters, plus the four portedTestBufferedMapQueueFlush*tests)go test ./pkg/move/...passes includingTestMoveWithVarcharPK(both queue-full-time and force-enable-buffered-map subtests)