This is an automated email from the ASF dual-hosted git repository.
morningman 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 46e0bd81388 [fix](fe) Reject unknown binlog properties and non-boolean
binlog.enable (#69375)
46e0bd81388 is described below
commit 46e0bd813888309a7faaba3544fe2a98959063f4
Author: Mingyu Chen (Rayner) <[email protected]>
AuthorDate: Thu Oct 22 10:16:43 2026 +0100
[fix](fe) Reject unknown binlog properties and non-boolean binlog.enable
(#69375)
### What problem does this PR solve?
Issue Number: None
Related PR: #69311
Problem Summary:
**In short.** CCR binlog is configured by four properties:
`binlog.enable`, `binlog.ttl_seconds`, `binlog.max_bytes` and
`binlog.max_history_nums`. `PropertyAnalyzer.analyzeBinlogConfig`, which
every DDL uses to check them, ignored any other key under `binlog.` and
read `binlog.enable` with `Boolean.parseBoolean`, so a misspelled key or
a value such as `"yes"` was accepted without a word. The worst case:
`ALTER TABLE t SET ("binlog.enable" = "yes")` succeeds and turns off the
CCR binlog of a table that had it on. This PR rejects both, the way
#69311 already made the `changelog.*` properties strict.
**Background.**
- Every DDL path validates the binlog properties with
`analyzeBinlogConfig`: CREATE DATABASE and ALTER DATABASE
(`CreateDatabaseCommand` / `AlterDatabasePropertiesCommand.validate`),
CREATE TABLE (`InternalCatalog.createOlapTable`) and ALTER TABLE
(`ModifyTablePropertiesOp.validate`, then
`SchemaChangeHandler.updateBinlogConfig`). Database properties are
stored as the user wrote them.
- #69311 split changelog (formerly row binlog) from CCR binlog. Its
`changelog.*` properties reject unknown keys (`Unknown changelog
property: <key>`) and accept only `true` / `false` for
`changelog.enable`; the replaced `binlog.format` and
`binlog.need_historical_value` are rejected in every DDL. The four CCR
keys kept the lenient parsing.
**The problem, and what it cost.** On a cluster running master before
this change:
| Statement | Before |
|---|---|
| `ALTER TABLE t SET ("binlog.enable" = "yes")`, `t` has binlog on |
succeeds; `t`'s binlog is now off |
| `CREATE TABLE ... ("binlog.enable" = "yes")` | the table is created
with binlog off |
| `ALTER TABLE t SET ("binlog.ttl_second" = "3600")` (typo) | `Nothing
is changed. please check your alter stmt.` |
| `CREATE TABLE ... ("binlog.ttl_second" = "3600")` | `Unknown
properties: {binlog.ttl_second=3600}` |
| `CREATE DATABASE d PROPERTIES ("binlog.ttl_second" = "3600")` |
succeeds; the key is persisted |
| `ALTER DATABASE d SET PROPERTIES ("binlog.max_byte" = "1024")` |
succeeds; the key is persisted |
| `ALTER DATABASE d SET PROPERTIES ("binlog.enable" = "1")` | succeeds;
`"1"` is persisted and read as false |
**How this PR fixes it.**
- `analyzeBinlogConfig` rejects any key under `binlog.` other than the
four with `Unknown binlog property: <key>`. The replaced `binlog.format`
and `binlog.need_historical_value` are still checked first, so they keep
their message that names the replacement.
- `binlog.enable` accepts only `true` or `false`, in any case, and
otherwise fails with the existing `Invalid binlog enable value:
<value>`, which the old `try` / `catch` around `Boolean.parseBoolean`
could never reach.
- CREATE TABLE runs this check on the properties as written, where it
only checked the replaced keys before. Its later call to
`analyzeBinlogConfig` sees the effective binlog config (the database
default merged with the table properties) that `createOlapTable` puts
back over them, and by then `"yes"` has already become `"false"`.
- Persisted metadata is not validated again: image load and edit log
replay read binlog properties through
`BinlogConfig.mergeFromProperties`, which is unchanged, so a database
that already stored such a key loads as before. A database's binlog
default reaches CREATE TABLE as a `BinlogConfig` object, not as
properties, so it does not make CREATE TABLE fail either.
**Results.**
| Statement | After |
|---|---|
| `ALTER TABLE t SET ("binlog.enable" = "yes")` | `Invalid binlog enable
value: yes`; `t` keeps its binlog setting |
| `CREATE TABLE ... ("binlog.enable" = "yes")` | `Invalid binlog enable
value: yes` |
| `ALTER TABLE` / `CREATE TABLE` / `CREATE DATABASE` with
`"binlog.ttl_second"` | `Unknown binlog property: binlog.ttl_second` |
| `ALTER DATABASE ... ("binlog.max_byte" = "1024")` | `Unknown binlog
property: binlog.max_byte` |
| `ALTER DATABASE ... ("binlog.enable" = "1")` | `Invalid binlog enable
value: 1` |
| `"binlog.enable" = "TRUE"` / `"False"` | accepted, stored as `true` /
`false` (unchanged) |
---
.../apache/doris/common/util/PropertyAnalyzer.java | 17 ++--
.../apache/doris/datasource/InternalCatalog.java | 3 +-
.../doris/catalog/ChangelogConfigSplitTest.java | 16 +++
.../apache/doris/common/PropertyAnalyzerTest.java | 31 ++++++
.../ddl_p0/test_binlog_property_validation.out | 6 ++
.../ddl_p0/test_binlog_property_validation.groovy | 108 +++++++++++++++++++++
6 files changed, 174 insertions(+), 7 deletions(-)
diff --git
a/fe/fe-core/src/main/java/org/apache/doris/common/util/PropertyAnalyzer.java
b/fe/fe-core/src/main/java/org/apache/doris/common/util/PropertyAnalyzer.java
index 4387caf05f1..746bb1d07a3 100644
---
a/fe/fe-core/src/main/java/org/apache/doris/common/util/PropertyAnalyzer.java
+++
b/fe/fe-core/src/main/java/org/apache/doris/common/util/PropertyAnalyzer.java
@@ -1534,17 +1534,22 @@ public class PropertyAnalyzer {
return null;
}
checkLegacyBinlogProperties(properties);
+ for (String key : properties.keySet()) {
+ if (key.startsWith(PROPERTIES_BINLOG_PREFIX) &&
!key.equals(PROPERTIES_BINLOG_ENABLE)
+ && !key.equals(PROPERTIES_BINLOG_TTL_SECONDS) &&
!key.equals(PROPERTIES_BINLOG_MAX_BYTES)
+ && !key.equals(PROPERTIES_BINLOG_MAX_HISTORY_NUMS)) {
+ throw new AnalysisException("Unknown binlog property: " + key);
+ }
+ }
Map<String, String> binlogConfigMap = Maps.newHashMap();
// check PROPERTIES_BINLOG_ENABLE = "binlog.enable";
if (properties.containsKey(PROPERTIES_BINLOG_ENABLE)) {
- String enable = properties.get(PROPERTIES_BINLOG_ENABLE);
- try {
- binlogConfigMap.put(PROPERTIES_BINLOG_ENABLE,
String.valueOf(Boolean.parseBoolean(enable)));
- properties.remove(PROPERTIES_BINLOG_ENABLE);
- } catch (Exception e) {
+ String enable = properties.remove(PROPERTIES_BINLOG_ENABLE);
+ if (!StringUtils.equalsAnyIgnoreCase(enable, "true", "false")) {
throw new AnalysisException("Invalid binlog enable value: " +
enable);
}
+ binlogConfigMap.put(PROPERTIES_BINLOG_ENABLE,
String.valueOf(Boolean.parseBoolean(enable)));
}
// check PROPERTIES_BINLOG_TTL_SECONDS = "binlog.ttl_seconds";
if (properties.containsKey(PROPERTIES_BINLOG_TTL_SECONDS)) {
@@ -1587,7 +1592,7 @@ public class PropertyAnalyzer {
/**
* Rejects the binlog keys the changelog.* properties replaced, whatever
their value is.
*/
- public static void checkLegacyBinlogProperties(Map<String, String>
properties) throws AnalysisException {
+ private static void checkLegacyBinlogProperties(Map<String, String>
properties) throws AnalysisException {
if (properties.containsKey(PROPERTIES_LEGACY_BINLOG_FORMAT)) {
throw new AnalysisException("Property " +
PROPERTIES_LEGACY_BINLOG_FORMAT
+ " is no longer supported, use \"" +
PROPERTIES_CHANGELOG_ENABLE
diff --git
a/fe/fe-core/src/main/java/org/apache/doris/datasource/InternalCatalog.java
b/fe/fe-core/src/main/java/org/apache/doris/datasource/InternalCatalog.java
index 676ed8dc5cb..1b879531b12 100644
--- a/fe/fe-core/src/main/java/org/apache/doris/datasource/InternalCatalog.java
+++ b/fe/fe-core/src/main/java/org/apache/doris/datasource/InternalCatalog.java
@@ -2495,7 +2495,8 @@ public class InternalCatalog implements
CatalogIf<Database> {
boolean tableHasExist = false;
ChangelogConfig createTableChangelogConfig;
try {
-
PropertyAnalyzer.checkLegacyBinlogProperties(createTableInfo.getProperties());
+ // Check the binlog properties as written: the effective binlog
config is put back over them below.
+
PropertyAnalyzer.analyzeBinlogConfig(Maps.newHashMap(createTableInfo.getProperties()));
createTableChangelogConfig =
db.getChangelogConfigForCreateTable(createTableInfo.getProperties());
} catch (AnalysisException e) {
throw new DdlException(e.getMessage());
diff --git
a/fe/fe-core/src/test/java/org/apache/doris/catalog/ChangelogConfigSplitTest.java
b/fe/fe-core/src/test/java/org/apache/doris/catalog/ChangelogConfigSplitTest.java
index d157a67200b..5a7b2968568 100644
---
a/fe/fe-core/src/test/java/org/apache/doris/catalog/ChangelogConfigSplitTest.java
+++
b/fe/fe-core/src/test/java/org/apache/doris/catalog/ChangelogConfigSplitTest.java
@@ -280,6 +280,22 @@ public class ChangelogConfigSplitTest extends
TestWithFeService {
"Unknown changelog property: changelog.format");
}
+ @Test
+ public void testDdlValidatesBinlogProperties() throws Exception {
+ String dup = "(k INT, v INT) DUPLICATE KEY(k)";
+ assertCreateFails("binlog_invalid_enable", dup,
"'binlog.enable'='yes'", "Invalid binlog enable value: yes");
+ assertCreateFails("binlog_unknown_key", dup,
"'binlog.ttl_second'='10'",
+ "Unknown binlog property: binlog.ttl_second");
+
+ OlapTable table = createDupTable(db, "binlog_alter",
"'binlog.enable'='TRUE'");
+ Assertions.assertTrue(table.getBinlogConfig().isEnableForCCR());
+ assertAlterFails("ALTER TABLE split.binlog_alter SET
('binlog.enable'='yes')",
+ "Invalid binlog enable value: yes");
+ assertAlterFails("ALTER TABLE split.binlog_alter SET
('binlog.ttl_second'='10')",
+ "Unknown binlog property: binlog.ttl_second");
+ Assertions.assertTrue(table.getBinlogConfig().isEnableForCCR());
+ }
+
@Test
public void testDatabaseChangelogDefaults() throws Exception {
createDatabaseWithSql("CREATE DATABASE split_default_db "
diff --git
a/fe/fe-core/src/test/java/org/apache/doris/common/PropertyAnalyzerTest.java
b/fe/fe-core/src/test/java/org/apache/doris/common/PropertyAnalyzerTest.java
index 13304fc5704..9101501f33e 100644
--- a/fe/fe-core/src/test/java/org/apache/doris/common/PropertyAnalyzerTest.java
+++ b/fe/fe-core/src/test/java/org/apache/doris/common/PropertyAnalyzerTest.java
@@ -67,6 +67,37 @@ public class PropertyAnalyzerTest {
() -> PropertyAnalyzer.analyzeBinlogConfig(properties));
}
+ @Test
+ public void testAnalyzeBinlogConfig() throws AnalysisException {
+ Map<String, String> properties = new HashMap<>();
+ properties.put(PropertyAnalyzer.PROPERTIES_BINLOG_ENABLE, "TRUE");
+ properties.put(PropertyAnalyzer.PROPERTIES_BINLOG_TTL_SECONDS, "60");
+ properties.put(PropertyAnalyzer.PROPERTIES_BINLOG_MAX_BYTES, "1024");
+ properties.put(PropertyAnalyzer.PROPERTIES_BINLOG_MAX_HISTORY_NUMS,
"10");
+ properties.put(PropertyAnalyzer.PROPERTIES_CHANGELOG_ENABLE, "false");
+ Map<String, String> analyzed =
PropertyAnalyzer.analyzeBinlogConfig(properties);
+
Assertions.assertEquals(Map.of(PropertyAnalyzer.PROPERTIES_BINLOG_ENABLE,
"true",
+ PropertyAnalyzer.PROPERTIES_BINLOG_TTL_SECONDS, "60",
+ PropertyAnalyzer.PROPERTIES_BINLOG_MAX_BYTES, "1024",
+ PropertyAnalyzer.PROPERTIES_BINLOG_MAX_HISTORY_NUMS, "10"),
analyzed);
+
Assertions.assertEquals(Map.of(PropertyAnalyzer.PROPERTIES_CHANGELOG_ENABLE,
"false"), properties);
+
+ properties.put(PropertyAnalyzer.PROPERTIES_BINLOG_ENABLE, "False");
+ Assertions.assertEquals("false",
PropertyAnalyzer.analyzeBinlogConfig(properties)
+ .get(PropertyAnalyzer.PROPERTIES_BINLOG_ENABLE));
+
+ for (Map.Entry<String, String> invalid : Map.of(
+ PropertyAnalyzer.PROPERTIES_BINLOG_ENABLE, "Invalid binlog
enable value: yes",
+ "binlog.ttl_second", "Unknown binlog property:
binlog.ttl_second",
+ "binlog.compaction", "Unknown binlog property:
binlog.compaction").entrySet()) {
+ Map<String, String> invalidProperties = new HashMap<>();
+ invalidProperties.put(invalid.getKey(), "yes");
+ AnalysisException exception =
Assertions.assertThrows(AnalysisException.class,
+ () ->
PropertyAnalyzer.analyzeBinlogConfig(invalidProperties));
+
Assertions.assertTrue(exception.getMessage().contains(invalid.getValue()),
exception.getMessage());
+ }
+ }
+
@ParameterizedTest
@ValueSource(strings = {"ROW", "row", "STATEMENT_AND_SNAPSHOT",
"statement_and_snapshot", "rows"})
public void testReplacedBinlogKeysAreRejected(String value) {
diff --git a/regression-test/data/ddl_p0/test_binlog_property_validation.out
b/regression-test/data/ddl_p0/test_binlog_property_validation.out
new file mode 100644
index 00000000000..61b5eb70522
--- /dev/null
+++ b/regression-test/data/ddl_p0/test_binlog_property_validation.out
@@ -0,0 +1,6 @@
+-- This file is automatically generated. You should know what you did if you
want to edit this
+-- !table_binlog_enable --
+test_binlog_property_validation_tbl binlog.enable false
+
+-- !database_binlog_properties --
+
diff --git
a/regression-test/suites/ddl_p0/test_binlog_property_validation.groovy
b/regression-test/suites/ddl_p0/test_binlog_property_validation.groovy
new file mode 100644
index 00000000000..15e9d1b82a2
--- /dev/null
+++ b/regression-test/suites/ddl_p0/test_binlog_property_validation.groovy
@@ -0,0 +1,108 @@
+// 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.
+
+// The "binlog.*" properties configure CCR binlog. A key under "binlog." other
than binlog.enable,
+// binlog.ttl_seconds, binlog.max_bytes and binlog.max_history_nums is
rejected instead of being
+// ignored, and binlog.enable accepts only true or false, in any case.
+suite("test_binlog_property_validation") {
+ sql "DROP TABLE IF EXISTS test_binlog_property_validation_tbl"
+
+ test {
+ sql """
+ CREATE TABLE test_binlog_property_validation_tbl (
+ k1 INT NOT NULL,
+ v1 INT
+ )
+ DUPLICATE KEY(k1)
+ DISTRIBUTED BY HASH(k1) BUCKETS 1
+ PROPERTIES (
+ "replication_num" = "1",
+ "binlog.ttl_second" = "3600"
+ )
+ """
+ exception "Unknown binlog property: binlog.ttl_second"
+ }
+ test {
+ sql """
+ CREATE TABLE test_binlog_property_validation_tbl (
+ k1 INT NOT NULL,
+ v1 INT
+ )
+ DUPLICATE KEY(k1)
+ DISTRIBUTED BY HASH(k1) BUCKETS 1
+ PROPERTIES (
+ "replication_num" = "1",
+ "binlog.enable" = "yes"
+ )
+ """
+ exception "Invalid binlog enable value: yes"
+ }
+
+ sql """
+ CREATE TABLE test_binlog_property_validation_tbl (
+ k1 INT NOT NULL,
+ v1 INT
+ )
+ DUPLICATE KEY(k1)
+ DISTRIBUTED BY HASH(k1) BUCKETS 1
+ PROPERTIES (
+ "replication_num" = "1",
+ "binlog.enable" = "False"
+ )
+ """
+ order_qt_table_binlog_enable """
+ SELECT TABLE_NAME, PROPERTY_NAME, PROPERTY_VALUE
+ FROM information_schema.table_properties
+ WHERE TABLE_CATALOG = 'internal'
+ AND TABLE_SCHEMA = '${context.dbName}'
+ AND TABLE_NAME = 'test_binlog_property_validation_tbl'
+ AND PROPERTY_NAME = 'binlog.enable'
+ """
+
+ // The unknown key used to fail with "Nothing is changed", and "yes" used
to be read as false,
+ // which turned binlog off on a table that had it on.
+ test {
+ sql """ALTER TABLE test_binlog_property_validation_tbl SET
("binlog.ttl_second" = "3600")"""
+ exception "Unknown binlog property: binlog.ttl_second"
+ }
+ test {
+ sql """ALTER TABLE test_binlog_property_validation_tbl SET
("binlog.enable" = "yes")"""
+ exception "Invalid binlog enable value: yes"
+ }
+
+ sql "DROP DATABASE IF EXISTS test_binlog_property_validation_db"
+ test {
+ sql """CREATE DATABASE test_binlog_property_validation_db PROPERTIES
("binlog.ttl_second" = "3600")"""
+ exception "Unknown binlog property: binlog.ttl_second"
+ }
+ sql "CREATE DATABASE test_binlog_property_validation_db"
+ // Database properties are stored as given, so these used to be persisted
as they were.
+ test {
+ sql """ALTER DATABASE test_binlog_property_validation_db SET
PROPERTIES ("binlog.max_byte" = "1024")"""
+ exception "Unknown binlog property: binlog.max_byte"
+ }
+ test {
+ sql """ALTER DATABASE test_binlog_property_validation_db SET
PROPERTIES ("binlog.enable" = "1")"""
+ exception "Invalid binlog enable value: 1"
+ }
+ order_qt_database_binlog_properties """
+ SELECT SCHEMA_NAME, PROPERTY_NAME, PROPERTY_VALUE
+ FROM information_schema.database_properties
+ WHERE SCHEMA_NAME = 'test_binlog_property_validation_db'
+ AND PROPERTY_NAME LIKE 'binlog.%'
+ """
+}
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
