Repository navigation
[4.2-dev] fix: preserve view authorization across index joins and replacement - #30820
Conversation
…lacement Backport #30799 for #30785 and #30786. Adapt authorization propagation to OriginViews/DirectView and preserve logical view identity using the existing transactional catalog path. Source: @TrentShen.
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
XuPeng-SH
left a comment
There was a problem hiding this comment.
Request changes: repeated CREATE OR REPLACE VIEW in one transaction still invalidates existing grants.
Reviewed head 553e7b3dc564b6de5043a62e822d5debe1d5e16f against 26d3a200cb13f4be3b5f64d489bac75d8dcc5814, including all seven changed files and the surrounding planner authorization, catalog identity, locking and rollback owners. The index JOIN authorization changes follow the source scan correctly and introduce no unnecessary execution path or grants storage.
Validation: verified exact-head CI passes for pkg/sql/compile and all eight pkg/sql/plan race shards, SCA (0 issues), and grant_view_non_sys.sql (55/55). Independent real CN/TN and authenticated MySQL tenant probes reproduce the identity/grant failure below with privilege cache both on and off; rollback controls pass. The compiler stub regression only captures the Create context and cannot detect the engine's stale cached TableDef.
A separate warm-plan/view-definition issue also reproduces on the clean base and is excluded from the blocking finding. The unrelated TN CI race is not the reason for this request-changes decision.
| oldLogicalID = oldDef.GetLogicalId() | ||
| if oldLogicalID == 0 { | ||
| oldLogicalID = oldRelation.GetTableID(c.proc.Ctx) |
There was a problem hiding this comment.
[P2] Preserve identity across repeated replacements in the same transaction
This reads the cached TableDef as the identity authority, but txnDatabase.createWithID calls tbl.GetTableDef(ctx) before assigning tbl.logicalId (txn_database.go:495,509). The newly created relation therefore retains a cached TableDef with LogicalId=0. On a second replacement in the same transaction, this fallback substitutes the first replacement's new physical ID instead of retaining the original logical ID.
Reproduced on this head through real CN/TN and authenticated tenant MySQL connections: grant SELECT on a view, then BEGIN; CREATE OR REPLACE VIEW ...; CREATE OR REPLACE VIEW ...; COMMIT. A fresh view-only reader fails with error 20101, with privilege cache both on and off. The engine probe records logical IDs 16385 -> 16385 -> 16386; rollback controls retain the old definition and access.
Please initialize/synchronize logical identity in the engine metadata owner before exposing its TableDef, and add a real transaction regression covering repeated replacement and commit/rollback. A compiler-local identity cache or another fallback would duplicate the existing metadata responsibility.
There was a problem hiding this comment.
Fixed in 551de35.
createWithID now resolves and initializes the effective logical ID before GetTableDef materializes its cache. The existing hidden-index exclusion and logical-ID index update behavior are preserved.
Added an engine regression that fails on the old cached identity, plus real tenant SQL coverage for repeated replacement and commit/rollback. Before the fix, authenticated readers fail with 20101 with privilege cache both ON and OFF; the same probes pass after the fix. The extended authorization BVT passes twice (65/65 each), including grant-option retention and fresh reader checks.
Validation: complete disttae/compile/plan suites, focused engine race test, existing ALTER/RelationExists engine integration controls, build, vet and incremental lint pass. Related view BVT: 113/113. Requested re-review through the GitHub review-request API.
…itions Resolve logical identity at the engine metadata owner before GetTableDef materializes its cache. Keep identity and view grants stable across repeated replacements in one transaction, and cover commit/rollback through real tenant SQL regressions. Addresses PR #30820 review feedback.
What type of PR is this?
Which issue(s) this PR fixes:
Related to #30785, #30786.
Backport of #30799 to
4.2-dev.What this PR does / why we need it:
View-only SELECT can fail with error 20101 when an index JOIN rewrite loses the original scan's view authorization. CREATE OR REPLACE VIEW also changes the view's logical identity and leaves existing grants attached to the old object.
OriginViews,DirectView, snapshot and binding context into synthetic index JOIN scans, using the authorization representation in4.2-dev.TableDef, matching the relevant prerequisite already present in main (feat(plan): add bounded read-only view schema requests #29633). Repeated replacements in the same transaction now retain the original identity through commit and rollback; hidden index tables retain their own identities.Source: @TrentShen (MatrixOne Cloud production report).
Validation on
4.2-devwith Go 1.26.4 and locally built native dependencies:pkg/vm/engine/disttae,pkg/sql/compileandpkg/sql/plansuites pass. The new engine regression also passes with-race -tags matrixone_test -short -parallel=2. Existing engine integration controlsTestAlterTableBasicandTestRelationExistspass.make build, changed-packagego vet, and incremental golangci-lint v2.6.2 pass (0 lint issues).grant_view_non_sys.sqlpasses twice on the same instance (65/65 each), with account cleanup and global privilege-cache restoration checked after each run. Relatedalter_view.sqlandreplace_view.testpass (113/113 total).