fix(sysview): enforce object metadata visibility - #27695
Conversation
XuPeng-SH
left a comment
There was a problem hiding this comment.
Deep re-review completed at exact head b1df766af70ec4ce7bbba15a2b0f3e508e8e1643.
The two authorization blockers from my prior review are closed in this implementation: mo_current_roles() computes a cycle-safe complete inherited-role closure and reads the active role at execution time; SCHEMATA now derives database visibility from ownership, database metadata privileges, visible objects, account-level SHOW DATABASES, and the intended system schemas. The public SQL matrix also covers three role levels, direct/no grant, role rename, prepared and ordinary plan-cache reuse across SET ROLE, sibling metadata views, database ownership/privilege, admin, and cleanup.
Requesting changes for two remaining blockers:
-
[P1 / rolling-upgrade correctness] Rebase and allocate a unique protocol capability. Current
origin/mainis22d56a30b102b58050a31ff04e4f5187541c0f7band already definesMORPCVersion33for stable complete-key distributed string shuffle. This head independently defines version 33 formo_current_roles, and GitHub correctly reports the PR as conflicting inpkg/defines/const.go. This is not just a textual conflict:requireCurrentRolesProtocol, tenant bootstrap, and all ten same-version view upgrades interpret common version 33 as proof that every CN implementsmo_current_roles. During a rolling deployment, an existing v33 CN from main lacks that table function, so a newer CN can install/plan canonical views that an older CN cannot execute. Rebase the latest main, assign the next still-unused version (currently 34), and updateMORPCLatestVersion, the planner/bootstrap gates, everyRequiredProtocolVersion, tests, and comments together. -
[P1 / performance and OOM unhappy path] Bound role-closure work to the relevant graph. Every query of any protected
information_schemaview currently runsSELECT granted_id, grantee_id FROM mo_catalog.mo_role_grant, materializes the entire tenant result, copies it into[]roleGrantEdge, and then duplicates all edges into a Go adjacency map even when the active role has no inherited edge. There is no hard tenant role/grant limit. On this exact head, the closure stage alone for 100,000 completely disconnected legal edges measured 5.66 ms/op, 11,292,160 B/op, and 100,532 allocs/op on Apple M4; this excludes the internal executor result anddecodeCurrentRoleGrantEdgescopy. Concurrent ORM/driver metadata discovery therefore scales memory and CPU with the tenant's unrelated global role graph and can cause large transient amplification or OOM. Fetch only the reachable frontier, reuse an appropriately invalidated bounded privilege/role cache, or provide another design whose per-query resource use is bounded by the active closure; add a disconnected-large-graph benchmark/stress regression.
Resource ownership and cancellation are otherwise sound: the internal result is closed, the table-function batch is operator-owned, traversal is cycle-safe, and no goroutine or wait edge is introduced. Exact-head CI is green, but it cannot detect the newly landed protocol collision or the unbounded legal-catalog scale case.
# Conflicts: # pkg/bootstrap/versions/v4_0_6/tenant_upgrade_list.go # pkg/bootstrap/versions/v4_0_6/upgrade_test.go # pkg/defines/const.go
aunjgr
left a comment
There was a problem hiding this comment.
Reviewed exact head b1df766af70ec4ce7bbba15a2b0f3e508e8e1643 against merge base a73f76d109bd528ae2e53a4b8ab294422fb13e21 and current main.
The inherited-role and SCHEMATA authorization gaps are closed, but two blockers remain:
-
Current main assigns v33 to stable string-shuffle ownership and v34 to persisted unsigned metadata. This head independently advertises
mo_current_rolesat v33 and uses that value in planner/bootstrap/view-upgrade gates. A main-v33 CN does not implement the table function, so a mixed deployment can install or plan canonical views that an advertised-v33 participant cannot execute. Merge current main, allocate the next unique cumulative generation, and update every gate/test together. -
mo_current_roles()executes an unfiltered scan of everymo_role_grantedge, copies all rows into[]roleGrantEdge, and duplicates the complete tenant graph into an adjacency map before traversing the active closure. There is no hard role/grant bound. Thus one metadata query by a role with no inherited edges still performs O(all tenant grants) work and allocations; concurrent metadata discovery can amplify this into material latency and OOM. Restrict loading to the reachable frontier or use an appropriately invalidated bounded authorization cache, and retain a disconnected-large-graph regression.
The result and batch ownership paths are otherwise closed, traversal is cycle-safe, and exact-head CI is green.
aptend
left a comment
There was a problem hiding this comment.
Reviewed exact head 66efe77, including every prior review/thread/reply, the increment since the last reviewed head, and the complete diff. The prior inherited-role closure, SCHEMATA/sibling-view, protocol-v33, and unbounded role-graph scan blockers are materially addressed by this revision. The focused package tests, focused race tests, and the large-disconnected-graph benchmark all pass. One new rolling-upgrade blocker remains because main advanced after this head was created: current main 02af906 already owns protocol v35 for a different wire contract.
# Conflicts: # pkg/defines/const.go
aptend
left a comment
There was a problem hiding this comment.
Deep re-review completed at exact head edc3708. I read the complete review and thread history, checked the increment since the previous reviewed head 66efe77, and revalidated the complete security/upgrade diff.
The remaining rolling-upgrade blocker is closed: current-role capability, planner admission, tenant bootstrap, all metadata-view upgrades, the supporting role-grant index, tests, and MORPCLatestVersion now consistently use the new cumulative protocol v36. Current main b789245 still ends at v35 for scaled variance, so v36 is unique and a v35 participant cannot be mistaken for one implementing mo_current_roles.
The prior authorization and resource blockers remain closed: cycle-safe transitive role closure is evaluated from the stable active role ID, frontier queries are limited to reachable grants in bounded 256-role batches, visibility covers the protected and sibling metadata views plus SCHEMATA, and pre-v36 bootstrap uses the fail-closed compatibility definitions.
Exact-head validation passed:
- full tests for table_function, plan, compile, sysview, v4_0_6 upgrade, and frontend packages
- focused race tests across current-role/protocol/metadata-visibility paths, count=3
- disconnected 100k-edge closure benchmark: about 535-549 ns/op, 512 B/op, 17 allocs/op
- go vet and git diff --check
I found no remaining blocking correctness, authorization, rolling-upgrade, resource-lifecycle, or performance issue.
XuPeng-SH
left a comment
There was a problem hiding this comment.
Deep re-review completed at exact head edc3708. The prior authorization, protocol-v36, reachable-frontier, upgrade, and unhappy-path blockers are closed, and exact-head CI is green. One new performance blocker remains: the visibility CTE does not evaluate the role closure once per metadata query. Because the planner does not share CTEs containing FUNCTION_SCAN nodes, repeated references to __mo_active_roles are inlined into independent mo_current_roles() scans. A focused exact-head planner probe that counts only nodes reachable from Query.Steps found 3 independent scans for TABLES, COLUMNS, STATISTICS, CHECK_CONSTRAINTS, and VIEWS, and 6 for SCHEMATA. Each scan invokes the internal SQL executor at least once for SELECT ... FROM mo_role_grant; a depth-d inherited chain repeats that frontier work by level. Therefore common ORM or driver metadata discovery pays 3–6 complete nested-SQL closure evaluations per view query. The reported approximately 535 ns, 512 B benchmark exercises currentRoleClosure with an in-memory map expander and does not measure any of these internal SQL executions, planning, result batches, or transaction work. Please make the active-role closure statement-local and shared, or restructure the visibility plan so there is exactly one closure evaluation per information_schema query, and add a plan-shape assertion plus an end-to-end benchmark covering the real internal-SQL path. The compatibility and correctness work otherwise looks sound.
aunjgr
left a comment
There was a problem hiding this comment.
Re-reviewed exact head edc370838461be905ab7798857eb2534446f0b47 against merge base 02af906c367b3ef7e530336651ba033f8c14861c and newest main@958a4ceb9e73c5e70928304996873e089279b667.
The prior security and compatibility blockers are closed: metadata visibility covers the protected sibling views and SCHEMATA; the active-role closure is cycle-safe, stable-role based and restricted to reachable frontier queries; pre-v36 definitions fail closed; and v36 is a unique cumulative capability after main’s v35 variance contract. Result/batch ownership and error cleanup are sound.
One material metadata-query performance blocker remains. informationSchemaMetadataVisibilityCTEWithActiveRoles references __mo_active_roles three independent times while deriving __mo_visible_tables (table owner, database owner, role privileges), and three more while deriving __mo_visible_databases. The planner explicitly rejects CTE subtree reuse when it contains Node_FUNCTION_SCAN (cteSubtreeIsDeterministic), so these references are inlined rather than sharing one mo_current_roles() result. Consequently TABLES/COLUMNS/STATISTICS/CHECK_CONSTRAINTS/VIEWS plans contain three reachable current-role scans and SCHEMATA contains six. Every scan runs currentRoleClosure, which issues at least one nested internal SQL query against mo_role_grant, and a depth-d inheritance chain repeats the frontier SQL by level for every copy.
This affects ordinary ORM/driver metadata discovery and is not measured by the reported ~535 ns / 512 B benchmark: that benchmark injects an in-memory frontier expander and excludes internal SQL planning/execution, result batches, transactions, and the 3–6 plan copies. Make the active-role closure statement-local and shared (or restructure the plan to one execution), then add a reachable-plan assertion requiring exactly one mo_current_roles FUNCTION_SCAN per protected metadata query plus a benchmark/test exercising the real internal-SQL boundary. No other blocking correctness, authorization, lifecycle, compatibility, or boundedness issue remains.
aunjgr
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 63c9e472b0d4112a451a8a6b6d4960dd2d6c6b19 against merge base 67ac89c05858d6114fe078a48dccfb0a7ca7f411 and newest main@52336df80596de93a3f279dfcd6c993d7227bcb9.
The prior multiplicity blocker is closed for the intended views: reachable plans for TABLES/COLUMNS/STATISTICS/CHECK_CONSTRAINTS/VIEWS/SCHEMATA now contain one mo_current_roles producer and one statement-local sink, so prepared and ordinary plan-cache executions still evaluate the current session role at runtime without repeating the frontier SQL. Security, v36 rollout, frontier bounds, and operator ownership remain coherent.
One blocking planner-scope issue remains. The new fast path does not identify the exact __mo_active_roles AS (SELECT role_id FROM mo_current_roles()) shape; cteSubtreeContainsStatementStableFunction returns true when mo_current_roles appears anywhere in an otherwise deterministic CTE subtree. If every occurrence contains that leaf, canReuseCTE bypasses hasNestedUse, cteConsumersFullyDrain, cost/profitability, variable-width, and the 32 MiB materialization bound solely because the subtree contains the function.
A user CTE such as WITH c AS (SELECT n.*, r.role_id FROM large_table n CROSS JOIN mo_current_roles() r) ... therefore takes the privileged path. With repeated LIMIT/SEMI consumers, the historical inline plan could stop early, while this head eagerly materializes the complete large-table × role-closure output with no size guard. The comment’s premise—that mo_current_roles itself computes one fixed-width batch before emitting—does not bound or make fully draining the surrounding JOIN/AGG/scan subtree safe; this can turn a small metadata optimization into material latency or OOM for unrelated CTEs.
Restrict the exception to the exact statement-stable role CTE (allowing only cardinality-preserving projection wrappers), or attach an explicit planner marker to that definition. Every CTE containing additional scans/joins/aggregates must retain the normal nested-use, full-drain, profitability and memory gates. Add negative plan tests for a large-table join plus LIMIT/SEMI and keep the existing positive one-scan information_schema matrix. No other blocking authorization, compatibility, lifecycle, or performance issue remains.
XuPeng-SH
left a comment
There was a problem hiding this comment.
Deep re-review completed at exact head 63c9e472b0d4112a451a8a6b6d4960dd2d6c6b19. The previous authorization, protocol-v36, reachable-frontier, upgrade, and repeated 3–6 role-closure execution blockers are closed for the protected metadata views: the new plans contain one statement-local mo_current_roles() producer, and the prior feature diff outside this optimization remains semantically unchanged after the latest main merges.
One blocking planner/performance regression remains:
[P1] Restrict the early-stop/materialization exemption to the exact bounded role-closure CTE. In canReuseCTE, lines 67–82 classify an occurrence by whether its subtree merely contains a zero-argument mo_current_roles scan. That condition immediately returns true and bypasses hasNestedUse, cteConsumersFullyDrain, the 32 MiB estimate guard, and the profitability check. Therefore any otherwise deterministic user CTE can add mo_current_roles() beside an arbitrarily large table and become force-materialized even when every consumer has LIMIT 1 or a short-circuiting SEMI/ANTI path.
I reproduced this on the exact head with:
WITH c AS (
SELECT r.role_id, t.rel_id
FROM mo_current_roles() AS r
CROSS JOIN mo_catalog.mo_tables AS t
)
(SELECT * FROM c LIMIT 1)
UNION ALL
(SELECT * FROM c LIMIT 1);A focused real-planner test confirms the current plan has one shared SINK and one mo_current_roles scan. Before this change the two inline branches can each stop after one row; now the producer must consume and materialize the complete join. Replacing mo_tables with a large legal user table turns a two-row query into a full table scan plus up to 64 MiB retained memory and then query-scoped spill, eventually failing at the spill budget for sufficiently large input. The comment at lines 76–81 is true for the function output itself but not for an arbitrary ancestor subtree containing it.
Please recognize only the exact bounded fixed-width closure shape (for example, a projection directly over the zero-argument function with no other row producer), or keep the existing drain/memory/profitability gates for wider subtrees. Add a negative plan-shape regression combining mo_current_roles, a table scan, and early-stop consumers, alongside the positive information_schema one-scan assertions.
Unhappy-path audit: materialized source producer/reader cleanup, cancellation wakeups, memory cap, and spill ownership are otherwise sound; the blocker is the newly broadened admission decision, not a source lifecycle leak or hang. Exact-head CI is green, and the focused author tests pass, but they cover only the intended sysview shape and do not exercise this reachable general-SQL counterexample.
XuPeng-SH
left a comment
There was a problem hiding this comment.
Deep re-review completed at exact head 3d3fd4e8a54e9ed7ac568e836c6b2fe7ff25ec40.
The remaining CTE materialization blocker is closed. The exemption now accepts only a single fixed-width INT64 output that is a direct projection chain over a zero-argument, childless mo_current_roles() scan; joins, scans, filters, aggregates, limits/offsets, variable-width outputs, and other amplifying shapes retain the normal full-drain, memory, and profitability guards. The new LIMIT/UNION and SEMI-join negative plan tests cover the previously demonstrated early-stop regression, while all protected information_schema views retain exactly one statement-local role-closure producer.
I also rechecked the higher-risk surfaces: complete cycle-safe inherited-role closure, live role changes under prepared/plan-cache reuse, sibling metadata views and SCHEMATA authorization boundaries, disconnected role graphs, result ownership/cancellation, protocol-v37 rolling-upgrade gates, compatibility bootstrap definitions, and the grantee index. The focused exact-head planner tests pass locally. I found no remaining correctness, lifecycle, compatibility, or blocking performance issue.
Non-blocking follow-up: retain a real end-to-end metadata-query latency/allocation benchmark in performance coverage; the current structural tests prove one closure execution but the checked-in benchmarks still mock the internal SQL boundary.
XuPeng-SH
left a comment
There was a problem hiding this comment.
Superseding my approval on exact head 3d3fd4e8a54e9ed7ac568e836c6b2fe7ff25ec40 after completing the cross-PR protocol allocation audit.
The implementation findings in the approval remain closed, but MORPCVersion37 is not unique. #27467 currently assigns v37 to independent prepared-parameter StringSource transport across batch/process/group-state boundaries; its current branch established that allocation at 398ed49dd2b83675d8eed2b06bbc3fdfc7125995 before this branch resolved v37 for mo_current_roles. #27553 and #27756 also currently claim the same value.
This is a P1 rolling-upgrade blocker, not merely a future textual conflict. A deployed v37 binary containing StringSource but not mo_current_roles would satisfy this PRs v37 planner/bootstrap gate and permit canonical metadata views that the receiver cannot execute. Preserve explicit merge order: land the first v37 owner, then rebase this PR and allocate the next free version, updating MORPCLatestVersion, planner/bootstrap gates, every upgrade entry, tests, comments, and rolling-upgrade evidence. Do not resolve by retaining a shared v37.
No other correctness, lifecycle, authorization, or performance blocker is reopened.
XuPeng-SH
left a comment
There was a problem hiding this comment.
Deep re-reviewed exact head 6d9f70fad009e255ef0b5b6ac2f0e63919dea843 against base 4661c00ce016cf0d075d5395c73fc02c75307f08.
The latest merge consistently moves this branch from v37 to v38, and the previously reviewed authorization, bounded-frontier, one-scan CTE shape, early-stop exclusion, upgrade fallback, and lifecycle fixes remain materially intact. Two approval blockers remain.
-
[P1 / rolling-upgrade correctness] v38 is already owned by an earlier independent capability branch.
pkg/defines/const.go:76assignsMORPCVersion38tomo_current_roles, and the planner, bootstrap, role-grant index, view upgrades, tests, and compatibility threshold all treat v38 as that cumulative capability. However, open PR #27553 introduced v38 earlier at commit55719a0649583dbe2f3b129ad7f9657b8ff4039c(2026-08-26 16:23 +08:00) for the validated MongoDB explicit-query scan payload, and its current head8f0d09f81dbd775f61c217b2495ccaf978a9d37estill declares that allocation. #27756 currently also claims v38 for an independent DDL fence. A deployed v38 binary from the first branch can therefore satisfy this PRs v38 planner/bootstrap gate while lackingmo_current_roles, allowing canonical metadata views that a participating CN cannot execute. Preserve explicit merge order: land or formally abandon the first v38 owner, then rebase this PR and allocate the next unused cumulative version, updatingMORPCLatestVersion, every producer/consumer gate, upgrade entry, test, comment, and mixed-version evidence together. Do not retain a shared v38. -
[Design gate / security and architecture] The mandatory versioned design review is missing. Excluding tests/BVT/mock support, this change is 603 production-line additions/deletions across bootstrap, frontend, table-function execution, compile, planner, function registration, and sysview packages. It introduces a new table function, a protocol capability and mixed-version bootstrap contract, a special statement-local CTE materialization admission path, an indexed role-closure algorithm with internal SQL, tenant view migration, and a security/tenant visibility boundary. It exceeds the repository default design threshold and independently triggers the protocol, upgrade, lifecycle/performance, and authorization complexity gates. The PR links only the bug report; neither the PR diff nor issue #27656 contains a stable reviewed design artifact. Add and link a versioned design that defines the authorization invariant and trust boundary, role-closure/current-role semantics, statement-local ownership and materialization bounds, internal-SQL cancellation/resource model, protocol allocation plus upgrade/downgrade/rollback behavior, alternatives and rejected tradeoffs, and the security/performance/compatibility validation map. That design must receive a traceable design approval before implementation approval.
Exact-head CI is terminal green and git diff --check passes. Those results cannot prove a unique cross-PR protocol allocation or replace the required design decision record. I did not find a new implementation blocker in the latest main-merge delta beyond the v38 collision; the missing design gate prevents final implementation approval.
What type of PR is this?
Which issue(s) this PR fixes:
issue #27656
Fixes #27656
What this PR does / why we need it:
The affected
information_schemaviews only filtered catalog rows by account, so a role with no privilege on a user table could still inspect its table, column, index, and constraint metadata.This change:
current_role_id()as real-time-related so prepared statements and ordinary plan-cache entries reevaluate the active role instead of retaining a folded privileged role ID;TABLES,COLUMNS,STATISTICS, and both current and legacyTABLE_CONSTRAINTSdefinitions;mo_role_grant, database owner/ID, and table logical-ID dependencies;SHOW CREATEexpectations and adds parser/upgrade unit coverage plus a public SQL privilege matrix covering no privilege, exact inherited table privilege, active-role rename, public-role isolation, databaseSHOW TABLES, CHECK constraints, admin access, and cleanup.Testing
.agents/skills/mo-dev/scripts/mo-cgo-test -v -count=1 -run "^(TestOnlyFullGroupByAllowsNonAggregateHavingOnInformationSchemaView|Test_cte|TestBuildShowDatabasesRestrictsDatabaseSnapshot)$" ./pkg/sql/plan.agents/skills/mo-dev/scripts/mo-cgo-test -count=1 -timeout=600s ./pkg/sql/plan.agents/skills/mo-dev/scripts/mo-cgo-test -count=1 -timeout=600s ./pkg/util/sysview ./pkg/bootstrap/versions/v4_0_6make buildon the final head after merging the latestmo/mainmo-testernormal comparison:dml/show/show.test: 189/189 passedzz_accesscontrol/account_restricted.sql: 125 passed, 1 expected ignored, 0 failedzz_accesscontrol/information_schema_object_visibility.sql: 48/48 passed twice on the same instance, including PREPARE and ordinary plan-cache reuse acrossSET ROLE; database/user/role teardown verified