[improvement](be) Separate local tablet schemas from metadata - #67088
[improvement](be) Separate local tablet schemas from metadata#67088zyp-V wants to merge 1 commit into
Conversation
### Release note Reduce local metadata size by storing reusable tablet and rowset schemas separately. Keep variant rowset schemas inline. ### Check List (For Author) - Test: Unit Test coverage updated but not run at user request; targeted clang-format 16, git diff checks, and BE build-hygiene checks passed - Behavior changed: Yes, local tablet and rowset schema persistence and loading now use separated metadata - Does this need documentation: No
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Requesting changes: the schema-separation migration is not safe across publication, restart, downgrade, clone failure, cleanup, and index-rewrite paths. I found 12 substantiated issues, detailed inline.
Review status: capped/incomplete. Two new valuable findings survived the third and maximum independent round, so this review includes every validated issue but does not claim convergence.
Critical checkpoint conclusions:
- Goal and proof: the happy path reduces inline metadata, but the implementation does not preserve the exact-schema, recoverability, or lifecycle invariants needed to prove the change safe.
- Scope and focus: all 27 authoritative changed files were reviewed. No additional user focus was supplied. The unrelated JSON whitespace edit is out of scope and the live Clang Formatter check is failing on it.
- Concurrency and locks: a writer-publication race can create a stripped rowset without its schema key; index add/drop performs O(rowsets) synchronous metadata reads while holding the exclusive header lock. No separate lock-order deadlock was found.
- Lifecycle and errors: force restore, direct administrative deletion, and shutdown cleanup each mishandle UID-scoped schema ownership or broken-header cleanup; clone can mutate live ownership before a fallible schema write.
- Configuration: no new configuration defect was found. Existing default startup behavior makes a retained broken header fatal.
- Compatibility and protocol: the first migrated checkpoint is unreadable by the immediately prior BE; normal snapshot, clone-header, storage-migration, cloud, and tool transfer boundaries otherwise remain self-contained.
- Parallel paths and conditions: normal writers, the direct PushHandler writer, standalone recovery, CCR and row-binlog export, index add/drop, incremental/full clone, force restore, normal drop, administrative delete, and path-present/path-absent shutdown cleanup were traced.
- Tests and results: no build or test was run because the review bundle forbids it. The read-only BE build-hygiene check passed. The live Clang Formatter check fails. One changed path-GC test aborts during setup, and crash/restart/concurrency/downgrade/index/restore/delete/malformed-schema coverage is missing.
- Observability: dependency lookup failures are mislabeled as protobuf parse failures or conflated with header absence, so current logs/status propagation are insufficient.
- Transaction, persistence, and crash safety: schema publication is not guaranteed before stripped rowset metadata; upgrade recovery can later strip the only inline historical schema; clone failure can leave live rowsets pointing to files the caller unlinks.
- Data-write atomicity: successful schema-before-meta writes are directionally safe, but the writer flag, one-time migration flag, version-only identity, and post-mutation clone write create broken-reference windows.
- Transmitted variables and memory/static initialization: no new FE/BE transmitted-variable, static-initialization, or nullable-memory issue was found.
- Performance: repeated RocksDB reads under the exclusive header lock are a regression proportional to rowset count rather than distinct schemas.
| LOG(WARNING) << "set debug point RowsetMetaManager::save::zero_partition_id old=" | ||
| << partition_id << " new=" << rowset_meta_pb.DebugString(); | ||
| }); | ||
| if (rowset_meta.need_persist_schema()) { |
There was a problem hiding this comment.
[P1] Guarantee the schema key at the persistence boundary. This PB is stripped at line 82, but this branch trusts a flag captured before _build_current_tablet_schema() can advance the tablet's in-memory maximum. If the first writer for version N advances that maximum and aborts, a second writer for N records need_persist_schema()==false and can commit a rowset with neither an inline schema nor an rs_..._N key; PushHandler::_convert_v2() has the same default-false path. After a crash, DataDir::load() cannot recover that committed rowset and CCR schema export fails. Please idempotently persist or verify the exact schema before every stripped save, and cover the abort/commit ordering in a restart test.
| const int32_t schema_version = schema->schema_version(); | ||
| std::string schema_key = fmt::format("{}{}_{}_{}", ROWSET_SCHEMA_PREFIX, tablet_uid.to_string(), | ||
| schema_hash, schema_version); | ||
| if (schema_exists(meta, tablet_uid, schema_hash, schema_version)) { |
There was a problem hiding this comment.
[P1] Do not use schema version as the rowset-schema identity. Online inverted/ANN index add and drop copy a rowset schema, change its index metadata, and replace the rowset without incrementing schema_version. On an already migrated tablet this early return therefore preserves the pre-change bytes, while the replacement rowset is saved without its inline schema; after restart it is reconstructed with the wrong index metadata. Please reference an exact content identity or retain divergent schemas inline, and add restart coverage for both index add and drop.
|
|
||
| auto t1 = MonotonicMicros(); | ||
| serialize(&meta_binary); | ||
| serialize(&meta_binary, true); |
There was a problem hiding this comment.
[P1] Preserve rollback readability before writing this stripped format. The immediately prior BE ignores the new tablet_schema_saved field and only reads TabletMetaPB.schema plus each rowset's inline tablet_schema; after any ordinary checkpoint reaches this call, all of those fields are omitted. Rolling that node back then constructs an empty tablet schema and schema-less rowsets. Please dual-write during the supported rollback window or guard migration with an explicitly coordinated format gate, with an old-reader compatibility test.
| meta, _rowset_meta_pb.tablet_id(), TabletUid(_rowset_meta_pb.tablet_uid()), | ||
| _rowset_meta_pb.tablet_schema_hash(), _rowset_meta_pb.schema_version(), | ||
| &schema_binary); | ||
| if (!status.ok()) { |
There was a problem hiding this comment.
[P1] Preserve this lookup Status instead of reporting every failure as a parse error. DataDir::load() treats false as skippable malformed metadata, so a missing, corrupt, or unreadable schema key silently removes a committed rowset from recovery; the background unused-meta cleaner treats the same result as corruption and deletes its standalone record. Please distinguish protobuf corruption from schema dependency errors and fail or quarantine the load rather than continuing with a shortened version chain.
|
|
||
| // Clone and restore replace the tablet uid. Backfill schemas missing from legacy rowset metas | ||
| // before _add_tablet_unlocked() persists them under the new uid via TabletMeta::_save_meta(). | ||
| if (need_persist_schema) { |
There was a problem hiding this comment.
[P2] Reclaim the replaced UID's schema prefix during force restore. This path generates a new tablet UID and persists its schemas, but _add_tablet_unlocked(..., force=true) drops the old tablet with keep_files=true; that old object is not queued for _move_tablet_to_trash(), the only normal path that calls remove_schemas(). Repeated restores therefore retain every old rs_<uid>_... prefix indefinitely. Please remove the old UID's keys after replacement is durable, or add an ownership-proven orphan-schema sweep and a repeated-restore test.
| OlapMeta* meta = store->get_meta(); | ||
| Status res = meta->remove(META_COLUMN_FAMILY_INDEX, key); | ||
| VLOG_NOTICE << "remove tablet_meta, key:" << key << ", res:" << res; | ||
| if (res.ok() && header_prefix == HEADER_PREFIX) { |
There was a problem hiding this comment.
[P2] Remove the UID-scoped rowset schemas as part of this deletion contract. meta_tool's single and batch delete commands call TabletMetaManager::remove() directly, but a successful call now deletes only the header and ts_ key; no caller removes rs_<uid>_<hash>_..., and no background sweep scans that prefix. Once this header is gone, the UID needed to identify those keys is also lost, so every administrative deletion leaks them permanently. Please read or prove the UID and remove its prefix before erasing the header, with single- and batch-delete coverage.
| } | ||
| auto pair = TabletSchemaCache::instance()->insert( | ||
| TabletSchema::deterministic_string_serialize(tablet_schema)); | ||
| auto pair = TabletSchemaCache::instance()->insert(schema_binary); |
There was a problem hiding this comment.
[P1] Reject malformed schema bytes before inserting them into the cache. TabletSchemaCache::insert() parses this value with TabletSchemaPB::ParseFromString() but ignores the result, so a successful RocksDB lookup containing malformed bytes produces a non-null empty or partial TabletSchema. Deserialization then returns success, DataDir's null-schema fallback is bypassed, and restart can recover a committed rowset with an invalid schema instead of failing. Please propagate a parse Status and add malformed-rs_ restart coverage.
| } | ||
|
|
||
| for (const auto& rowset : to_add) { | ||
| RETURN_IF_ERROR(RowsetMetaManager::save_schema(data_dir()->get_meta(), tablet_id(), |
There was a problem hiding this comment.
[P2] Keep this RocksDB I/O out of the exclusive header-lock section. IndexBuilder holds get_header_lock() while calling modify_rowsets() and creates one output per candidate rowset. This loop calls save_schema() per output; for an already migrated same-version set, schema_exists() still performs KeyMayExist plus a synchronous DB::Get each time, commonly against the same key. Large index add/drop operations therefore do O(rowsets) metadata reads while publish waits. Please deduplicate exact schemas and persist or batch them before taking the lock, retaining fail-before-mutation ordering.
| rowset_meta->rowset_id(), | ||
| rowset_meta->get_rowset_pb(), binlog_format, | ||
| attach_row_binlog_rowset_meta)); | ||
| rowset_meta->set_persist_schema(true); |
There was a problem hiding this comment.
[P1] Persist inline recovered schemas too. During upgrade, DataDir::load() migrates legacy tablet headers and sets tablet_schema_saved before processing standalone rowsets. A visible old-format s_ record can already carry a historical inline schema, for example after a crash between publish_txn() and add_inc_rowset(), so this null-only block is skipped. add_rowset() then adds it to the migrated tablet; the next checkpoint strips its inline schema and deletes s_, but no rs_ key was written, making the following restart fail. Please save or verify every recovered visible schema before adding it, and cover upgrade plus checkpoint plus restart.
| << ", schema_hash=" << tablet->schema_hash() | ||
| << ", tablet_path=" << tablet_path; | ||
| return true; | ||
| return !check_st.is<META_KEY_NOT_FOUND>() || remove_separated_schemas(); |
There was a problem hiding this comment.
[P1] Do not treat this status as proof that the tablet header is absent. TabletMetaManager::get_meta() can read tabletmeta_ successfully and then return META_KEY_NOT_FOUND from the new deserializer when ts_ or a referenced rs_ key is missing. This branch, and the path-present branch above, then removes only side keys or the path and returns true, leaving the stripped header while dequeuing the shutdown tablet. The next startup traverses that header, fails before reading its shutdown state, and is fatal by default. Please distinguish raw header absence and delete the header plus proven side keys with retriable error handling.
3 participants
What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
Release note
Reduce local metadata size by storing reusable tablet and rowset schemas separately. Keep variant rowset schemas inline.
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)