Repository navigation
Add ItemColumn and IndexColumn durable collections - #39818
Conversation
QA LLM Review1. MEDIUM -- Bootstrap refresh records columns from
|
| pub nullable: bool, | ||
| pub type_oid: u32, | ||
| pub type_mod: i32, | ||
| pub custom_type: Option<CatalogItemId>, |
There was a problem hiding this comment.
Rather than the SQLScalarType, I decided to put the computed derived state since I didn't want to embed all of SQLScalarType into the durable catalog
1d28288 to
ce75aca
Compare
|
Here's a couple comments from an LLM review. I haven't looked at the code yet, but the suggestion make sense if the current behavior or coverage is actually as they describe. 😅 -- AI below -- Blocking: I found no concrete correctness blocker in the current diff. The existing review comment about refreshing pre-migration state is addressed by the current placement in Strong suggestion: test stale metadata, not just missing metadata. The bootstrap test deletes two rows and verifies they are restored. It does not exercise the important upgrade promise: an existing row has the wrong nullability/type, or the new planner produces fewer columns. The durable-layer test covers replacement and truncation separately, but a bootstrap test with deliberately stale rows would connect those pieces. Nit: |
|
@aljoscha the feedback makes sense! I've addressed it in changes: test: hoist the item metadata refresh test helpers |
SangJunBak
changed the title
SangJunBak
changed the title
66de35e to
fe755d4
Compare
QA LLM Review1. MEDIUM -- Every catalog transaction now rebuilds and decodes one entry per column of every relation
Each catalog transaction rebuilds the full proto Details
Other measurements, all optimized: at 5k items × 20 columns, |
aljoscha
left a comment
aljoscha
left a comment
There was a problem hiding this comment.
The shape of this looks good, but what Dennis found here is concerning: #39818 (comment). Can we fix that?
|
@aljoscha @def- Implemented a version where instead of keying on the object and position, we key on just the object then store a vec for the columns. That way it should scale with the number of objects. Repeated the same benchmark and found: Still worse, but maybe a price we pay? I'll push the commits with the schema changes after this comment. |
|
Yeah, I think that' okay. I think we have to independently tackle that problem that we do touch all this catalog content for a snapshot/transaction, which is a pre-existing problem. |
ec2e0b2 to
e0b0f9e
Compare
|
@aljoscha Ah sorry forgot to push! Also rebased off main too. Here are the PRs and the commits that do the change (they should each be on top). PR #39818, Add ItemColumn and IndexColumn durable collections: #39818
PR #40001, Convert mz_columns to a builtin materialized view: #40001
PR #39427, Convert mz_index_columns to a builtin materialized view: #39427
|
All of this commit is boilerplate. The durable catalog gains two collections: ItemColumn, one row per column of a relation, and IndexColumn, one row per key of an index. Both will be used by mz_columns and mz_index_columns. We cannot easily derive these values from each object's DDL statements, thus we store its column information durably/
For columns and index columns, we implement a setter `set_item_metadata` and a cleanup method `truncate_item_metadata` such that for every update to an item, we update an item's column information durably. The goal is to remove the code we have in builtin_table_updates for our builtin column relations.
initialize_state records the columns and index keys of every item as the running build's planner produces them, right after our builtin table updates. We introduce a getter `is_read_only` such that the catalog in debug mode doesn't fail when we refresh the column information. We can't use config.read_only since this variable actually is intended for the read only 0dt follower process, which opens the catalog in savepoint mode.
The rows are written with the item, replaced as a whole, and dropped with it by either removal path.
We'll need this for a future that tests stale metadata
Although we tested before for missing column information, we don't test if an existing row has the wrong nullability/type, or if the new planner produces fewer columns. Thus we create a new test for these cases.
truncate_item_metadata would assume that column positions are contiguous but we never explicitly call out this contract. Thus we document this contract in truncate_item_metadata and soft assert on callers of it.
Because transactions scale with the number of keys in the catalog, by having the column position in the key, we negatively impacted performance by a fair bit. By storing a vector of the columns and keying by the catalog ID, we notice only a ~2x regression on transactions rather than a 9-15x regression. Much of this commit is boilerplate/updating tests. I did make some changes to change the voice of some of the comments to my own. I also simplified the truncation code since now we can just remove by the catalog id rather than iterating over (catalog_id, position) and deleting column information that way.
e0b0f9e to
560e5f4
Compare
From the durable column information we recorded in #39818, we derive the column information mz_columns and convert it to a builtin mv. See commit messages for more details ### Motivation Part of sql-494 ### Description We report the column defaults from a create_sql statement, generate the mz_builtin_columns for our builtin collections since they don't exist in our catalog, then we convert mz_columns to a builtin mv using a combination of mz_builtin_columns and mz_catalog_raw. ### Verification - Expanded the existing SLT with a more thorough test suite that asserts against a column of each type in our system. - Diffed this relation using the prod analytics workload replay against main and this branch.
From the durable index key information we recorded in #39818, we derive the index key information from it and convert mz_index_columns to a builtin mv. See commit messages for more details ### Motivation Part of sql-494 ### Verification - Diffed this relation using the prod analytics workload replay against main and this branch. - Created an SLT for index columns that assert expected behavior as well as behavior for indexes where the keys are varying expressions
Motivation
Part of SQL-494
Verification