yuqi1129 commented on PR #13307:
URL: https://github.com/apache/gravitino/pull/13307#issuecomment-5727959397
Thanks for the fix, and for the detailed description — it made the review
much easier. I went through the diff and the surrounding call chain
(`alterTable`/`loadTable` locking, the `TableMetaService.updateTable`
transaction, the three "latest row" SQL paths, and the null handling in tag
listing). The core logic looks right to me for all the scenarios I traced:
chained renames, swap via a temp name, rename + add with the old name, drop +
re-add, rename into a name freed by a same-batch delete, nested fields, and
capability normalization. Nice work on `resolveColumnNameChanges` in particular.
A few thoughts below. Some of them touch `alterTable` locking, which may
well be out of scope for a tag-consistency PR, so before asking for any changes
I'd like to hear how you see the boundary of this PR. Happy to go with whatever
split keeps this one focused.
**1. The load/alter race seems only partially closed (would appreciate your
take on scope)**
If I read the locking correctly, `alterTable` without `RenameTable` takes a
READ tree lock on the table, and `internalLoadTable` also runs under READ
(reading the catalog first, then the store), so the two can interleave. The
re-match under the WRITE lock handles the ordering "load read the catalog after
the alter but the store before it". I think the opposite ordering is still
possible:
load reads catalog (`c1`) → alter completes fully (catalog `c1_new`, store
`c1_new` with the same id) → load reads store (`c1_new`) → mismatch → load
takes the WRITE lock and re-matches the now-stale catalog snapshot (`c1`)
against the fresh entity → writes `c1` with a new id and a DELETE row for
`c1_new`; with this PR, `deleteColumnRelations` then soft-deletes its tags in
the same transaction. The next load would flip it back with yet another id.
Two concurrent `alterTable`s on the same table (e.g. one rename, one add
column) look like they could hit the same thing under READ locks.
One small option would be to take a WRITE lock on the table ident in
`alterTable` when there is no `RenameTable` (it is a leaf node, so it only
blocks operations on that table); then the in-lock re-match becomes a safety
net rather than the fix. That said, I can see the argument for keeping locking
changes out of this PR and tracking them under #13303. If we defer it, maybe
the PR description and the comment in `updateColumnsIfNecessaryWhenLoad` could
say the window is narrowed rather than closed. Which way would you prefer?
**2. Rename matching may depend on `HashMap` iteration order when the store
is stale (I think this one fits here, but let me know)**
In `updateColumnsIfNecessary`, `columnsFromTableEntity` is a `HashMap`, and
the first stored column to claim a catalog name wins via
`matchedCatalogColumns`. Consider a store that still holds a column `b` that
was dropped outside Gravitino (no load since), and a user running
`renameColumn(a -> b)`, so `nameChanges = {a: b}`. If the stale `b` happens to
be visited first, it matches the catalog's `b` (which is really the renamed
`a`) and keeps its old id; `a` then finds `b` already taken, is treated as
dropped, and its tags are soft-deleted. Visiting `a` first gives the right
result.
A two-pass match might make this deterministic: first the entries that have
a mapping in `nameChanges`, then the untouched ones (or exclude
`nameChanges.values()` from the untouched pass). A test that drops `b` directly
via `TestCatalogOperations` and then renames `a -> b` through the dispatcher
would cover it. Admittedly this needs a stale store to trigger, so it is an
edge case — your call on whether it is worth handling now.
**3. Owner branch in `deleteColumnRelations` (nit)**
As far as I can tell, `OwnerManager` / `SecurableObjects` don't support
`MetadataObject.Type.COLUMN`, so there shouldn't be any column owner relations
today. It might be slightly clearer to either drop the owner branch and note in
the comment that only tags can currently be attached to columns, or keep it and
mention it is forward-looking. Either is fine with me.
**4. Load path now deletes relations permanently (just a design note,
nothing to change here)**
Previously an external rename, or a catalog returning `columns() == null`
(which `updateColumnsIfNecessary` treats as "all columns dropped"), left
dangling relation rows; now the same load soft-deletes them. I believe this is
functionally equivalent since the old id was already unreachable, and the
description already covers external renames. Just flagging that a read
operation now has an irreversible write side effect, in case we later want a
guard for the empty-column-set case.
**Tests that might be worth adding**, depending on the scope we settle on:
the stale-catalog-snapshot ordering from (1) (it would fail today, so it could
drive the lock change if we go that way); a case-insensitive catalog renaming
to a mixed-case name; and a catalog that ignores the rename (the returned table
still has the old name) to check that it falls back to drop + add without
errors.
One last thing: `TestTableColumnMetaService` /
`TestOrphanedMetadataObjectRelationService` run on H2 by default, so it would
be good to confirm CI exercises the new SQL on MySQL and PostgreSQL as well.
--
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]
