fix(snapshot): preserve data when restoring pre-subject_id snapshots - #248
fix(snapshot): preserve data when restoring pre-subject_id snapshots#248loveRhythm1990 wants to merge 7 commits into
Conversation
…nches Materialize historical rows with explicit column mappings and current defaults before transactional replacement. Reconcile subject_id on snapshot branches before registration. Add legacy-schema, failure-atomicity and MCP regressions verified on MatrixOne 3.0.11 and 3.0.15. Fixes matrixorigin#246
Post-rebase local validation (
|
| Suite | Result |
|---|---|
memoria-git --lib --include-ignored |
4 passed (includes DB rollback failure injection) |
memoria-git --test legacy_snapshot |
9 passed |
memoria-git --example restore_cleanup --include-ignored |
3 passed (includes DB deletion guardrails) |
memoria-mcp --test branch_e2e |
49 passed |
memoria-mcp --test snapshot_e2e |
21 passed |
memoria-storage --test branch_ops |
10 passed |
memoria-storage --test subject_id_mo_compat |
10 passed |
| Focused shared-schema and ASCII-branch-name unit tests | 2 passed |
Total: 101 DB test results + 7 database-free tests, zero failures in these
commands. Also passed Clippy with -D warnings for the git/storage/MCP/API
libraries and git diff --check.
Remote CI is still running; this comment does not claim CI completion or a
production upgrade approval. The separate 3.0.11 -> 4.2.1-d2393868a rehearsal
remains blocked by MatrixOne's incomplete catalog migration, as documented in
the PR body. MatrixOne source and the existing local service stack are unchanged.
|
Fixed the DB-test failure in appended commits Root cause
Changes
ValidationThe previously failing Unicode-branch test and all three cleanup-example tests pass on each of:
On that exact CI image, the broader regression run also passed: 49 branch E2E tests, 21 snapshot E2E tests, 9 legacy-snapshot tests, 4 library tests (including the ignored transaction-failure regression), and 3 cleanup-example tests. The broader run used the first fixture-fix revision; after splitting the DDL requests for 3.0.11, both affected test targets were rerun successfully on all four versions above. Formatting and the focused Clippy check pass. The new remote run for |
|
Findings on
|
gouhongshen
left a comment
There was a problem hiding this comment.
Findings on b8a2761:
-
[P1] Default merge drops
subject_idandauthor_id.memory_mergemaps the defaultacceptstrategy to thereplaceSQL path, but theINSERT INTO mem_memoriescolumn list and itsSELECTomit both fields (memoria/crates/memoria-mcp/src/git_tools.rs:1153-1229). A historical branch is explicitly migrated to includesubject_id, and the new regression writes a non-nullsubject_id, but no test merges that row. I reproduced this on MatrixOne 4.1.2: a branch row withsubject_id='subject-1'andauthor_id='author-1'is inserted into main with both valuesNULL. Add the fields to both lists and cover from-snapshot write → merge → read. -
[P1] Presence-only schema validation can register a branch whose native diff is corrupt.
ensure_branch_subject_idonly compares column names and appends a missingsubject_id(memoria/crates/memoria-storage/src/store.rs:2017-2047), thenmemory_branchregisters the clone (memoria/crates/memoria-mcp/src/git_tools.rs:1078-1096). MatrixOneDATA BRANCH DIFFis ordinal/schema-sensitive. In a disposable MatrixOne 4.1.2 database, the live table hadsubject_idbetweenuser_idandmemory_type, while the historical clone got it appended;DATA BRANCH DIFFdid not reject the clone and returned the live row withmemory_type=NULL,content=semantic, andsubject_id=old. That makesmemory_diff/merge unsafe for a registered historical branch. Compare the full column order/types or reject the clone before registration, and add a legacy-clone diff/merge regression. -
[P2] Unicode snapshot names are accepted but sent as unquoted SQL identifiers. The new shared validator accepts Unicode (
memoria/crates/memoria-core/src/lib.rs:10-14), andsnap_internalpreserves Unicode names (memoria/crates/memoria-mcp/src/git_tools.rs:162-173), butcreate_snapshotemitsCREATE SNAPSHOT {safe}without quoting (memoria/crates/memoria-git/src/service.rs:669-675). On the local MatrixOne 4.1.2 instance,CREATE SNAPSHOT mem_snap_1_review_实验 ...fails with SQL parser error 1064 near实验. Thereforememory_snapshotwith a Chinese name still fails; quote the physical snapshot identifier or make it ASCII while retaining the display name. The added Unicode coverage tests branch names only, not snapshot creation.
aptend
left a comment
There was a problem hiding this comment.
Additional blocker found on b8a2761:
[P1] Preserve subject_id in every memory_apply copy path. ensure_branch_subject_id makes scoped writes valid on historical branches, but every selective_apply branch-to-main INSERT ... SELECT copies author_id while omitting subject_id (memoria/crates/memoria-git/src/service.rs:1273-1281, 1308-1316, 1370-1394, 1470-1478, 1561-1569). Consequently, applying an add, restore, update, remove, or accepted conflict sets the target row’s subject_id to its default NULL, so the memory disappears from subject-scoped reads and can surface in unscoped reads. Add subject_id to both sides of all these column lists and cover historical branch → scoped write → memory_apply → scoped main read.
The three blockers in the earlier changes-request review also remain unresolved on this head.
Address PR matrixorigin#248 reviews: preserve subject and author metadata through merge and all apply paths, isolate similarity conflicts by subject, quote Unicode snapshot identifiers, and validate native branch schemas across migration and operations. Add ordinary/legacy branch scope regressions and schema/Unicode guards. Validated 93 focused and regression tests on MatrixOne 4.2.1 plus strict Clippy checks.
gouhongshen
left a comment
There was a problem hiding this comment.
Findings on 3fdf3eef:
- [P1] Do not ALTER a native branch before using native branch operations.
migrate_branch_subject_idaddssubject_idto the result ofDATA BRANCH CREATE TABLE ... FROM SNAPSHOTwithALTER TABLE(memoria/crates/memoria-storage/src/store.rs:2026-2037), and may add the subject index with anotherALTER TABLE(memoria/crates/memoria-storage/src/store.rs:2065-2075). On MatrixOne 4.1.2 this rewrites the object into a physical copy and drops its native branch lineage:mo_catalog.mo_tables.rel_createsqlcontains a..._copy_<uuid>table, andDATA BRANCH DELETEthen returns 20101,is not an active branch table. The same happens for an existing Unicode branch migrated at startup. A historical clone is registered after this mutation (memoria/crates/memoria-mcp/src/git_tools.rs:1078-1096), so subsequent native diff/merge/pick/delete operations are unusable; when schema validation rejects the clone, the cleanup call uses the same native delete and leaves the unregistered clone behind. I reproduced this with the new tests:branch_scope_e2efailed 3/6,legacy_snapshotfailed 1/11, andbranch_e2efailed 2/49 on MatrixOne 4.1.2. Preserve native lineage through migration, or keep the migrated object out of native operations and provide a deliberate cleanup/operation path that matches its actual table type. Add a regression on the supported 4.x build.
aptend
left a comment
There was a problem hiding this comment.
Blocker on 3fdf3eef:
[P1] Do not destroy native branch lineage while migrating historical branches. The historical-branch path creates a native branch, then migrate_branch_subject_id runs ALTER TABLE to add/reposition subject_id and may run another ALTER TABLE for the index (memoria/crates/memoria-storage/src/store.rs:2026-2075). On MatrixOne 4.1.2, ALTER rewrites that native branch into a physical-copy table, after which DATA BRANCH DIFF, merge, pick, and delete no longer recognize it as an active branch. This also means cleanup after validation failure can leave an unregistered clone behind because it calls native branch delete.
Preserve native lineage, or stop routing the migrated object through native branch operations and give it a matching cleanup/operation path. Add a supported-4.x regression covering historical clone → migration → diff/merge/delete.
gouhongshen
left a comment
There was a problem hiding this comment.
Findings on 0015e31:
- [P1] Migrate
author_idon Unicode legacy branch tables. The PR now accepts Unicode physical names inmigrate_branch_subject_idand documents that existing Unicode physical names remain valid, but theauthor_idcompatibility loop still skips every non-ASCII table name (memoria/crates/memoria-storage/src/store.rs:1844-1851). For a pre-author_idnative branch such asbr_1234_实验, migration addsauthor_idtomem_memoriesand silently leaves the branch without it. The next schema check reports the branch missingauthor_id, and branch read/write SQL that projectsauthor_idalso fails, so the registered branch becomes unusable. Remove the ASCII-only guard and use the shared identifier validator/quotedself.t(bt_raw), then add a regression for a Unicode physical branch missingauthor_idthat runs migration and a branch read/write or native operation.
What type of PR is this?
Which issue(s) this PR fixes
Fixes #246
What this PR does / why we need it
Restoring a snapshot created before
subject_idwas introduced previously ran anautocommitted DELETE followed by
INSERT ... SELECT *. The INSERT could fail onthe historical column count after live rows had already been deleted. Historical
snapshot branches also inherited an outdated schema.
This is a separate snapshot-safety fix, based on current
mainafter #244. It doesnot reintroduce that PR's scoped-key changes or modify MatrixOne source/deployment.
Changes
reject missing required values before touching live data.
replace live rows with DELETE + INSERT in one transaction. Preserve the original
statement error even if rollback fails. Do not retry the non-idempotent stage INSERT.
subject_idmigration. Require all current memory-columnnames before registering a historical clone, while treating index creation as
best-effort. Reject and clean up unusable clones.
retain quoted access/migration/cleanup for existing Unicode physical names.
explicit all-writers-stopped requirements. Protect every registered branch and
fail closed if the registry cannot be read; no online prefix-based reaper.
schema gaps, Unicode names, permissions, and cleanup guardrails. Explicitly run
ignored DB failure-injection tests and example tests in both CI test workflows.
Validation
6a0394c) and 3.0.15 (43e871c):9 legacy snapshot tests, 1 transaction failure test, 1 DB cleanup test, 49 MCP
branch tests, 21 MCP snapshot tests, and 20 storage compatibility tests per build.
the follow-up validation comment. These are regression baselines, not production
upgrade approval. Remote CI results must be checked separately.
cargo clippy -p memoria-git -p memoria-storage -p memoria-mcp -p memoria-api --lib -- -D warningsand
git diff --checkpass.Production upgrade status — not yet approved
The intended production target is MatrixOne
v4.2.1-d2393868a-2026-08-28, not 3.0.15. A separate 3.0.11-to-target rehearsalusing unchanged Memoria
apiserver-20260511-27107f5-df643526hit an engine catalogmigration failure:
no such table mo_catalog.mo_feature_registry. The internalupgrade remained incomplete and
SHOW SNAPSHOTSfailed withcolumn kind does not exist. The target accepted SQL connections, so connectivityand API liveness alone are not sufficient upgrade readiness checks.
This PR does not fix that MatrixOne upgrade failure, and its new Memoria DB
suites have not been validated on the target build. Resolve/rehearse the supported
MatrixOne upgrade path first, validate old Memoria, then validate/deploy new Memoria.
Review focus / boundaries
snapshot operations and quiesce competing writes.
whole-table transaction need capacity planning; 512 rows is not a scale limit.
tool, not proof that an unregistered table is safe to delete while writers run.
Branch diff schema compatibility on every MatrixOne version.
See the compatibility and verification notes
for the regression commands, operational constraints, and exact upgrade evidence.