jerryshao commented on PR #12824:
URL: https://github.com/apache/gravitino/pull/12824#issuecomment-5567378175
Found a bug while reviewing this - not introduced by this PR, but this PR
makes it newly dangerous.
`ViewMetaBaseSQLProvider.updateViewMeta`'s `SET` clause (unchanged by this
diff, pre-existing on `main`) updates `view_name`, `schema_id`,
`current_version`, `last_version`, `audit_info`, and `deleted_at` - but never
`metalake_id` or `catalog_id`:
```java
public String updateViewMeta(
@Param("newViewMeta") ViewPO newViewPO, @Param("oldViewMeta") ViewPO
oldViewPO) {
return "UPDATE "
+ TABLE_NAME
+ " SET view_name = #{newViewMeta.viewName}, "
+ " schema_id = #{newViewMeta.schemaId}, "
+ " current_version = #{newViewMeta.currentVersion}, "
+ " last_version = #{newViewMeta.lastVersion}, "
+ " audit_info = #{newViewMeta.auditInfo}, "
+ " deleted_at = #{newViewMeta.deletedAt} "
+ " WHERE view_id = #{oldViewMeta.viewId} "
+ " AND current_version = #{oldViewMeta.currentVersion} "
+ " AND deleted_at = 0";
}
```
`ViewPO.buildViewPO` (used by `ViewMetaService.updateViewPO` for every
alter, including cross-catalog moves) correctly resolves fresh
`metalakeId`/`catalogId` from the new namespace into the in-memory `newViewPO`
- the value is computed correctly and then silently dropped on write.
`ViewMetaPostgreSQLProvider` doesn't override this method, so both MySQL/H2 and
PostgreSQL inherit it. For comparison, the sibling
`FunctionMetaBaseSQLProvider.updateFunctionMeta` explicitly includes both
columns.
Why this matters for *this* PR specifically: it newly wires
`softDeleteViewMetasByCatalogId`/`softDeleteViewVersionsByCatalogId` into
`CatalogMetaService.deleteCatalog`. So: move a view from a schema in catalog A
to a schema in catalog B via `updateView` - `schema_id` correctly points into B
afterward, but `catalog_id`/`metalake_id` still read A. Dropping catalog A now
wrongly soft-deletes a view that actually lives in B, and dropping catalog B
(the view's real owner) leaves the row un-cleaned since its stored `catalog_id`
doesn't match - an orphan the legacy-timeline collector can never reach.
This is unrelated to the file's diff (line 195 in the current version isn't
part of a changed hunk, so I can't leave it inline), but seemed worth flagging
here given how directly the new cascade-delete wiring interacts with it.
--
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]
