Repository navigation
Refactor out inlined values in builtin materialized views - #39468
Conversation
SangJunBak
changed the title
3db1f61 to
03701c4
Compare
03701c4 to
631ddf2
Compare
0ce5fa8 to
a3430b2
Compare
a3430b2 to
b3e9ac9
Compare
mtabebe
left a comment
mtabebe
left a comment
There was a problem hiding this comment.
I want to make sure I understand this right.
The argument is that we don't need to add the VALUES into the fingerprint of the materialized view because (1) if the schema stays the same we have self correcting behaviour and (2) if the schema does change we may have errors and we will catch them in tests.
The (2) feels fuzzy to me. We will catch them in tests based on our catalog, but what if the customers have different catalogs? Can we be stronger in anyway?
Or are you saying the only errors possible through (2) are schema definition errors which do not depend on the data in the catalog?
Or maybe it is a different argument all together, that we are just moving around definitions?
I guess I'd just like some clarity.
| let materialized_views: &'static BuiltinView = | ||
| Box::leak(Box::new(make_builtin_materialized_views(mv_iter))); | ||
| let tables: &'static BuiltinView = Box::leak(Box::new(make_builtin_tables(table_iter))); | ||
| Box::leak(Box::new(make_builtin_materialized_views(builtin_items))); |
| FROM (VALUES {values}) AS v(oid, schema_name, name, type, privileges)" | ||
| FROM (VALUES {source_values}) AS v(oid, schema_name, name, type, privileges) | ||
| UNION ALL | ||
| SELECT oid, schema_name, name, 'log', privileges |
|
I went through the 7 commits and the code does look good, I'm just trying to make sure my understanding of the error case is correct. I also think this is a change where we should do a nightly run? |
I was thinking this! So the cases where I can see a regression happening are:
We would fail on boot which CI should catch when planning the builtins.
This would change the SQL of the mv, so the fingerprint mismatch in the 0dt smoke test would guide us to creating a shard migration.
If the new type works with the original SQL of the mv but the byte representation changes (e.g. int32 to int64), then persist fails to register the However, we have a test called
If we said a column was not nullable in its desc but then an upstream view emits null, then the replica would panic on write after registration from the assert in There's also the case I said in a previous commit message:
I've since realized though that we're not guaranteed to query all builtins in our SLTs. However,
Because persist interprets columns by position and not name (repr/src/row/encode.rs:1516), if types of the column have a different encoding, we fail to register. However if it has the same encoding (e.g. we change Proposed solution: Although And in a follow up PR (my preference), we can change the fingerprint scheme for a builtin MV to be its I can also change the commit message:
to better explain why this is acceptable! |
|
Thanks for laying out these cases, can we put this comment somewhere in the code too? A follow up PR for the fix seems fine to me. |
5fd4d3e to
1ecc7db
Compare
ggevay
left a comment
ggevay
left a comment
There was a problem hiding this comment.
LGTM, two comment nits. The Nightly reds are pre-existing: the three orchestratord jobs are CLO-315 (red on main and the release tags too), and Parallel Workload (kill) hit SS-346's bootstrap panic after a kill, which also shows up on main.
| // 4. The upstream relation changes a referenced column's nullability: as in 3, | ||
| // `verify_builtin_descs` fails until `desc` is updated. If the shard keeps a | ||
| // non-nullable column and the relation emits NULL, the replica panics in | ||
| // `DatumEncoder::push` on write. |
There was a problem hiding this comment.
Nit: this case can't reach DatumEncoder::push. The sink opens its writer with the planned desc, which becomes nullable as soon as the upstream emits NULL, and every boot evolves the MV's shard to it (evolve_nullability_for_bootstrap, coord.rs:3486). So widening needs no MigrationStep, and narrowing terminates envd at boot:
| // 4. The upstream relation changes a referenced column's nullability: as in 3, | |
| // `verify_builtin_descs` fails until `desc` is updated. If the shard keeps a | |
| // non-nullable column and the relation emits NULL, the replica panics in | |
| // `DatumEncoder::push` on write. | |
| // 4. The upstream relation changes a referenced column's nullability: as in 3, | |
| // `verify_builtin_descs` fails until `desc` is updated. Boot then evolves the | |
| // shard to the new desc (`evolve_nullability_for_bootstrap`), which persist | |
| // accepts for widening and rejects for narrowing, terminating envd. |
| /// `mz_builtin_sources` reads the privileges for its log rows. See | ||
| /// `MZ_INDEXES` for what that means for migrations. |
There was a problem hiding this comment.
Nit: MZ_INDEXES has no migration text; the explanation lives on the Fingerprint impl for BuiltinMaterializedView.
| /// `mz_builtin_sources` reads the privileges for its log rows. See | |
| /// `MZ_INDEXES` for what that means for migrations. | |
| /// `mz_builtin_sources` reads the privileges for its log rows. See the | |
| /// `Fingerprint` impl for `BuiltinMaterializedView` for what that means for migrations. |
1ecc7db to
cb7dbd0
Compare
QA LLM Review1. MEDIUM -- "Only nullability changed" exemption skips a needed
|
cb7dbd0 to
6638c19
Compare
…exes mz_indexes is a stable mz_catalog relation, so later changes to how builtin indexes are reported must not touch its schema. Moving the builtin index rows into their own mz_internal.mz_builtin_indexes view lets that view be extended and reused while mz_indexes reads it like any other upstream relation. The SQL of mz_indexes changes, so it gets a replacement MigrationStep. Reading the rows from a view instead of inlining them as VALUES means the mz_indexes fingerprint, which is its SQL, no longer changes when the set of builtin indexes changes. That is fine: adding or removing a builtin index changes the rows of mz_builtin_indexes but not its schema, and row changes flow through the mz_indexes dataflow like any other input. What the fingerprint can no longer see is a change to the shape of the upstream view. The ways that can regress a builtin MV, and what catches each: 1. The upstream view drops a referenced column: planning the MV fails at catalog open, which every test that opens a catalog hits. 2. The upstream view adds a column that the MV is changed to reference: the SQL changes, so the fingerprint changes, and update_fingerprints panics at upgrade until a MigrationStep is added. 3. The upstream view changes a referenced column's type: the planned desc changes and verify_builtin_descs fails until the declared desc is updated. The fingerprint does not change, so without a MigrationStep the coordinator evolves the existing shard to the new desc at bootstrap (evolve_nullability_for_bootstrap). Persist refuses when the Arrow encoding differs (int32 to int64), which terminates environmentd, and cannot tell when the encoding is shared (timestamp to timestamptz), which silently reinterprets the stored bytes. 4. The upstream view changes a referenced column's nullability. For a column under ASSERT NOT NULL, which covers every column of mz_indexes, the planner keeps it non-nullable, so verify_builtin_descs passes and the MV instead errors at runtime with MustNotBeNull once a NULL arrives. Only a test that queries the MV catches that. For other columns verify_builtin_descs fails until the declared desc is updated, and the shard then evolves at bootstrap without a MigrationStep. 5. The MV is SELECT * over the upstream view, which reorders or swaps columns: persist matches columns by position, so this is case 3 for each moved column. verify_builtin_descs plans every builtin against a debug catalog and compares the result with the declared desc, and its failure message names the MigrationStep requirement for shard-backed builtins. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…indexes - Because we don't need to inline VALUES into the DDLs of our builtin materialized views, we can create a new mz_builtin_log_indexes such that both mz_sources and mz_indexes can query it rather than get sent an iterator of the builtin logs. - We simply extract inline VALUES code into a new builtin view called mz_builtin_logs then reference it in mz_builtin_indexes - This comes with the nice benefit of not having to dynamically generate each mz_indexes and define the order statically. - We can also turn make_mz_indexes into a static MZ_INDEXES like the rest of our builtins - In a later commit, we'll making the same refactor for mz_sources
- In mz_sources, rather than inline the builtin sources and logs as a VALUES list, read them from mz_builtin_sources - mz_builtin_sources already existed inside "make_builtin_sources", however it was just never used and was dead code. This is why we delete code in make_mz_sources but don't see a positive diff in this commit. However if you were to look inside make_builtin_sources, the same code we deleted exists there.
…tin_log_indexes - Move `privileges` to `mz_builtin_log_indexes` from `mz_builtin_sources` - Instead of embedding the builtin logs to mz_sources, just join on `mz_builtin_log_indexes`
Simply move make_mz_sources to a static mz_sources to remove an unnecessary function call.
Given we're not sharing iterators across these view generators, we can simplify the code further.
A builtin materialized view's fingerprint is its SQL, so a change to the shape of a relation it reads is not seen by the fingerprint check at upgrade. Record the ways that can regress a builtin MV, and what catches each, at the Fingerprint impl. Notably a nullability change in a column under ASSERT NOT NULL is invisible to verify_builtin_descs, because the planner keeps the asserted column non-nullable, and only shows up as a runtime MustNotBeNull error on the MV. verify_builtin_descs is the check that catches the type and column-order cases, but a developer who fixes the declared desc it complains about still has to add a MigrationStep for a shard-backed builtin. Say so in its failure messages for tables, sources and materialized views, and check the column count before zipping so a SELECT * that picked up a new column fails with the same guidance instead of a bare zip_eq panic. A keys-only mismatch gets its own message without the hint, since keys never reach the shard schema. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
6638c19 to
e5236b6
Compare
4 participants
I'd recommend this PR commit by commit. All of this is entirely a refactor and most is just moving code around.
Motivation
Closes sql-653
Description
Should have no customer facing changes. The biggest change is instead of inlining builtin values into the fingerprint, we can use our original pattern of using upstream "mz_builtin_*" views. Initially we thought we had to inline the builtin VALUES into the materialized view, otherwise a silent change to an upstream view (i.e. mz_builtin_indexes) would silently create an error in the materialized view. However we've since realized that as long as the schema of the upstream view stays the same, any updates to it will simply sink into the materialized view. If the schema were to change however, e.g. we ASSERT NOT NULL on the mv but have the upstream view provide a NULL value, then the mv would error when queried. However, we have tests that actually query each builtin so this regression would be caught.
Verification
A proof point of this working is mz_object_dependencies. It is an mv that references an upstream view
mz_object_dependencies_rawthat's constantly changing. This has been in production for a while but things are healthy due to what I stated prior.