Repository navigation
Conversation
Rewrites RFC-005 to fix the design flaws a max-effort review found in revision 1: - storage=innodb checkpoints run on a timer at transaction boundaries, never at binlog rotation (rotation-triggered checkpoints that write to the binlog looped forever) - Phase 1 is standalone: index writes use sql_log_bin=0, so replicas keep their own rows and nothing collides; replicating index rows is opt-in in Phase 2b - GTID mode is required; resume, restore and promotion use GTID sets, never binlog file names compared across servers - writer_uuid compare-and-set on every write, GR primary claims automatically, async promotion uses MYVECTOR_INDEX_TAKEOVER - followers poll index_meta, check chunk counts, require commit order and refuse filtered myvector tables - tombstones keep generations rising, binary 776-byte index names, DATETIME(6), structural validation of stored data, crash-safe migration procedure, short-lived command-service sessions - corrected write-volume numbers (vector updates) and a sizing section - Phase 0 prerequisite: #265 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0137WC8aYwwneCuPXq4i7yTq
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 52 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. CodeRabbit Verify
|
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4a93b7c29c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| **version `s`**, then commits `index_meta.ckpt_seq = s` last: | ||
| **Copy, then write.** The checkpoint holds the online-update queue paused (as today, | ||
| §3) only while it copies the dirty chunks and the header into a buffer, which is a | ||
| memory copy. Then it lets updates continue and writes from the buffer. Online updates |
There was a problem hiding this comment.
When online updates resume after the buffer copy but before the SQL writes finish, they can add changes to the same dirty sets. The spec clears those sets only after publication (§4.4, line 382), so those post-copy changes can be cleared without being persisted; a later checkpoint may then advance ckpt_gtids past them, losing them after a restart. Keep post-copy dirtiness separate and publish the GTID boundary corresponding to the buffered snapshot.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid, fixed in fbc66e9. §4.4 Copy, then write now does three things while the queue is paused at a transaction boundary: copies the dirty chunks and header, records the applied GTID set at that instant as ckpt_gtids, and swaps the dirty sets (takes the current ones, starts empty ones). Updates during the SQL writes land in the new sets for the next checkpoint. The published ckpt_gtids therefore matches the published chunks exactly. A failed checkpoint merges its taken sets back. Test 9 now covers both cases.
| operator runs `CALL mysql.MYVECTOR_INDEX_TAKEOVER();` on the new primary. It sets | ||
| `writer_uuid` to this server for every index and switches the server to writer. | ||
| There is no automatic takeover: two servers that both believe they were promoted | ||
| must not both claim. |
There was a problem hiding this comment.
If an async primary remains running but becomes unreachable during failover, takeover changes writer_uuid only on the promoted replica until replication conveys that change. The old primary still has its own UUID in its local metadata, so both servers can pass the per-server compare-and-set and checkpoint divergent indexes. Require external fencing of the old primary before takeover, or use a mechanism that actually prevents both copies from authorizing writes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid, addressed in fbc66e9. With async replication there's no consensus, so nothing inside the database can stop a partitioned old primary from passing its own local compare-and-set; the same partition lets it accept user writes too. §4.10 now requires the operator to fence the old primary before takeover (super_read_only, stop it, or isolate it), as async failover already needs, and the procedure's docs and output say so. It also says what happens if an unfenced primary rejoins: the replicated takeover changes its writer_uuid and it becomes a follower, and its divergent checkpoints are errant transactions. Group Replication avoids this through certification. Test 20 now fences first.
| - Nothing about the index enters the binlog, so a primary's rows never reach a | ||
| replica, and nothing can collide or stop a replication applier. | ||
| - A backup taken on any server includes that server's index. | ||
| - `writer_uuid` is always the local `server_uuid`. |
There was a problem hiding this comment.
When a standalone backup or copied datadir is loaded under a new server_uuid (a scenario explicitly supported in §4.7), its persisted writer_uuid still names the source server. Checkpoints require writer_uuid = @@server_uuid (§4.4), so they will fail their compare-and-set and stop advancing the durable index. Rebind the UUID during standalone restore or startup before allowing writes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid, fixed in fbc66e9. In standalone mode, a server that loads an index whose writer_uuid isn't its own (after a restore, copied datadir or clone) rebinds it to @@server_uuid with sql_log_bin = 0 and logs it (§4.8, referenced from §4.7). Replicated mode never rebinds automatically, since that's what takeover is for. Test 3 now checks that checkpoints continue after a copied-datadir start.
…ore takeover, standalone writer rebind Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0137WC8aYwwneCuPXq4i7yTq
|
@coderabbitai review |
Action not completed
|
…dex checkpoints, mixed-backend stream question Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0137WC8aYwwneCuPXq4i7yTq
|
@coderabbitai full review |
Action not completed
|
|
@coderabbitai full review |
Action not completed
|
|
@coderabbitai full review |
Action not completed
|
Summary
Rewrites RFC-005 (
docs/RFC-005-INNODB-INDEX-STORAGE.md, merged as revision 1 in #218) after a max-effort review found design flaws in it. Design decisions confirmed by the maintainer before writing:storage=innodbindex.writer_uuidcompare-and-set, with an explicit takeover after async promotion.The RFC's new §11 maps each problem to its fix. In short:
storage=innodb; standalone writes usesql_log_bin=0read_onlywriter_uuidcompare-and-set on every write; GR claims automatically;MYVECTOR_INDEX_TAKEOVERfor asyncckpt_gtidsTIMESTAMPDATETIME(6)binlog_row_image=MINIMALsuggested (breaks online updates)save(dirty nodes only)MYVECTOR_INDEX_MIGRATE, real full save, crash-rule tableCREATE ... IF NOT EXISTSundersql_log_bin=0Also new:
Notes for reviewers
mainat 7a42512.replica_preserve_commit_order(ONsince 8.0.27)MYSQL_RPL_GTIDsupport inmysql_binlog_open()🤖 Generated with Claude Code
https://claude.ai/code/session_0137WC8aYwwneCuPXq4i7yTq