zhoujinsong commented on code in PR #4283:
URL: https://github.com/apache/amoro/pull/4283#discussion_r3628650231
##########
amoro-ams/src/main/java/org/apache/amoro/server/catalog/InternalCatalog.java:
##########
@@ -56,6 +59,12 @@ public List<String> listDatabases() {
}
public void createDatabase(String databaseName) {
+ createDatabase(databaseName, Collections.emptyMap());
+ }
+
+ public void createDatabase(String databaseName, Map<String, String>
properties) {
+ Map<String, String> databaseProperties =
+ properties == null ? Collections.emptyMap() : new
HashMap<>(properties);
if (!databaseExists(databaseName)) {
doAsTransaction(
Review Comment:
**Correction**: my earlier claim that `doAsTransaction` + `doAs` breaks the
lock was incorrect -- `NestedSqlSession` supports nesting, so all
`doAs`/`getAs` calls within a `doAsTransaction` share the same session and
transaction. The pessimistic lock *is* held across the full read-then-write
sequence.
The real concern is that the current `lockDatabase` approach uses a dummy
`UPDATE SET table_count = table_count` to acquire a row lock, then issues a
separate `SELECT` to read the data. This works but is a hack -- it requires two
DB round-trips and its intent is unclear.
A cleaner alternative is `SELECT ... FOR UPDATE`, which combines lock + read
in a single statement:
```java
Map<String, String> previousProperties = new HashMap<>();
doAsTransaction(
() -> {
DatabaseMetadata metadata =
getAs(
TableMetaMapper.class,
mapper -> mapper.selectDatabaseMetadataForUpdate(name(),
databaseName));
if (metadata == null) {
throw new ObjectNotExistsException(getDatabaseDesc(databaseName));
}
Map<String, String> properties =
metadata.getProperties() == null
? new HashMap<>()
: new HashMap<>(metadata.getProperties());
previousProperties.putAll(properties);
requestedRemovals.forEach(properties::remove);
properties.putAll(requestedUpdates);
doAsExisted(
TableMetaMapper.class,
mapper -> mapper.updateDatabaseProperties(name(), databaseName,
properties),
() -> new ObjectNotExistsException(getDatabaseDesc(databaseName)));
});
return previousProperties;
```
Mapper addition:
```java
@Select("SELECT catalog_name, db_name, properties FROM database_metadata "
+ "WHERE catalog_name = #{catalogName} AND db_name = #{dbName} FOR
UPDATE")
@Results({
@Result(property = "catalogName", column = "catalog_name"),
@Result(property = "databaseName", column = "db_name"),
@Result(property = "properties", column = "properties", typeHandler =
Map2StringConverter.class)
})
DatabaseMetadata selectDatabaseMetadataForUpdate(
@Param("catalogName") String catalogName, @Param("dbName") String
dbName);
```
Benefits: semantic clarity (FOR UPDATE signals intent to modify), one DB
round-trip instead of two, eliminates the dummy UPDATE hack.
--
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]
