jerryshao commented on code in PR #13006:
URL: https://github.com/apache/gravitino/pull/13006#discussion_r3965685705
##########
core/src/main/java/org/apache/gravitino/storage/relational/service/UserMetaService.java:
##########
@@ -260,6 +263,13 @@ public <E extends Entity & HasIdentifier> UserEntity
updateUser(
// metadata-only change, such as the audit info, still has to be written.
try {
SessionUtils.doMultipleWithCommit(
+ () -> {
+ if (!insertRoleIds.isEmpty()) {
Review Comment:
Making the fence conditional leaves the revoke / metadata-only path in the
same order inversion that an unconditional fence would close. Raising this as a
question rather than a change request, since it is pre-existing and there is a
genuine trade-off.
The metalake cascade soft-deletes in the order `user_role_rel` ->
`user_meta` -> `group_role_rel` -> `group_meta` -> `role_meta`
(`MetalakeMetaService.java:256-275`). `updateUser` writes the other way round:
`UPDATE user_meta` (`:273-283`), then `user_role_rel` (`:298-307` insert,
`:308-317` soft-delete).
When `insertRoleIds` is non-empty the two cannot interleave, because this
fence holds `metalake_meta` shared while the cascade needs it exclusively. That
is exactly what the fence buys, and on the grant path it works.
For a pure revoke, though, `insertRoleIds` is empty, so this guard skips the
fence entirely and `lockRolesForMembership` below degrades to a no-op loop.
That leaves:
- T1 `revokeRolesFromUser(M, [R], X)` takes the exclusive row lock on
`user_meta` X.
- T2 `deleteMetalake(M, cascade)` holds `metalake_meta` M exclusively and
takes the `user_role_rel` locks for M. Its `WHERE ... IN (SELECT ...)` subquery
is a non-locking MVCC read on PostgreSQL and H2, so it does not block on T1.
- T1 then blocks soft-deleting `user_role_rel` (X,R), and T2 blocks updating
`user_meta` X. Deadlock; the engine aborts one side.
`GroupMetaService.java:309-316` has the identical shape.
To be clear on scope: this is **not a regression**. I checked `c5b9affa41` —
base `updateUser` took no metalake lock at all and used the same write order,
so this PR strictly narrows the window rather than opening it. There is no
correctness hole either, since `updateUserMeta` carries `AND deleted_at = 0`
and a revoke racing the cascade is idempotent. The cost is purely a deadlock
abort.
Dropping the `insertRoleIds` guard would close it, at the price of more
serialization on H2, where both this fence and the new
`selectRoleMetaByIdForShare` degrade to `FOR UPDATE`. Either answer looks
defensible to me — I would just like the decision to be deliberate rather than
incidental, and a short comment here recording the reasoning would help the
next reader.
Related minor point: the new tests only exercise the grant path
(`testMetalakeCascadeWaitsForGrant`). A revoke or metadata-only update racing
the cascade is not covered either way.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
