Skip to content

[#12342] improvement(core): add OCC for namespace hierarchy - #12350

Closed
yuqi1129 wants to merge 12 commits into
apache:mainfrom
yuqi1129:feat/12342-occ-namespace-hierarchy
Closed

[#12342] improvement(core): add OCC for namespace hierarchy#12350
yuqi1129 wants to merge 12 commits into
apache:mainfrom
yuqi1129:feat/12342-occ-namespace-hierarchy

Conversation

@yuqi1129

@yuqi1129 yuqi1129 commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

Add database-backed optimistic concurrency control and transaction boundaries for metalakes, catalogs, and schemas.

  • Advance the OCC version for every alter and classify a failed CAS as either a stale conflict or a missing entity.
  • Protect catalog and flat managed-schema creation with shared parent-row locks on MySQL and PostgreSQL, without changing the parent version. H2 uses an exclusive lock because it has no shared row-lock syntax.
  • Serialize hierarchical schema ancestor materialization and schema drops through the catalog row so overlapping cascades have a deterministic lock order.
  • Keep the root CAS, non-empty checks or cascade cleanup, and change-log writes in one database transaction.
  • Lock catalog parent rows before a metalake cascade snapshot, then CAS-delete descendant schemas and catalogs with their observed identifier-and-version pairs.
  • Make managed schema creation insert-only so concurrent creates cannot silently overwrite metadata.
  • Check views and functions before a non-cascade schema drop.

Accepted tradeoff: a hierarchical schema create that materializes implicit ancestors takes an exclusive lock on the catalog row, so every other schema create under that catalog waits until that transaction ends, even when it touches a different ancestor path. The exclusive lock is needed because two concurrent creates can both find the same ancestor missing and both insert it, and a shared lock does not prevent that under MySQL REPEATABLE READ. Catalogs with heavy concurrent hierarchical schema creation will therefore serialize on this lock. If it becomes a bottleneck, a narrower fence — locking only the ancestor rows being created and relying on the unique constraint plus a retry — can be done in a follow-up.

This PR builds on the shared conflict response introduced by #12349.

Why are the changes needed?

Managed namespace operations previously consisted of multiple independent reads and writes. Concurrent alter, create, and delete requests could overwrite newer metadata, create children below a deleted parent, leave view/function rows orphaned, or run partial cascade cleanup. Overlapping hierarchical schema drops could also acquire descendant row locks in different orders.

Fix: #12342

Does this PR introduce any user-facing change?

Concurrent managed namespace version conflicts are reported as HTTP 409. If the observed entity was deleted or renamed away, alter reports not found and drop preserves its idempotent false result. A managed schema create that loses a concurrent same-name create returns SchemaAlreadyExistsException instead of overwriting the winner.

How was this patch tested?

  • ./gradlew :core:check -PskipITs -PskipDockerTests=true
  • env dockerTest=true ./gradlew :core:test --tests 'org.apache.gravitino.storage.relational.service.TestSchemaMetaService' --tests 'org.apache.gravitino.storage.relational.service.TestCatalogMetaService' --tests 'org.apache.gravitino.storage.relational.service.TestMetalakeMetaService' -PskipITs -PskipDockerTests=false (H2, MySQL 8.0, PostgreSQL 13)
  • ./gradlew :catalogs:catalog-model:test --tests 'org.apache.gravtitino.catalog.model.TestModelCatalogOperations' -PskipITs -PskipDockerTests=true
  • ./gradlew :core:javadoc

@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown

Code Coverage Report

Overall Project 68.7% +0.52% 🟢
Files changed 82.48% 🟢

Module Coverage
aliyun 1.72% 🔴
api 49.31% 🟢
authorization-common 85.96% 🟢
aws 42.04% 🟢
azure 2.47% 🔴
catalog-common 9.92% 🔴
catalog-fileset 79.77% 🟢
catalog-glue 68.95% 🟢
catalog-hive 79.4% 🟢
catalog-jdbc-common 45.7% 🟢
catalog-jdbc-doris 81.8% 🟢
catalog-jdbc-mysql 79.33% 🟢
catalog-jdbc-postgresql 83.39% 🟢
catalog-jdbc-starrocks 79.16% 🟢
catalog-kafka 76.72% 🟢
catalog-lakehouse-generic 59.18% 🟢
catalog-lakehouse-hudi 79.1% 🟢
catalog-lakehouse-iceberg 85.86% 🟢
catalog-lakehouse-paimon 84.23% 🟢
catalog-model 77.72% 🟢
cli 44.48% 🟢
client-java 78.46% 🟢
common 52.75% 🟢
core 83.64% -0.15% 🟢
filesystem-hadoop3 77.28% 🟢
flink 0.0% 🔴
flink-common 48.68% 🟢
flink-runtime 0.0% 🔴
gcp 14.12% 🔴
hadoop-auth 68.0% 🟢
hadoop-common 12.7% 🔴
hive-metastore-common 53.4% 🟢
iceberg-aliyun-bundle 0.0% 🔴
iceberg-common 64.75% 🟢
iceberg-rest-server 75.03% 🟢
idp-basic 86.02% 🟢
integration-test-common 0.0% 🔴
jobs 62.92% 🟢
lance-common 31.75% 🔴
lance-rest-server 63.47% 🟢
lineage 53.02% 🟢
optimizer 83.24% 🟢
optimizer-api 21.95% 🔴
server 87.25% 🟢
server-common 79.41% 🟢
spark 28.57% 🔴
spark-common 45.89% 🟢
tencent 69.84% 🟢
trino-connector 40.29% 🟢
Files
Module File Coverage
core MetalakeMetaSQLProviderFactory.java 100.0% 🟢
MetalakeMetaBaseSQLProvider.java 100.0% 🟢
FunctionMetaService.java 100.0% 🟢
TableMetaService.java 100.0% 🟢
ViewMetaService.java 100.0% 🟢
MetalakeMetaService.java 99.32% 🟢
SchemaMetaService.java 97.59% 🟢
CatalogMetaService.java 94.35% 🟢
FilesetMetaService.java 91.53% 🟢
CatalogMetaSQLProviderFactory.java 90.32% 🟢
CatalogMetaBaseSQLProvider.java 90.0% 🟢
ExceptionUtils.java 88.89% 🟢
POConverters.java 88.31% 🟢
CatalogMetaPostgreSQLProvider.java 85.71% 🟢
MetalakeMetaPostgreSQLProvider.java 83.33% 🟢
ModelMetaService.java 76.72% 🟢
SchemaMetaSQLProviderFactory.java 75.0% 🟢
TopicMetaService.java 72.48% 🟢
CatalogManager.java 70.56% 🟢
SchemaMetaBaseSQLProvider.java 68.0% 🟢
SchemaMetaPostgreSQLProvider.java 66.67% 🟢
MetalakeManager.java 65.88% 🟢
ManagedSchemaOperations.java 29.2% 🔴
CatalogMetaMapper.java 0.0% 🔴
MetalakeMetaMapper.java 0.0% 🔴
SchemaMetaMapper.java 0.0% 🔴

@yuqi1129
yuqi1129 force-pushed the feat/12342-occ-namespace-hierarchy branch from 7955cfc to 472a791 Compare August 4, 2026 08:54
@yuqi1129 yuqi1129 self-assigned this Aug 4, 2026
@yuqi1129
yuqi1129 requested a lite review from Copilot August 4, 2026 08:56
@yuqi1129
yuqi1129 marked this pull request as ready for review August 4, 2026 08:56

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Not ready to approve

Service-level tests are missing for some new OptimisticLockException conflict paths (notably catalog/schema alter conflicts and metalake stale delete), leaving the new OCC contract partially unverified end-to-end.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Pull request overview

This PR introduces optimistic concurrency control (OCC) for alter and delete operations across the namespace hierarchy (metalake, catalog, schema) in the relational metadata store, using version CAS (compare-and-swap) and reporting stale operations via OptimisticLockException.

Changes:

  • Increment current_version/last_version on every successful alter and enforce CAS updates via WHERE ... current_version = ?.
  • Enforce CAS deletes of the root entity (by id + expected current version) before running cascade cleanup; schemas CAS-delete the requested schema then authoritatively delete descendants.
  • Add/extend tests to validate version increments and stale update/delete row-count behavior.
File summaries
File Description
core/src/main/java/org/apache/gravitino/storage/relational/utils/POConverters.java Always increments entity versions on alter PO conversions.
core/src/main/java/org/apache/gravitino/storage/relational/service/MetalakeMetaService.java Throws OptimisticLockException on stale alter/delete; CAS-delete metalake before cascade cleanup.
core/src/main/java/org/apache/gravitino/storage/relational/service/CatalogMetaService.java Throws OptimisticLockException on stale alter/delete; CAS-delete catalog before cascade cleanup.
core/src/main/java/org/apache/gravitino/storage/relational/service/SchemaMetaService.java Throws OptimisticLockException on stale alter/delete; CAS-delete requested schema before descendant cleanup.
core/src/main/java/org/apache/gravitino/storage/relational/mapper/MetalakeMetaSQLProviderFactory.java Updates provider factory to pass expected version for metalake soft deletes.
core/src/main/java/org/apache/gravitino/storage/relational/mapper/MetalakeMetaMapper.java Updates mapper signature to soft-delete metalake by id + expected version.
core/src/main/java/org/apache/gravitino/storage/relational/mapper/provider/base/MetalakeMetaBaseSQLProvider.java Narrows update CAS predicate to current_version and adds version predicate to soft delete.
core/src/main/java/org/apache/gravitino/storage/relational/mapper/provider/postgresql/MetalakeMetaPostgreSQLProvider.java PostgreSQL-specific CAS soft delete by version and narrowed update predicate.
core/src/main/java/org/apache/gravitino/storage/relational/mapper/CatalogMetaSQLProviderFactory.java Updates provider factory to pass expected version for catalog soft deletes.
core/src/main/java/org/apache/gravitino/storage/relational/mapper/CatalogMetaMapper.java Updates mapper signature to soft-delete catalog by id + expected version.
core/src/main/java/org/apache/gravitino/storage/relational/mapper/provider/base/CatalogMetaBaseSQLProvider.java Narrows update CAS predicate to current_version and adds version predicate to soft delete.
core/src/main/java/org/apache/gravitino/storage/relational/mapper/provider/postgresql/CatalogMetaPostgreSQLProvider.java PostgreSQL-specific CAS soft delete by version and narrowed update predicate.
core/src/main/java/org/apache/gravitino/storage/relational/mapper/SchemaMetaSQLProviderFactory.java Adds provider factory method for schema soft delete by id + expected version.
core/src/main/java/org/apache/gravitino/storage/relational/mapper/SchemaMetaMapper.java Adds mapper method for schema soft delete by id + expected version.
core/src/main/java/org/apache/gravitino/storage/relational/mapper/provider/base/SchemaMetaBaseSQLProvider.java Narrows schema update CAS predicate to current_version and adds versioned soft delete SQL.
core/src/main/java/org/apache/gravitino/storage/relational/mapper/provider/postgresql/SchemaMetaPostgreSQLProvider.java PostgreSQL-specific schema soft delete by id + expected version and narrowed update predicate.
core/src/test/java/org/apache/gravitino/storage/relational/utils/TestPOConverters.java Asserts version increments on PO update conversion.
core/src/test/java/org/apache/gravitino/storage/relational/service/TestMetalakeMetaService.java Adds mapper-level stale update/delete tests and a metalake alter conflict test for OptimisticLockException.
core/src/test/java/org/apache/gravitino/storage/relational/service/TestCatalogMetaService.java Adds mapper-level stale update/delete tests.
core/src/test/java/org/apache/gravitino/storage/relational/service/TestSchemaMetaService.java Adds mapper-level stale update/delete tests (including versioned schema delete).
Review details
  • Files reviewed: 20/20 changed files
  • Comments generated: 4
  • Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

@yuqi1129
yuqi1129 requested a review from jerryshao August 5, 2026 09:33
@yuqi1129

yuqi1129 commented Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

@roryqi

Thanks for the suggestion. I understand it as extracting a common OCC SQL template for update/fence/soft-delete operations, with each provider supplying the table name, ID column, and MyBatis parameter names. After reviewing these statements, I think a generic SQL template would have limited benefit here. Besides the table and ID column, the MyBatis parameter paths, batch collection names, entity-specific SET clauses, and database-specific deleted_at expressions also differ. Passing these as string parameters would hide the final SQL, reduce readability, and move some errors to runtime. I would prefer to keep the entity-specific SQL explicit.

I agree that exact duplication should be removed. A narrower approach would be to reuse DatabaseTimeSQL for database-time expressions and let PostgreSQL providers inherit SQL that is identical to the base provider, keeping overrides only where the SQL dialect actually differs. If more entities later converge on an identical OCC SQL shape, we can revisit a narrowly scoped helper. Would this narrower cleanup address your concern, or do you have a specific interface/template design in mind?

@roryqi

roryqi commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

@roryqi

Thanks for the suggestion. I understand it as extracting a common OCC SQL template for update/fence/soft-delete operations, with each provider supplying the table name, ID column, and MyBatis parameter names. After reviewing these statements, I think a generic SQL template would have limited benefit here. Besides the table and ID column, the MyBatis parameter paths, batch collection names, entity-specific SET clauses, and database-specific deleted_at expressions also differ. Passing these as string parameters would hide the final SQL, reduce readability, and move some errors to runtime. I would prefer to keep the entity-specific SQL explicit.

I agree that exact duplication should be removed. A narrower approach would be to reuse DatabaseTimeSQL for database-time expressions and let PostgreSQL providers inherit SQL that is identical to the base provider, keeping overrides only where the SQL dialect actually differs. If more entities later converge on an identical OCC SQL shape, we can revisit a narrowly scoped helper. Would this narrower cleanup address your concern, or do you have a specific interface/template design in mind?

I prefer think further. We can give more constraints about table create SQL standard. Some common column must has the fixed name. Some services must have some specific interfaces.

For example, we have delete_at column in every table, we have a delete interface. Every storage service should implement it.

I think this is possible. We would better have a framework to handle this issue. We should think more about how to iterate our storage framework. Now, we are using AI to generate the code. It seems not bring much burden.
But it is still hard for people to review code and maintain our code base.

@yuqi1129

yuqi1129 commented Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

@roryqi
Thanks for the suggestion. I understand it as extracting a common OCC SQL template for update/fence/soft-delete operations, with each provider supplying the table name, ID column, and MyBatis parameter names. After reviewing these statements, I think a generic SQL template would have limited benefit here. Besides the table and ID column, the MyBatis parameter paths, batch collection names, entity-specific SET clauses, and database-specific deleted_at expressions also differ. Passing these as string parameters would hide the final SQL, reduce readability, and move some errors to runtime. I would prefer to keep the entity-specific SQL explicit.
I agree that exact duplication should be removed. A narrower approach would be to reuse DatabaseTimeSQL for database-time expressions and let PostgreSQL providers inherit SQL that is identical to the base provider, keeping overrides only where the SQL dialect actually differs. If more entities later converge on an identical OCC SQL shape, we can revisit a narrowly scoped helper. Would this narrower cleanup address your concern, or do you have a specific interface/template design in mind?

I prefer think further. We can give more constraints about table create SQL standard. Some common column must has the fixed name. Some services must have some specific interfaces.

For example, we have delete_at column in every table, we have a delete interface. Every storage service should implement it.

I think this is possible. We would better have a framework to handle this issue. We should think more about how to iterate our storage framework. Now, we are using AI to generate the code. It seems not bring much burden. But it is still hard for people to review code and maintain our code base.

@roryqi

Thanks for the suggestion. I understand it as extracting a common OCC SQL template for update/fence/soft-delete operations, with each provider supplying the table name, ID column, and MyBatis parameter names. After reviewing these statements, I think a generic SQL template would have limited benefit here. Besides the table and ID column, the MyBatis parameter paths, batch collection names, entity-specific SET clauses, and database-specific deleted_at expressions also differ. Passing these as string parameters would hide the final SQL, reduce readability, and move some errors to runtime. I would prefer to keep the entity-specific SQL explicit.

I agree that exact duplication should be removed. A narrower approach would be to reuse DatabaseTimeSQL for database-time expressions and let PostgreSQL providers inherit SQL that is identical to the base provider, keeping overrides only where the SQL dialect actually differs. If more entities later converge on an identical OCC SQL shape, we can revisit a narrowly scoped helper. Would this narrower cleanup address your concern, or do you have a specific interface/template design in mind?

I prefer think further. We can give more constraints about table create SQL standard. Some common column must has the fixed name. Some services must have some specific interfaces.

For example, we have delete_at column in every table, we have a delete interface. Every storage service should implement it.

I think this is possible. We would better have a framework to handle this issue. We should think more about how to iterate our storage framework. Now, we are using AI to generate the code. It seems not bring much burden.
But it is still hard for people to review code and maintain our code base.

I suggest we use another PR to discuss the problem and leave your ideas in as much detail as possible. AI hinted to me that

  • It's not so proper to do it in this PR
  • The final return is unclear.

If you insist on this point, please just create an issue that is targeted at this problem. I think we can do a pure refactor without this PR.

@yuqi1129
yuqi1129 requested a review from roryqi August 5, 2026 13:29

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 33 out of 33 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

core/src/test/java/org/apache/gravitino/storage/relational/service/TestSchemaMetaService.java:198

  • This concurrency test calls the lock helper directly, so it still passes if a production entity write forgets to acquire the fence—as the uncovered function/table/model-version update paths currently do. Exercise an actual child service operation (for example insertTable) while the schema delete is paused, and add stale-update/version-write cases so the transactional wiring, rollback, and orphan prevention are verified end to end.

Comment on lines +507 to +511
void lockSchemaForEntityWrite(
NameIdentifier entityIdentifier,
Long observedSchemaId,
Long observedCatalogId,
Long observedMetalakeId) {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for catching this. This is a valid cross-instance race, but it predates this PR: the existing TreeLock only serializes schema and child operations within one process, and the relational storage layer has never had a complete schema fence for child update and version-write paths.

A complete fix requires auditing table, view, fileset, topic, function, model, and model-version writes; locking both schemas for table moves; failing zero-row child updates inside the transaction; and adding deterministic concurrency coverage for H2, MySQL, and PostgreSQL. That is a substantial change beyond #12342, which is scoped to OCC for the metalake/catalog/schema namespace hierarchy.

I filed #12406 to track the complete schema-to-child write protocol and will address it in a follow-up PR instead of expanding this already large PR.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@yuqi1129

Copy link
Copy Markdown
Contributor Author

@roryqi @jerryshao
Do you have time to take a look again?

.build();
try {
store().put(schemaEntity, true /* overwrite */);
store().put(schemaEntity, false /* overwrite */);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the purpose of changing this to false?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The interface is an explicit create operation, and we need to change it to false to avoid concurrent insertion of data into the database.

Comment on lines +484 to +487
private OptimisticLockException optimisticLockException(NameIdentifier identifier) {
return new OptimisticLockException(
"The catalog %s was modified concurrently; retry the operation", identifier);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can define this method in the helper method.

SessionUtils.getWithoutCommit(
SchemaMetaMapper.class, mapper -> mapper.softDeleteSchemaMetasWithVersion(schemaPOs));
if (deleted != schemaPOs.size()) {
throw new OptimisticLockException(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is better to call a helper method if defined, rather than calling a class constructuor.

Comment on lines +490 to +493
private OptimisticLockException optimisticLockException(NameIdentifier identifier) {
return new OptimisticLockException(
"The metalake %s was modified concurrently; retry the operation", identifier);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should fix lots of duplications.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strictly, it's not duplicated code and is only used for the metalake. Anyway, we need to refine it.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 33 out of 33 changed files in this pull request and generated 5 comments.

Comment on lines +240 to +247
int updated =
SessionUtils.getWithoutCommit(
SchemaMetaMapper.class,
mapper ->
ops.updatePO(
mapper,
POConverters.updateSchemaPOWithVersion(oldSchemaPO, newEntity),
oldSchemaPO));

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one and L477, L496, and CatalogMetaService:253/429 are the same problem.

The observation is accurate: these fences validate the immediate parent row by id + name, and an ancestor rename does not change that row.

This is intentional. In Gravitino, entity identity is the ID, and rename preserves identity — the request resolved metalakeId/catalogId, and after the rename those ids still denote the same entity, so the write lands on exactly the entity the caller resolved, just reachable under a new name. Nothing is orphaned and no update is lost. The anomaly this PR is closing is the different one where the parent is deleted (or replaced by a same-named entity with a new id) and a child write would survive it — that case is fenced by the id + existence check.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +492 to +496
private void lockCatalogForSchemaDelete(NameIdentifier identifier, SchemaPO observedSchemaPO) {
CatalogPO currentCatalogPO =
SessionUtils.getWithoutCommit(
CatalogMetaMapper.class,
mapper -> mapper.selectCatalogMetaByIdForUpdate(observedSchemaPO.getCatalogId()));

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +473 to +477
private void lockCatalogForSchemaCreate(
CatalogPO observedCatalogPO, boolean createsImplicitAncestors) {
CatalogPO currentCatalogPO =
SessionUtils.getWithoutCommit(
CatalogMetaMapper.class,

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +246 to +253
int updated =
SessionUtils.getWithoutCommit(
CatalogMetaMapper.class,
mapper ->
mapper.updateCatalogMeta(
POConverters.updateCatalogPOWithVersion(
oldCatalogPO, newEntity, oldCatalogPO.getMetalakeId()),
oldCatalogPO));

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +423 to +429
private void deleteCatalogWithVersion(NameIdentifier identifier, CatalogPO observedCatalogPO) {
int deleted =
SessionUtils.getWithoutCommit(
CatalogMetaMapper.class,
mapper ->
mapper.softDeleteCatalogMetasByCatalogId(
observedCatalogPO.getCatalogId(), observedCatalogPO.getCurrentVersion()));

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

CatalogMetaMapper.class,
mapper ->
createsImplicitAncestors
? mapper.selectCatalogMetaByIdForUpdate(observedCatalogPO.getCatalogId())

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth flagging as a design tradeoff: any hierarchical schema create (one that materializes implicit ancestors) takes an exclusive lock on the whole catalog row, which will block all other schema creates under that catalog — not just ones touching the same ancestor path — for the duration of the transaction. That's the right call for correctness, but it means catalogs with heavy concurrent hierarchical schema creation will serialize on this lock. Worth calling out explicitly in the PR description as an accepted throughput tradeoff.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right. When a create needs to make implicit ancestors, we take an exclusive lock on the catalog row, so all other schema creates under the same catalog have to wait, even if they use a different path.

We need the exclusive lock here because two concurrent creates can both see that an ancestor is missing and both insert it. Under MySQL REPEATABLE READ, a shared lock is not enough to stop this.

So this is an accepted tradeoff: correctness first, less concurrency for hierarchical creates. I will add this to the PR description.

If it becomes a problem later, we can make the lock smaller: only lock the ancestor rows we really need to create, and use the unique constraint plus a retry instead of locking the whole catalog row. I can do that in a follow-up issue.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right. When a create needs to make implicit ancestors, we take
an exclusive lock on the catalog row, so all other schema creates under
the same catalog have to wait, even if they use a different path.

We need the exclusive lock here because two concurrent creates can both
see that an ancestor is missing and both insert it. Under MySQL
REPEATABLE READ, a shared lock is not enough to stop this.

So this is an accepted tradeoff: correctness first, less concurrency for
hierarchical creates. I will add this to the PR description.

If it becomes a problem later, we can make the lock smaller: only lock
the ancestor rows we really need to create, and use the unique
constraint plus a retry instead of locking the whole catalog row. I can
do that in a follow-up issue.


return store.delete(ident, EntityType.METALAKE, true);
} catch (NoSuchMetalakeException e) {
} catch (NoSuchMetalakeException | NoSuchEntityException e) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CatalogManager.dropCatalog's equivalent new catch block also invalidates catalogCache before returning false (to discard the now-stale cache entry). This branch doesn't do the same for whatever metalake-level cache exists — is that intentional (metalakes aren't cached the same way) or a gap?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First, catalogCache is not an entity cache. It caches CatalogWrapper objects, which hold the live catalog instance and its class loader. A metalake has nothing like this, and MetalakeManager has no cache field
at all. The "preload all metalakes" comment in the constructor talks about the entity cache in the store, not a cache in the manager.

Second, the entity cache in the store is already invalidated for us. RelationalEntityStore.delete() calls cache.invalidate(ident, entityType) in a finally block, so it runs both when the delete succeeds and when
the entity is already gone. So there is no stale entry left for this branch to clean up.

Also, in this method the NoSuchMetalakeException / NoSuchEntityException mostly comes from metalakeInUse() and store.list(), which run before we reach the delete.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is intentional, not a gap. There are two reasons.

First, catalogCache is not an entity cache. It caches CatalogWrapper
objects, which hold the live catalog instance and its class loader. A
metalake has nothing like this, and MetalakeManager has no cache field
at all. The "preload all metalakes" comment in the constructor talks
about the entity cache in the store, not a cache in the manager.

Second, the entity cache in the store is already invalidated for us.
RelationalEntityStore.delete() calls cache.invalidate(ident, entityType)
in a finally block, so it runs both when the delete succeeds and when
the entity is already gone. So there is no stale entry left for this
branch to clean up.

Also, in this method the NoSuchMetalakeException / NoSuchEntityException
mostly comes from metalakeInUse() and store.list(), which run before
we reach the delete.

@jerryshao

Copy link
Copy Markdown
Contributor

I really would suggest you split PRs into small one, so that we can have a better review.

@yuqi1129

Copy link
Copy Markdown
Contributor Author

I really would suggest you split PRs into small one, so that we can have a better review.

I will adopt it for the remaining few PRs. This one is small initially, and getting larger and larger after several round of reviews.

Move the duplicated concurrent-modification messages into two shared
factories on ExceptionUtils and drop the per-service private
optimisticLockException helpers.
…ace-hierarchy

Signed-off-by: yuqi <yuqi@datastrato.com>
@yuqi1129

Copy link
Copy Markdown
Contributor Author

Per review feedback that this PR is too large, it has been split into three smaller PRs, in dependency order:

The three branches stacked together produce a tree identical to this PR's head (git diff between them is empty), so nothing is dropped. Each branch is independently green on :core:test with H2.

Keeping this PR open for the discussion context; it will be closed once the three land.

@yuqi1129

Copy link
Copy Markdown
Contributor Author

Use #12454, #12455, and #12456 to replace this big one.

@yuqi1129

Copy link
Copy Markdown
Contributor Author

Close it temporarily.

@yuqi1129 yuqi1129 closed this Aug 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Subtask] Implement version-CAS OCC for metalake, catalog, and schema

4 participants