This is an automated email from the ASF dual-hosted git repository.
gavinchou pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/doris.git
The following commit(s) were added to refs/heads/master by this push:
new 249f4f5cfd1 [fix](storage) check partition storage policy resource
before BE update (#65053)
249f4f5cfd1 is described below
commit 249f4f5cfd1fcaea183b41d7993ba6e038b7c65b
Author: Gavin Chou <[email protected]>
AuthorDate: Thu Jul 2 18:06:08 2026 +0800
[fix](storage) check partition storage policy resource before BE update
(#65053)
### What problem does this PR solve?
When `ALTER TABLE ... MODIFY PARTITION ... SET ("storage_policy" = ...)`
changes an existing partition policy to another policy backed by a
different storage resource, Doris should reject the operation.
Before this patch, the BE tablet meta update was sent before FE checked
the old/new policy resource compatibility. The SQL could fail later in
FE, while BE tablet meta had already persisted the new
`storage_policy_id`.
### Check List
- [x] Add a pre-check before updating BE tablet meta.
- [x] Keep same-resource policy switching allowed.
- [x] Add regression coverage for `MODIFY PARTITION (*)` cross-resource
failure.
---
.../main/java/org/apache/doris/alter/Alter.java | 57 +++++++++-
.../alter/ModifyPartitionStoragePolicyTest.java | 116 +++++++++++++++++++++
.../modify_partition_add_policy.groovy | 63 +++++++++++
3 files changed, 235 insertions(+), 1 deletion(-)
diff --git a/fe/fe-core/src/main/java/org/apache/doris/alter/Alter.java
b/fe/fe-core/src/main/java/org/apache/doris/alter/Alter.java
index 1825170b320..9f9eb03ceec 100644
--- a/fe/fe-core/src/main/java/org/apache/doris/alter/Alter.java
+++ b/fe/fe-core/src/main/java/org/apache/doris/alter/Alter.java
@@ -94,6 +94,8 @@ import org.apache.doris.persist.ModifyPartitionInfo;
import org.apache.doris.persist.ModifyTableEngineOperationLog;
import org.apache.doris.persist.ModifyTablePropertyOperationLog;
import org.apache.doris.persist.ReplaceTableOperationLog;
+import org.apache.doris.policy.Policy;
+import org.apache.doris.policy.PolicyTypeEnum;
import org.apache.doris.policy.StoragePolicy;
import org.apache.doris.qe.ConnectContext;
import org.apache.doris.qe.ConnectContextUtil;
@@ -647,9 +649,11 @@ public class Alter {
ModifyPartitionOp clause = ((ModifyPartitionOp) alterOp);
Map<String, String> properties = clause.getProperties();
List<String> partitionNames = clause.getPartitionNames();
+ OlapTable olapTable = (OlapTable) tableIf;
+ checkModifyPartitionStoragePolicyResource(
+ olapTable, partitionNames, properties,
clause.isTempPartition());
((SchemaChangeHandler)
schemaChangeHandler).updatePartitionsProperties(
db, tableName, partitionNames, properties);
- OlapTable olapTable = (OlapTable) tableIf;
olapTable.writeLockOrDdlException();
try {
modifyPartitionsProperty(db, olapTable, partitionNames,
properties, clause.isTempPartition());
@@ -916,6 +920,57 @@ public class Alter {
}
}
+ private void checkModifyPartitionStoragePolicyResource(OlapTable
olapTable, List<String> partitionNames,
+ Map<String, String> properties, boolean isTempPartition) throws
DdlException, AnalysisException {
+ if (!PropertyAnalyzer.hasStoragePolicy(properties)) {
+ return;
+ }
+
+ String newStoragePolicy =
PropertyAnalyzer.analyzeStoragePolicy(properties);
+ if (Strings.isNullOrEmpty(newStoragePolicy)) {
+ return;
+ }
+
Env.getCurrentEnv().getPolicyMgr().checkStoragePolicyExist(newStoragePolicy);
+
+ Optional<Policy> newPolicy = Env.getCurrentEnv().getPolicyMgr()
+ .findPolicy(newStoragePolicy, PolicyTypeEnum.STORAGE);
+ if (!newPolicy.isPresent() || !(newPolicy.get() instanceof
StoragePolicy)) {
+ return;
+ }
+
+ olapTable.readLock();
+ try {
+ PartitionInfo partitionInfo = olapTable.getPartitionInfo();
+ for (String partitionName : partitionNames) {
+ Partition partition = olapTable.getPartition(partitionName,
isTempPartition);
+ if (partition == null) {
+ continue;
+ }
+
+ DataProperty dataProperty =
partitionInfo.getDataProperty(partition.getId());
+ String oldStoragePolicy = dataProperty.getStoragePolicy();
+ if (Strings.isNullOrEmpty(oldStoragePolicy) ||
oldStoragePolicy.equals(newStoragePolicy)) {
+ continue;
+ }
+
+ Optional<Policy> oldPolicy = Env.getCurrentEnv().getPolicyMgr()
+ .findPolicy(oldStoragePolicy, PolicyTypeEnum.STORAGE);
+ if (!oldPolicy.isPresent() || !(oldPolicy.get() instanceof
StoragePolicy)) {
+ continue;
+ }
+
+ String newResource = ((StoragePolicy)
newPolicy.get()).getStorageResource();
+ String oldResource = ((StoragePolicy)
oldPolicy.get()).getStorageResource();
+ if (!newResource.equals(oldResource)) {
+ throw new AnalysisException("currently do not support
change origin "
+ + "storage policy to another one with different
resource: ");
+ }
+ }
+ } finally {
+ olapTable.readUnlock();
+ }
+ }
+
/**
* Batch update partitions' properties
* caller should hold the table lock
diff --git
a/fe/fe-core/src/test/java/org/apache/doris/alter/ModifyPartitionStoragePolicyTest.java
b/fe/fe-core/src/test/java/org/apache/doris/alter/ModifyPartitionStoragePolicyTest.java
new file mode 100644
index 00000000000..37d7fadd99e
--- /dev/null
+++
b/fe/fe-core/src/test/java/org/apache/doris/alter/ModifyPartitionStoragePolicyTest.java
@@ -0,0 +1,116 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package org.apache.doris.alter;
+
+import org.apache.doris.catalog.Env;
+import org.apache.doris.common.Config;
+import org.apache.doris.common.ExceptionChecker;
+import org.apache.doris.common.FeConstants;
+import org.apache.doris.common.jmockit.Deencapsulation;
+import org.apache.doris.utframe.TestWithFeService;
+
+import org.junit.jupiter.api.Test;
+import org.mockito.Mockito;
+
+public class ModifyPartitionStoragePolicyTest extends TestWithFeService {
+
+ @Override
+ protected void beforeCluster() {
+ Config.enable_storage_policy = true;
+ FeConstants.runningUnitTest = true;
+ }
+
+ @Override
+ protected void runBeforeAll() throws Exception {
+ createDatabase("test_cir_19357");
+ useDatabase("test_cir_19357");
+
+ executeSql("CREATE RESOURCE IF NOT EXISTS \"cir_19357_resource_a\" "
+ + "PROPERTIES("
+ + "\"type\"=\"s3\","
+ + "\"AWS_REGION\"=\"bj\","
+ + "\"AWS_ENDPOINT\"=\"bj.s3.comaaaa\","
+ + "\"AWS_ROOT_PATH\"=\"path/to/rootaaaa\","
+ + "\"AWS_SECRET_KEY\"=\"aaaa\","
+ + "\"AWS_ACCESS_KEY\"=\"bbba\","
+ + "\"AWS_BUCKET\"=\"test-bucket-a\","
+ + "\"s3_validity_check\"=\"false\")");
+ executeSql("CREATE RESOURCE IF NOT EXISTS \"cir_19357_resource_b\" "
+ + "PROPERTIES("
+ + "\"type\"=\"s3\","
+ + "\"AWS_REGION\"=\"bj\","
+ + "\"AWS_ENDPOINT\"=\"bj.s3.comaaaa\","
+ + "\"AWS_ROOT_PATH\"=\"path/to/rootaaaa\","
+ + "\"AWS_SECRET_KEY\"=\"aaaa\","
+ + "\"AWS_ACCESS_KEY\"=\"bbba\","
+ + "\"AWS_BUCKET\"=\"test-bucket-b\","
+ + "\"s3_validity_check\"=\"false\")");
+ executeSql("CREATE STORAGE POLICY IF NOT EXISTS cir_19357_policy_a "
+ + "PROPERTIES("
+ + "\"storage_resource\"=\"cir_19357_resource_a\","
+ + "\"cooldown_datetime\"=\"2999-01-01 00:00:00\")");
+ executeSql("CREATE STORAGE POLICY IF NOT EXISTS cir_19357_policy_b "
+ + "PROPERTIES("
+ + "\"storage_resource\"=\"cir_19357_resource_b\","
+ + "\"cooldown_datetime\"=\"2999-01-01 00:00:00\")");
+
+ createTable("CREATE TABLE tbl_with_policy (k1 int, k2 int) "
+ + "PARTITION BY RANGE(k1) ("
+ + "PARTITION p1 VALUES LESS THAN (\"10\"),"
+ + "PARTITION p2 VALUES LESS THAN (\"20\")) "
+ + "DISTRIBUTED BY HASH(k1) BUCKETS 1 "
+ + "PROPERTIES(\"replication_num\"=\"1\",
\"storage_policy\"=\"cir_19357_policy_a\")");
+ createTable("CREATE TABLE tbl_without_policy (k1 int, k2 int) "
+ + "PARTITION BY RANGE(k1) ("
+ + "PARTITION p1 VALUES LESS THAN (\"10\"),"
+ + "PARTITION p2 VALUES LESS THAN (\"20\")) "
+ + "DISTRIBUTED BY HASH(k1) BUCKETS 1 "
+ + "PROPERTIES(\"replication_num\"=\"1\")");
+ }
+
+ @Test
+ public void testRejectDifferentResourcePolicyBeforeBeUpdate() throws
Exception {
+ assertModifyPartitionRejectedBeforeBeUpdate(
+ "ALTER TABLE tbl_with_policy MODIFY PARTITION (*) "
+ + "SET (\"storage_policy\"=\"cir_19357_policy_b\")",
+ "currently do not support change origin storage policy to
another one with different resource");
+ }
+
+ @Test
+ public void testRejectUninitializedDefaultPolicyBeforeBeUpdate() throws
Exception {
+ assertModifyPartitionRejectedBeforeBeUpdate(
+ "ALTER TABLE tbl_without_policy MODIFY PARTITION (*) "
+ + "SET
(\"storage_policy\"=\"default_storage_policy\")",
+ "Use default storage policy, but not give s3 info");
+ }
+
+ private void assertModifyPartitionRejectedBeforeBeUpdate(String sql,
String errorMessage) throws Exception {
+ Alter alter = Env.getCurrentEnv().getAlterInstance();
+ SchemaChangeHandler originHandler = (SchemaChangeHandler)
alter.getSchemaChangeHandler();
+ SchemaChangeHandler spyHandler = Mockito.spy(originHandler);
+ Deencapsulation.setField(alter, "schemaChangeHandler", spyHandler);
+ try {
+ ExceptionChecker.expectThrowsWithMsg(IllegalStateException.class,
errorMessage,
+ () -> executeSql(sql));
+ Mockito.verify(spyHandler,
Mockito.never()).updatePartitionsProperties(
+ Mockito.any(), Mockito.anyString(), Mockito.anyList(),
Mockito.anyMap());
+ } finally {
+ Deencapsulation.setField(alter, "schemaChangeHandler",
originHandler);
+ }
+ }
+}
diff --git
a/regression-test/suites/cold_heat_separation/empty_table_use_policy/modify_partition_add_policy.groovy
b/regression-test/suites/cold_heat_separation/empty_table_use_policy/modify_partition_add_policy.groovy
index 9608c296836..bff574b0473 100644
---
a/regression-test/suites/cold_heat_separation/empty_table_use_policy/modify_partition_add_policy.groovy
+++
b/regression-test/suites/cold_heat_separation/empty_table_use_policy/modify_partition_add_policy.groovy
@@ -112,6 +112,63 @@ suite("add_table_policy_by_modify_partition") {
"cooldown_datetime" = "$cooldownTime"
);
"""
+ sql """ALTER STORAGE POLICY tmp2 PROPERTIES("cooldown_datetime" =
"$cooldownTime")"""
+
+ try_sql """
+ CREATE RESOURCE IF NOT EXISTS
"test_modify_partition_table_use_resource_diff"
+ PROPERTIES(
+ "type"="s3",
+ "AWS_REGION" = "bj",
+ "AWS_ENDPOINT" = "bj.s3.comaaaa",
+ "AWS_ROOT_PATH" = "path/to/rootaaaa",
+ "AWS_SECRET_KEY" = "aaaa",
+ "AWS_ACCESS_KEY" = "bbba",
+ "AWS_BUCKET" = "test-bucket-diff",
+ "s3_validity_check" = "false"
+ );
+ """
+ try_sql """
+ CREATE STORAGE POLICY IF NOT EXISTS
created_create_table_partition_alter_policy_diff
+ PROPERTIES(
+ "storage_resource" = "test_modify_partition_table_use_resource_diff",
+ "cooldown_datetime" = "$cooldownTime"
+ );
+ """
+ sql """ALTER STORAGE POLICY
created_create_table_partition_alter_policy_diff PROPERTIES("cooldown_datetime"
= "$cooldownTime")"""
+
assertEquals(storage_exist.call("created_create_table_partition_alter_policy_diff"),
true)
+
+ def partitionsAfterSetPolicy = sql_return_maparray """
+ show partitions from create_table_partition
+ """
+ for (def par in partitionsAfterSetPolicy) {
+ assertTrue(par.RemoteStoragePolicy ==
"created_create_table_partition_alter_policy")
+ }
+
+ test {
+ sql """
+ ALTER TABLE create_table_partition MODIFY PARTITION (*)
SET("storage_policy"="created_create_table_partition_alter_policy_diff");
+ """
+ exception "currently do not support change origin storage policy to
another one with different resource"
+ }
+
+ def partitionsAfterFailedAlter = sql_return_maparray """
+ show partitions from create_table_partition
+ """
+ for (def par in partitionsAfterFailedAlter) {
+ assertTrue(par.RemoteStoragePolicy ==
"created_create_table_partition_alter_policy")
+ }
+
+ def alter_table_partition_same_resource_result = try_sql """
+ ALTER TABLE create_table_partition MODIFY PARTITION (*)
SET("storage_policy"="tmp2");
+ """
+ assertEquals(alter_table_partition_same_resource_result.size(), 1);
+
+ def partitionsAfterSameResourceAlter = sql_return_maparray """
+ show partitions from create_table_partition
+ """
+ for (def par in partitionsAfterSameResourceAlter) {
+ assertTrue(par.RemoteStoragePolicy == "tmp2")
+ }
sql """
CREATE TABLE create_table_partion_use_created_policy_test
@@ -148,9 +205,15 @@ suite("add_table_policy_by_modify_partition") {
DROP STORAGE POLICY created_create_table_partition_alter_policy
"""
sql """
+ DROP STORAGE POLICY created_create_table_partition_alter_policy_diff
+ """
+ sql """
DROP STORAGE POLICY tmp2
"""
sql """
DROP RESOURCE test_modify_partition_table_use_resource
"""
+ sql """
+ DROP RESOURCE test_modify_partition_table_use_resource_diff
+ """
}
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
