yuqi1129 commented on code in PR #12350:
URL: https://github.com/apache/gravitino/pull/12350#discussion_r3766497923
##########
core/src/main/java/org/apache/gravitino/storage/relational/service/SchemaMetaService.java:
##########
@@ -227,20 +234,23 @@ public <E extends Entity & HasIdentifier> SchemaEntity
updateSchema(
NameIdentifierUtil.ofSchema(metalakeName, catalogName,
oldSchemaEntity.name()).toString();
boolean isRenamed = !Objects.equals(oldSchemaEntity.name(),
newEntity.name());
- AtomicInteger updateResult = new AtomicInteger(0);
try {
SessionUtils.doMultipleWithCommit(
- () ->
- updateResult.set(
- SessionUtils.getWithoutCommit(
- SchemaMetaMapper.class,
- mapper ->
- ops.updatePO(
- mapper,
-
POConverters.updateSchemaPOWithVersion(oldSchemaPO, newEntity),
- oldSchemaPO))),
() -> {
- if (isRenamed && updateResult.get() > 0) {
+ int updated =
+ SessionUtils.getWithoutCommit(
+ SchemaMetaMapper.class,
+ mapper ->
+ ops.updatePO(
+ mapper,
+
POConverters.updateSchemaPOWithVersion(oldSchemaPO, newEntity),
+ oldSchemaPO));
Review Comment:
The observation is accurate — this CAS locks and validates the schema row
only, so neither a catalog nor a metalake rename is fenced here. This is
intentional.
Rename in Gravitino is identity-preserving: it changes only the name column
of the renamed row, and the ids this request already resolved
(`metalakeId`/`catalogId`/`schemaId`) keep denoting the same entities. So the
write lands on exactly the entity the caller resolved, just one that is now
reachable under a different path — nothing is orphaned and no update is lost.
The anomaly this PR closes is the other one: the parent is *deleted*, or
replaced by a same-named entity with a fresh id, and a stale child write
survives it. That case is caught by the id + existence checks these fences
already do.
Making a stale fully qualified path fail would require shared locks on the
whole ancestor chain, root-to-leaf, on every write in the hierarchy — a
metalake row read on every catalog/schema/table/model write. That is a much
larger change than this PR, and it buys strictness rather than integrity, so
I'd rather track it separately if we decide we want strict path semantics.
##########
core/src/main/java/org/apache/gravitino/storage/relational/service/SchemaMetaService.java:
##########
@@ -492,13 +470,151 @@ private List<SchemaPO> listSchemaPOs(Namespace
namespace) {
mapper -> POStorageReadRouting.listPOs(mapper, namespace, ops,
Entity.EntityType.SCHEMA));
}
+ private void lockCatalogForSchemaCreate(
+ CatalogPO observedCatalogPO, boolean createsImplicitAncestors) {
+ CatalogPO currentCatalogPO =
+ SessionUtils.getWithoutCommit(
+ CatalogMetaMapper.class,
Review Comment:
Correct — the create fence revalidates the catalog row, and a metalake
rename leaves that row and its `metalakeId` untouched. This is intentional.
Rename in Gravitino is identity-preserving: it changes only the name column
of the renamed row, and the ids this request already resolved
(`metalakeId`/`catalogId`/`schemaId`) keep denoting the same entities. So the
write lands on exactly the entity the caller resolved, just one that is now
reachable under a different path — nothing is orphaned and no update is lost.
The anomaly this PR closes is the other one: the parent is *deleted*, or
replaced by a same-named entity with a fresh id, and a stale child write
survives it. That case is caught by the id + existence checks these fences
already do.
Making a stale fully qualified path fail would require shared locks on the
whole ancestor chain, root-to-leaf, on every write in the hierarchy — a
metalake row read on every catalog/schema/table/model write. That is a much
larger change than this PR, and it buys strictness rather than integrity, so
I'd rather track it separately if we decide we want strict path semantics.
##########
core/src/main/java/org/apache/gravitino/storage/relational/service/SchemaMetaService.java:
##########
@@ -492,13 +470,151 @@ private List<SchemaPO> listSchemaPOs(Namespace
namespace) {
mapper -> POStorageReadRouting.listPOs(mapper, namespace, ops,
Entity.EntityType.SCHEMA));
}
+ private void lockCatalogForSchemaCreate(
+ CatalogPO observedCatalogPO, boolean createsImplicitAncestors) {
+ CatalogPO currentCatalogPO =
+ SessionUtils.getWithoutCommit(
+ CatalogMetaMapper.class,
+ mapper ->
+ createsImplicitAncestors
+ ?
mapper.selectCatalogMetaByIdForUpdate(observedCatalogPO.getCatalogId())
+ :
mapper.selectCatalogMetaByIdForShare(observedCatalogPO.getCatalogId()));
+ if (currentCatalogPO == null
+ || !Objects.equals(currentCatalogPO.getCatalogName(),
observedCatalogPO.getCatalogName())
+ || !Objects.equals(currentCatalogPO.getMetalakeId(),
observedCatalogPO.getMetalakeId())) {
+ throw new NoSuchEntityException(
+ NoSuchEntityException.NO_SUCH_ENTITY_MESSAGE,
+ Entity.EntityType.CATALOG.name().toLowerCase(),
+ observedCatalogPO.getCatalogName());
+ }
+ }
+
+ private void lockCatalogForSchemaDelete(NameIdentifier identifier, SchemaPO
observedSchemaPO) {
+ CatalogPO currentCatalogPO =
+ SessionUtils.getWithoutCommit(
+ CatalogMetaMapper.class,
+ mapper ->
mapper.selectCatalogMetaByIdForUpdate(observedSchemaPO.getCatalogId()));
Review Comment:
Correct — the delete fence takes the catalog row and validates its name and
`metalakeId`, neither of which changes when the metalake is renamed. This is
intentional.
Rename in Gravitino is identity-preserving: it changes only the name column
of the renamed row, and the ids this request already resolved
(`metalakeId`/`catalogId`/`schemaId`) keep denoting the same entities. So the
write lands on exactly the entity the caller resolved, just one that is now
reachable under a different path — nothing is orphaned and no update is lost.
The anomaly this PR closes is the other one: the parent is *deleted*, or
replaced by a same-named entity with a fresh id, and a stale child write
survives it. That case is caught by the id + existence checks these fences
already do.
Making a stale fully qualified path fail would require shared locks on the
whole ancestor chain, root-to-leaf, on every write in the hierarchy — a
metalake row read on every catalog/schema/table/model write. That is a much
larger change than this PR, and it buys strictness rather than integrity, so
I'd rather track it separately if we decide we want strict path semantics.
##########
core/src/main/java/org/apache/gravitino/storage/relational/service/CatalogMetaService.java:
##########
@@ -226,41 +240,37 @@ public <E extends Entity & HasIdentifier> CatalogEntity
updateCatalog(
String oldFullName =
NameIdentifierUtil.ofCatalog(metalakeName,
oldCatalogEntity.name()).toString();
- AtomicInteger updateResult = new AtomicInteger(0);
try {
SessionUtils.doMultipleWithCommit(
- () ->
- updateResult.set(
- SessionUtils.getWithoutCommit(
- CatalogMetaMapper.class,
- mapper ->
- mapper.updateCatalogMeta(
- POConverters.updateCatalogPOWithVersion(
- oldCatalogPO, newEntity,
oldCatalogPO.getMetalakeId()),
- oldCatalogPO))),
() -> {
- if (updateResult.get() > 0) {
+ int updated =
+ SessionUtils.getWithoutCommit(
+ CatalogMetaMapper.class,
+ mapper ->
+ mapper.updateCatalogMeta(
+ POConverters.updateCatalogPOWithVersion(
+ oldCatalogPO, newEntity,
oldCatalogPO.getMetalakeId()),
+ oldCatalogPO));
Review Comment:
Correct — this CAS validates the catalog row and its version only, and a
metalake rename changes neither. This is intentional.
Rename in Gravitino is identity-preserving: it changes only the name column
of the renamed row, and the ids this request already resolved
(`metalakeId`/`catalogId`/`schemaId`) keep denoting the same entities. So the
write lands on exactly the entity the caller resolved, just one that is now
reachable under a different path — nothing is orphaned and no update is lost.
The anomaly this PR closes is the other one: the parent is *deleted*, or
replaced by a same-named entity with a fresh id, and a stale child write
survives it. That case is caught by the id + existence checks these fences
already do.
Making a stale fully qualified path fail would require shared locks on the
whole ancestor chain, root-to-leaf, on every write in the hierarchy — a
metalake row read on every catalog/schema/table/model write. That is a much
larger change than this PR, and it buys strictness rather than integrity, so
I'd rather track it separately if we decide we want strict path semantics.
##########
core/src/main/java/org/apache/gravitino/storage/relational/service/CatalogMetaService.java:
##########
@@ -415,6 +420,72 @@ public boolean deleteCatalog(NameIdentifier identifier,
boolean cascade) {
return true;
}
+ private void deleteCatalogWithVersion(NameIdentifier identifier, CatalogPO
observedCatalogPO) {
+ int deleted =
+ SessionUtils.getWithoutCommit(
+ CatalogMetaMapper.class,
+ mapper ->
+ mapper.softDeleteCatalogMetasByCatalogId(
+ observedCatalogPO.getCatalogId(),
observedCatalogPO.getCurrentVersion()));
Review Comment:
Correct — the catalog delete CAS checks the catalog row and its version,
which a metalake rename leaves unchanged. This is intentional.
Rename in Gravitino is identity-preserving: it changes only the name column
of the renamed row, and the ids this request already resolved
(`metalakeId`/`catalogId`/`schemaId`) keep denoting the same entities. So the
write lands on exactly the entity the caller resolved, just one that is now
reachable under a different path — nothing is orphaned and no update is lost.
The anomaly this PR closes is the other one: the parent is *deleted*, or
replaced by a same-named entity with a fresh id, and a stale child write
survives it. That case is caught by the id + existence checks these fences
already do.
Making a stale fully qualified path fail would require shared locks on the
whole ancestor chain, root-to-leaf, on every write in the hierarchy — a
metalake row read on every catalog/schema/table/model write. That is a much
larger change than this PR, and it buys strictness rather than integrity, so
I'd rather track it separately if we decide we want strict path semantics.
##########
core/src/main/java/org/apache/gravitino/storage/relational/service/SchemaMetaService.java:
##########
@@ -492,13 +470,151 @@ private List<SchemaPO> listSchemaPOs(Namespace
namespace) {
mapper -> POStorageReadRouting.listPOs(mapper, namespace, ops,
Entity.EntityType.SCHEMA));
}
+ private void lockCatalogForSchemaCreate(
+ CatalogPO observedCatalogPO, boolean createsImplicitAncestors) {
+ CatalogPO currentCatalogPO =
+ SessionUtils.getWithoutCommit(
+ CatalogMetaMapper.class,
+ mapper ->
+ createsImplicitAncestors
+ ?
mapper.selectCatalogMetaByIdForUpdate(observedCatalogPO.getCatalogId())
+ :
mapper.selectCatalogMetaByIdForShare(observedCatalogPO.getCatalogId()));
+ if (currentCatalogPO == null
+ || !Objects.equals(currentCatalogPO.getCatalogName(),
observedCatalogPO.getCatalogName())
+ || !Objects.equals(currentCatalogPO.getMetalakeId(),
observedCatalogPO.getMetalakeId())) {
+ throw new NoSuchEntityException(
+ NoSuchEntityException.NO_SUCH_ENTITY_MESSAGE,
+ Entity.EntityType.CATALOG.name().toLowerCase(),
+ observedCatalogPO.getCatalogName());
+ }
+ }
+
+ private void lockCatalogForSchemaDelete(NameIdentifier identifier, SchemaPO
observedSchemaPO) {
+ CatalogPO currentCatalogPO =
+ SessionUtils.getWithoutCommit(
+ CatalogMetaMapper.class,
+ mapper ->
mapper.selectCatalogMetaByIdForUpdate(observedSchemaPO.getCatalogId()));
+ if (currentCatalogPO == null
+ || !Objects.equals(currentCatalogPO.getCatalogName(),
identifier.namespace().level(1))
+ || !Objects.equals(currentCatalogPO.getMetalakeId(),
observedSchemaPO.getMetalakeId())) {
+ throw new NoSuchEntityException(
+ NoSuchEntityException.NO_SUCH_ENTITY_MESSAGE,
+ Entity.EntityType.CATALOG.name().toLowerCase(),
+ identifier.namespace().level(1));
+ }
+ }
+
+ void lockSchemaForEntityWrite(
+ NameIdentifier entityIdentifier,
+ Long observedSchemaId,
+ Long observedCatalogId,
+ Long observedMetalakeId) {
Review Comment:
The premise is right: `lockSchemaForEntityWrite` is taken by the child
*inserts* and by the cross-schema table move, not by same-schema updates. I
looked into each path you named and confirmed the concrete defects:
- `TableMetaService.updateTable` writes the new `table_version_info` row
outside the `updateResult > 0` guard, so a lost CAS still commits an active
version row. Since the legacy cleanup only removes rows with `deleted_at > 0`,
that row is never collected.
- `FunctionMetaService.updateFunction` discards the meta update result
entirely, so a concurrent delete leaves an active orphan version row *and*
returns success — a silent lost update.
- `insertModelVersion` takes no schema fence at all.
All three are pre-existing on `main` and sit outside this PR's scope
(metalake/catalog/schema OCC), and this PR is already large, so I'm going to
fix them in a follow-up issue rather than grow the diff here.
I don't plan to take the broader suggestion of acquiring the shared schema
lock at the start of every child write transaction. Same-schema updates are
already mutually exclusive with a cascade delete through the per-row CAS
(`current_version` + `deleted_at = 0`) — the defects above are about what the
losing transaction commits, not about missing exclusion. Guarding the losing
path (and adding the missing fence to `insertModelVersion`) is the smaller and
more targeted fix.
--
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]
