fix(frontend): keep DDL ownership on matched role#24970
Conversation
ouyuanning
requested review from
XuPeng-SH,
gouhongshen and
heni02
as code owners
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? |
ouyuanning
changed the title
ouyuanning
changed the title
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
Requesting changes for a drop-path regression.
After this PR, DROP TABLE / DROP DATABASE first resolve the current object owner role name and now propagate that lookup error. But DROP ROLE still deletes roles unconditionally, so an object can legitimately keep an owner role ID that no longer exists.
In that state, getRoleNameByIDWithBackgroundExec() returns there is no role id ..., doRevokePrivilegeImplicitly() returns the error, and the DROP aborts before execution. That means deleting the owner role can make the owned table/database undeletable by normal DDL.
Please either treat a missing owner role as a no-op during implicit revoke, or block DROP ROLE while the role still owns objects. I think this needs a regression test too.
|
A more actionable suggestion:
The reason I think the short-term behavior matters is that implicit revoke is cleanup logic, not a correctness prerequisite for DROP itself. Right now a stale owner-role reference becomes a hard failure before execution, which can brick normal DDL on orphan-owned objects. A regression test for this would be very helpful too:
|
|
@XuPeng-SH Addressed this with the short-term safe behavior you suggested. The implicit ownership cleanup is now best-effort when the object owner role row is already gone: object owner lookup returns an empty role name for a missing I also added regression coverage:
Verified locally: For the long-term consistency point, I agree |
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
Looks good to me now. The deleted-owner-role case is handled safely by treating the missing owner-role lookup as non-fatal for implicit revoke, and the added BVT covers the orphan-owner drop path directly.
Merge Queue Status
This pull request spent 1 hour 10 seconds in the queue, including 59 minutes 51 seconds running CI. Required conditions to merge
|
5 participants
What type of PR is this?
Which issue(s) this PR fixes:
issue #24956
What this PR does / why we need it:
This PR fixes DDL ownership handling when privileges are satisfied by secondary roles or inherited roles.
Validation:
go test -mod=mod ./pkg/defines -count=1go test -mod=mod ./pkg/frontend -run 'TestCreateDatabaseOwnerRoleFollowsActiveInheritedRoleRoot|TestCreateTableOwnerRoleFollowsPrivilegeRole|Test_determineUserHasPrivilegeSet' -count=1was blocked locally by missingxxhash.hbefore running tests.