Repository navigation
[fix](fe) Reject unknown binlog properties and non-boolean binlog.enable - #69375
Merged
Merged
Conversation
### What problem does this PR solve? Issue Number: None Related PR: apache#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 apache#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. - apache#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. Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 28687 ms |
Contributor
TPC-DS: Total hot run time: 150228 ms |
Contributor
ClickBench: Total hot run time: 24.24 s |
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
Author
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
No substantiated new findings in the six changed files at 938f875. Review converged in one round: all three reviewers returned NO_NEW_VALUABLE_FINDINGS, all assigned coverage is complete, and both initial risk questions were dismissed with independently verified evidence. There are no existing inline findings to carry forward. No additional user focus was supplied.
The relevant flow is raw DDL properties -> shared binlog validation -> typed configuration/default merge -> metadata update. CREATE TABLE now validates before the merge can turn an invalid boolean into false. Database and table image/replay reconstruction continue through the unchanged permissive BinlogConfig parser.
Critical checkpoint conclusions:
- Goal: the four supported binlog keys remain accepted; unknown binlog.* keys and values other than case-insensitive true/false fail before configuration changes. Added unit and regression cases exercise this behavior.
- Scope and reuse: both production changes are focused and reuse the shared analyzer and existing exception handling.
- Concurrency and locking: validation uses statement-owned map copies, introduces no shared state or locks, and preserves the existing database-default read-lock snapshot. No new lock-order or blocking-IO concern was found.
- Lifecycle: no resource, task, static-initialization or ownership mechanism is added; early CREATE validation precedes table/tablet allocation.
- Configuration: no configuration item is added. The existing property grammar becomes strict while mixed-case booleans remain supported.
- Compatibility: no storage/wire format changes. Historical database properties remain loadable because strict validation stays in statement paths; typed inheritance avoids carrying unknown raw keys into new tables. The now-private legacy helper has no external references.
- Thrift/protobuf IDs: no gensrc definitions change.
- Parallel paths: CREATE/ALTER TABLE and DATABASE use the analyzer. Ordinary, temporary, CTAS, LIKE and cloud creation share the checked path; LIKE/CCR-generated binlog properties are canonical typed values.
- Conditions: the allowlist and boolean checks reject concrete invalid inputs; legacy-key migration errors retain priority. CREATE's copied-map comment explains the pre-normalization check.
- Tests: parser tests cover all four keys, normalization, map consumption and invalid inputs. DDL tests cover failed CREATE and preservation of an enabled table after failed ALTER; regression negatives cover all four DDL forms.
- Expected results: both new output blocks match the metadata emitters statically: canonical false for the table, and no matching binlog property for the untouched database.
- Observability: property/value-specific exceptions provide sufficient synchronous validation diagnostics; no additional metrics or logging is needed.
- Persistence and failover: existing journal writes, replay and image reconstruction are unchanged; strict validation does not enter replay.
- Data changes and atomicity: early rejection cannot mutate committed metadata. No row-data, transaction, visible-version or delete-bitmap logic changes.
- FE-BE variables: none are added or changed.
- Performance: the extra scan/map copy is limited to DDL property maps; no query hot-path work or external IO is introduced.
- Other concerns: the final sweep found no further substantiated issue or unresolved code-path question.
Validation limits: static review only; no builds or tests were run under the review contract. The expected-output generation provenance was not independently verified. These limits do not represent an unresolved code finding.
Review converged after 1 round(s).
924060929
approved these changes
Oct 22, 2026
Contributor
|
PR approved by anyone and no changes requested. |
github-actions
Bot
added
the
approved
Contributor
|
PR approved by at least one committer and no changes requested. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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_bytesandbinlog.max_history_nums.PropertyAnalyzer.analyzeBinlogConfig, which every DDL uses to check them, ignored any other key underbinlog.and readbinlog.enablewithBoolean.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 thechangelog.*properties strict.Background.
analyzeBinlogConfig: CREATE DATABASE and ALTER DATABASE (CreateDatabaseCommand/AlterDatabasePropertiesCommand.validate), CREATE TABLE (InternalCatalog.createOlapTable) and ALTER TABLE (ModifyTablePropertiesOp.validate, thenSchemaChangeHandler.updateBinlogConfig). Database properties are stored as the user wrote them.changelog.*properties reject unknown keys (Unknown changelog property: <key>) and accept onlytrue/falseforchangelog.enable; the replacedbinlog.formatandbinlog.need_historical_valueare 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:
ALTER TABLE t SET ("binlog.enable" = "yes"),thas binlog ont's binlog is now offCREATE TABLE ... ("binlog.enable" = "yes")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")ALTER DATABASE d SET PROPERTIES ("binlog.max_byte" = "1024")ALTER DATABASE d SET PROPERTIES ("binlog.enable" = "1")"1"is persisted and read as falseHow this PR fixes it.
analyzeBinlogConfigrejects any key underbinlog.other than the four withUnknown binlog property: <key>. The replacedbinlog.formatandbinlog.need_historical_valueare still checked first, so they keep their message that names the replacement.binlog.enableaccepts onlytrueorfalse, in any case, and otherwise fails with the existingInvalid binlog enable value: <value>, which the oldtry/catcharoundBoolean.parseBooleancould never reach.analyzeBinlogConfigsees the effective binlog config (the database default merged with the table properties) thatcreateOlapTableputs back over them, and by then"yes"has already become"false".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 aBinlogConfigobject, not as properties, so it does not make CREATE TABLE fail either.Results.
ALTER TABLE t SET ("binlog.enable" = "yes")Invalid binlog enable value: yes;tkeeps its binlog settingCREATE TABLE ... ("binlog.enable" = "yes")Invalid binlog enable value: yesALTER TABLE/CREATE TABLE/CREATE DATABASEwith"binlog.ttl_second"Unknown binlog property: binlog.ttl_secondALTER DATABASE ... ("binlog.max_byte" = "1024")Unknown binlog property: binlog.max_byteALTER DATABASE ... ("binlog.enable" = "1")Invalid binlog enable value: 1"binlog.enable" = "TRUE"/"False"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 underbinlog.other thanbinlog.enable,binlog.ttl_seconds,binlog.max_bytesandbinlog.max_history_nums, andbinlog.enableaccepts onlytrueorfalse. Before, such keys were ignored or stored, and any otherbinlog.enablevalue was read as false.Check List (For Author)
PropertyAnalyzerTest.testAnalyzeBinlogConfigandChangelogConfigSplitTest.testDdlValidatesBinlogProperties(new; CREATE TABLE and ALTER TABLE), plus the other test classes that create tables or databases with binlog properties.ddl_p0/test_binlog_property_validation(new) covers each row of the Results table on tables and databases.binlog.*key or a non-booleanbinlog.enablenow fails instead of being ignored, stored, or read as false.binlog.*keys or otherbinlog.enablevalues.