fix(meta): skip unrecognized table index type in TableMeta deserialization#20115
fix(meta): skip unrecognized table index type in TableMeta deserialization#20115vismaytiwari wants to merge 1 commit into
Conversation
…ation TableMeta::from_pb deserializes every index eagerly, and TableIndex::from_pb returns Incompatible when TableIndexType::from_i32 does not recognize the stored discriminant. So once a table enables an index type a given binary does not know (e.g. a newer version added one), that binary fails to read the entire table metadata instead of just skipping the unknown index. Skip indexes whose type is not recognized so the rest of the metadata still loads. Recognized indexes and other incompatibilities are unaffected.
![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: 65d1179fda
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // Skip indexes whose type this binary does not recognize, so a table | ||
| // that enables a newer index type can still be read by an older binary | ||
| // instead of failing to deserialize the whole TableMeta (#20113). | ||
| .filter(|(_, index)| mt::TableIndexType::from_i32(index.index_type).is_some()) |
There was a problem hiding this comment.
When this runs on an older/rolling-upgrade binary against a table that contains a future index type, the filter removes that entry from TableMeta before the in-memory value is returned. Any later metadata write of the same table, even an unrelated one such as upsert_table_option mutating seq_meta.data and writing it back with txn_put_pb, will serialize the filtered map and permanently delete the newer index metadata. Either preserve the raw unknown entries through round-trip serialization or reject writes when unknown indexes were present instead of silently dropping them during deserialization.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — this is real. My patch lets an older binary read the table, but since reads round-trip through from_pb/to_pb (e.g. upsert_table_option mutates seq_meta.data and writes it back), dropping the unknown index on read means a later unrelated write silently deletes it — arguably worse than the original hard failure, which at least didn't lose data. Doing this safely means either preserving the raw unknown entries through the round-trip so to_pb re-emits them, or rejecting writes when unknown indexes were present — both reach beyond this deserialization spot into TableMeta. @zhang2014 which would you prefer? Happy to implement it.
1 participant
I hereby agree to the terms of the CLA available at: https://docs.databend.com/dev/policies/cla/
Summary
TableMeta::from_pbdeserializes every index eagerly, andTableIndex::from_pbreturns
IncompatiblewhenTableIndexType::from_i32does not recognize thestored discriminant (
src/meta/proto-conv/src/impls/table.rs). So once a tableenables an index type that a given binary does not know about — e.g. a table
created by a newer version that added a new index type — that binary can no
longer read the entire table metadata, not just the unknown index.
This skips indexes whose type is not recognized so the rest of the metadata
still loads and the table stays readable. Recognized indexes and any other
incompatibilities are unaffected.
Reproduced with
test_table_meta_skips_unknown_index_type: aTableMetawith avalid index plus one carrying an unrecognized
index_typefailsfrom_pbwithinvalid IndexType: 9999before the change; after it, deserialization succeedsand only the unknown index is dropped.
Tests
Type of change
This change is