jerryshao commented on code in PR #12823:
URL: https://github.com/apache/gravitino/pull/12823#discussion_r3947074699
##########
core/src/main/java/org/apache/gravitino/storage/relational/service/FunctionMetaService.java:
##########
@@ -333,15 +313,171 @@ private void fillFunctionPOBuilderParentEntityId(
builder.withSchemaId(namespacedEntityId.entityId());
}
- private FunctionPO updateFunctionPO(FunctionPO oldFunctionPO, FunctionEntity
newFunction) {
- Integer newVersion = oldFunctionPO.functionLatestVersion() + 1;
+ private FunctionPO updateFunctionPO(
+ FunctionPO oldFunctionPO,
+ FunctionEntity newFunction,
+ Long newSchemaId,
+ Long newCatalogId,
+ Long newMetalakeId) {
+ // Both version columns always advance together, but deriving the next
version from
+ // the higher of the two keeps it above every version row that exists,
even if a
+ // future path ever leaves the current version behind the latest one.
+ Integer newVersion = nextVersion(oldFunctionPO);
FunctionPO.FunctionPOBuilder builder =
FunctionPO.builder()
- .withMetalakeId(oldFunctionPO.metalakeId())
- .withCatalogId(oldFunctionPO.catalogId())
- .withSchemaId(oldFunctionPO.schemaId())
+ .withMetalakeId(newMetalakeId)
+ .withCatalogId(newCatalogId)
+ .withSchemaId(newSchemaId)
.withFunctionLatestVersion(newVersion)
.withFunctionCurrentVersion(newVersion);
return buildFunctionPO(newFunction, builder, newVersion);
}
+
+ /**
+ * Writes a new function or replaces the active function selected by natural
key or stable ID.
+ * Locking the root before choosing the next version makes overwrite
behavior identical across
+ * databases and keeps standard reads strict about the matching version-row
invariant.
+ */
+ private void insertFunctionWithoutCommit(
+ FunctionEntity functionEntity, FunctionPO initializedFunctionPO, boolean
overwrite) {
+ if (!overwrite) {
+ insertNewFunctionWithoutCommit(initializedFunctionPO);
+ return;
+ }
+
+ FunctionPO existingFunctionPO =
findAndLockFunctionForOverwrite(initializedFunctionPO);
+ if (existingFunctionPO == null) {
+ insertNewFunctionWithoutCommit(initializedFunctionPO);
+ return;
+ }
+
+ FunctionPO replacementPO = functionPOForOverwrite(initializedFunctionPO,
existingFunctionPO);
+ int updated =
+ SessionUtils.getWithoutCommit(
+ FunctionMetaMapper.class,
+ mapper -> ops.updatePO(mapper, replacementPO, existingFunctionPO));
+ if (updated == 0) {
+ throw functionWriteFailure(functionEntity.nameIdentifier(),
existingFunctionPO);
+ }
+ SessionUtils.doWithoutCommit(
+ FunctionVersionMetaMapper.class,
+ mapper ->
mapper.insertFunctionVersionMeta(replacementPO.functionVersionPO()));
+ }
+
+ private void insertNewFunctionWithoutCommit(FunctionPO functionPO) {
+ SessionUtils.doWithoutCommit(
+ FunctionMetaMapper.class, mapper -> ops.insertPO(mapper, functionPO,
false));
+ SessionUtils.doWithoutCommit(
+ FunctionVersionMetaMapper.class,
+ mapper ->
mapper.insertFunctionVersionMeta(functionPO.functionVersionPO()));
+ }
+
+ private FunctionPO findAndLockFunctionForOverwrite(FunctionPO
initializedFunctionPO) {
Review Comment:
This locking read doesn't actually serialize the case where the function
doesn't exist yet.
A `SELECT ... FOR UPDATE` that matches **zero rows** takes no lock at all -
this is standard RDBMS behavior: PostgreSQL only gap/predicate-locks under
`SERIALIZABLE`, and MySQL/InnoDB's gap locking for a non-matching scan needs
`REPEATABLE READ`. `SqlSessions.getSqlSession()` opens every session at
`READ_COMMITTED`, which rules out even MySQL's usual mitigation here.
So two concurrent `insertFunction(f, overwrite=true)` calls for a function
`f` that doesn't exist yet can both see zero rows from
`selectFunctionMetaBySchemaIdAndNameForUpdate`, both conclude
`existingFunctionPO == null`, and both fall through to
`insertNewFunctionWithoutCommit`'s plain `INSERT`. The loser then collides with
the `(schema_id, function_name, deleted_at)` unique constraint and gets
`EntityAlreadyExistsException` - not the retryable `OptimisticLockException`
the rest of this OCC design produces - instead of the graceful overwrite the
caller asked for.
The old `INSERT ... ON DUPLICATE KEY UPDATE` / `ON CONFLICT ... DO UPDATE`
handled this exact race atomically in one statement. None of the new
concurrency tests (`testNaturalKeyOverwriteWaitsForConcurrentRename`, etc.)
cover "row doesn't exist yet, two overwrites race" - they only race an
overwrite against something happening to an *already-existing* row.
--
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]
