morningman opened a new pull request, #69375:
URL: https://github.com/apache/doris/pull/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) |
### Release note
The `binlog.*` properties of tables and databases are validated strictly:
CREATE / ALTER TABLE and CREATE / ALTER DATABASE reject a key under `binlog.`
other than `binlog.enable`, `binlog.ttl_seconds`, `binlog.max_bytes` and
`binlog.max_history_nums`, and `binlog.enable` accepts only `true` or `false`.
Before, such keys were ignored or stored, and any other `binlog.enable` value
was read as false.
### Check List (For Author)
- Test: Unit Test, Regression test, Manual test
- FE UT: `PropertyAnalyzerTest.testAnalyzeBinlogConfig` and
`ChangelogConfigSplitTest.testDdlValidatesBinlogProperties` (new; CREATE TABLE
and ALTER TABLE), plus the other test classes that create tables or databases
with binlog properties.
- Regression: `ddl_p0/test_binlog_property_validation` (new) covers each
row of the Results table on tables and databases.
- Manual: the Before table was taken on a local cluster running master
before this change.
- Behavior changed: Yes. DDL that set an unknown `binlog.*` key or a
non-boolean `binlog.enable` now fails instead of being ignored, stored, or read
as false.
- Does this need documentation: No. The current docs do not describe other
`binlog.*` keys or other `binlog.enable` values.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
