Ma/convert builtins/mz roles by mtabebe · Pull Request #36912 · MaterializeInc/materialize

mtabebe · 2026-06-04T23:43:06Z

draft pr because it is stacked on: #36771

mtabebe · 2026-06-08T17:41:39Z

Failures are also present against main, so ignoring for now

ggevay

LGTM, just minor comments

ggevay · 2026-06-11T12:43:55Z

+        // The two built-in super-roles (mz_system, mz_support) are identified by
+        // name; they correspond to `MZ_SYSTEM_ROLE_ID` and `MZ_SUPPORT_ROLE_ID` in
+        // `mz_sql::session::user`. Their names are hard-coded constants there too,
+        // so filtering by name is stable.


Couldn't we grab these names from some compile-time constants, and interpolate into this SQL query?

ggevay · 2026-06-11T13:01:18Z

+        WHEN entry->'val' ? 'Flat' THEN entry->'val'->>'Flat'
+        ELSE (
+            SELECT pg_catalog.string_agg(t.elem, ', ' ORDER BY t.ord)
+            FROM jsonb_array_elements_text(entry->'val'->'SqlSet')


Flat and SqlSet here are from VarInput/OwnedVarInput, right? It would be good to put comments on those enums pointing here, so that if somebody adds a new variant later, they'll know to extend this SQL query.

ggevay · 2026-06-11T13:05:35Z

-            CatalogItemType::Table,
-            MZ_CATALOG_SCHEMA,
-            "mz_roles",
-        ),


This confused me for a moment, because above we say "Smallest supported version: 0.147.0", which I understood naively to mean that we can't remove migrations at versions not older than that. But Claude tells me it's fine, and suggests extending the doc comment with something like this:

/// Exception: when a builtin's `SystemObjectDescription` changes — e.g. a builtin table is /// converted to a materialized view (see `migrate_builtin_tables_to_mvs`), or a builtin is /// renamed or removed — existing steps naming the old description must be removed regardless /// of version, because `validate_migration_steps` panics on steps that don't resolve to a /// current builtin. This is safe only if a `Replacement` step for the new description is added /// at the conversion version: every environment that needed the removed steps upgrades from an /// even older version, so the new replacement subsumes them.

ggevay · 2026-06-11T13:12:00Z

+# Every assertion must pass against the current BuiltinTable
+# implementation. After the conversion, this file must pass unchanged
+# — any diff is a behavioural regression that must be either fixed
+# in the new view SQL or explicitly justified.


Is this comment still needed?

And the same question in mz_role_parameters.slt.

Ooops my bad

ggevay · 2026-06-11T13:16:07Z

+# reformatted a single quoted-string input like 'abc,def' by parsing and
+# re-joining it with ", ". Now the catalog stores 'abc,def' as written.
+# Multi-arg list inputs (ALTER ROLE ... SET search_path TO a, b, c) still
+# round-trip through Vec<Ident>::format() which joins with ", ".


Could mention that the new behavior is closer to the Postgres behavior. I asked Claude to verify this on an actual Postgres:

● Verified on your Postgres 18.4 — yes, the PR's new raw-display behavior is exactly the Postgres convention, with one small quoting nuance. (I created and dropped a throwaway canon_test role for this.)

What Postgres does:

┌─────────────────────────────┬───────────────────────────┬──────────────────────────────┐ │ Input │ setconfig/useconfig shows │ Materialize after this PR │ ├─────────────────────────────┼───────────────────────────┼──────────────────────────────┤ │ statement_timeout = '60000' │ statement_timeout=60000 │ 60000 — matches │ ├─────────────────────────────┼───────────────────────────┼──────────────────────────────┤ │ enable_seqscan = 'yes' │ enable_seqscan=yes │ yes — matches │ ├─────────────────────────────┼───────────────────────────┼──────────────────────────────┤ │ search_path = aa, bb, cc │ search_path=aa, bb, cc │ aa, bb, cc — matches │ ├─────────────────────────────┼───────────────────────────┼──────────────────────────────┤ │ search_path = 'abc,def' │ search_path="abc,def" │ abc,def — differs in quoting │ └─────────────────────────────┴───────────────────────────┴──────────────────────────────┘

So Postgres stores and displays the raw input in pg_db_role_setting.setconfig/pg_user.useconfig — no canonicalization. Meanwhile SHOW statement_timeout on an active session returns 1min: Postgres canonicalizes
at SHOW-time, not at storage. That means the old Materialize behavior (canonicalizing inside the catalog table) was the un-Postgres-like one, and this PR moves toward Postgres. The pg_catalog_user.slt
expectation change is a compatibility improvement, not a regression — worth saying exactly that in the PR description.

The one nuance: for the comma-containing string, Postgres shows "abc,def" with quotes, because for Postgres SET search_path = 'abc,def' is a single schema whose name contains a comma (confirmed: SHOW
search_path → "abc,def"). Materialize parses that same input as two schemas (the old canonical display abc, def shows this) — that's a pre-existing semantic difference in how Materialize interprets Flat strings
for list vars, not something this PR introduces or could fix in display. The MV showing unquoted abc,def is faithful to Materialize's own semantics.

ggevay · 2026-06-11T13:27:46Z

 pub const VIEW_MZ_BUILTIN_SOURCES_OID: u32 = 17087;
 pub const MV_MZ_INDEXES_OID: u32 = 17088;
+pub const MV_MZ_ROLES_OID: u32 = 17089;
+pub const MV_MZ_ROLE_PARAMETERS_OID: u32 = 17090;


Suggestion: reuse the old OIDs instead of allocating new ones.

Seven of the eight prior table→MV conversions (mz_databases, mz_schemas, mz_role_members, mz_connections, mz_secrets, mz_materialized_views, and your mz_sources) kept the OID and just renamed the constant, e.g. TABLE_MZ_SOURCES_OID → MV_MZ_SOURCES_OID (= 16710). Only mz_indexes (#36771) allocated a new one.

Reuse seems preferable here:

It matches the durable identity: migrate_builtin_tables_to_mvs deliberately preserves the CatalogItemId/GlobalId, so the system treats this as the same object changing representation. A new OID makes the pg-facing identity churn while mz_objects.id stays stable.

Builtin item OIDs are compiled-in statics (not durably persisted), so reuse is mechanically safe — the reused-OID conversions have been deployed since ~26.18 without issue.

It avoids leaving TABLE_MZ_ROLES_OID/TABLE_MZ_ROLE_PARAMETERS_OID as dead constants, and keeps the oid.slt diff minimal (mz_roles stays at 16720 instead of disappearing and reappearing at 17089).

Concretely: rename TABLE_MZ_ROLES_OID → MV_MZ_ROLES_OID (= 16720) and TABLE_MZ_ROLE_PARAMETERS_OID → MV_MZ_ROLE_PARAMETERS_OID (= 16945), and drop the new 17089/17090 constants.

Relatedly: since 26.29 is still -dev, do you want to realign mz_indexes (17088 → 16705) in a follow-up before the release cut, for consistency across the series?

I'll get it in this PR and merge today

…zedView Problem: mz_catalog.mz_roles and mz_catalog.mz_role_parameters are BuiltinTables whose rows are written by pack_role_update. This requires coordinated writes from a single environmentd process, conflicting with the multi-envd goal. Solution: Convert mz_roles and mz_role_parameters to BuiltinMaterializedViews backed by queries over mz_internal.mz_catalog_raw. The Public role is filtered out of mz_roles to match the prior pack_role_update behavior. For mz_role_parameters, the OwnedVarInput::SqlSet branch unpacks the JSON array via jsonb_array_elements_text WITH ORDINALITY and reassembles it with string_agg(..., ', ' ORDER BY ord) so that list elements preserve their original order (matching Vec<Ident>::format()). Behavior change: The previous BuiltinTable populator called var.check() on the user input before storing it. For list-typed session vars (e.g. search_path), var.check() reparsed and reformatted a single quoted-string input like 'abc,def' into 'abc, def'. The new materialized view stores the raw user input verbatim, so 'abc,def' is preserved as-is. Multi-argument inputs (ALTER ROLE ... SET search_path TO a, b, c) still round-trip through Vec<Ident>::format() and are joined with ", ". Testing: New behavioural lockdown test for mz_role_parameters. Updated tests to reflect that mz_roles and mz_role_parameters are now MATERIALIZED VIEWs, and to reflect the search_path normalization behavior change.

mtabebe force-pushed the ma/convert-builtins/mz-roles branch 4 times, most recently from bc83f39 to c42b2e9 Compare June 8, 2026 16:19

mtabebe requested review from SangJunBak and ggevay June 8, 2026 17:41

mtabebe marked this pull request as ready for review June 8, 2026 17:41

mtabebe requested a review from a team as a code owner June 8, 2026 17:41

mtabebe mentioned this pull request Jun 10, 2026

[catalog] convert mz_clusters/mz_cluster_replicas/mz_cluster_schedules to BuiltinMaterializedView #36969

Open

ggevay approved these changes Jun 11, 2026

View reviewed changes

mtabebe force-pushed the ma/convert-builtins/mz-roles branch from c42b2e9 to f60af94 Compare June 11, 2026 14:02

mtabebe force-pushed the ma/convert-builtins/mz-roles branch from f60af94 to 7e7d531 Compare June 11, 2026 14:54

mtabebe enabled auto-merge (squash) June 11, 2026 15:12

mtabebe merged commit 5b42d5e into MaterializeInc:main Jun 11, 2026
117 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ma/convert builtins/mz roles#36912

Ma/convert builtins/mz roles#36912
mtabebe merged 1 commit into
MaterializeInc:mainfrom
mtabebe:ma/convert-builtins/mz-roles

mtabebe commented Jun 4, 2026

Uh oh!

mtabebe commented Jun 8, 2026

Uh oh!

ggevay left a comment

Uh oh!

ggevay Jun 11, 2026

Uh oh!

ggevay Jun 11, 2026

Uh oh!

ggevay Jun 11, 2026

Uh oh!

mtabebe Jun 11, 2026

Uh oh!

ggevay Jun 11, 2026

Uh oh!

mtabebe Jun 11, 2026

Uh oh!

ggevay Jun 11, 2026

Uh oh!

ggevay Jun 11, 2026

Uh oh!

ggevay Jun 11, 2026

Uh oh!

mtabebe Jun 11, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

mtabebe commented Jun 4, 2026

Uh oh!

mtabebe commented Jun 8, 2026

Uh oh!

ggevay left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants