Skip to content

Antalya 26.1 - Add ALTER TABLE MODIFY SETTING support for Hybrid watermarks #1659

Open
mkmkme wants to merge 2 commits intoantalya-26.1from
mkmkme/hybrid/alter-watermark
Open

Antalya 26.1 - Add ALTER TABLE MODIFY SETTING support for Hybrid watermarks #1659
mkmkme wants to merge 2 commits intoantalya-26.1from
mkmkme/hybrid/alter-watermark

Conversation

@mkmkme
Copy link
Copy Markdown
Collaborator

@mkmkme mkmkme commented Apr 17, 2026

Introduce hybridParam('name', 'type') pseudo-function for Hybrid engine
predicates, allowing segment boundaries to be changed at runtime via
ALTER TABLE ... MODIFY SETTING without recreating the table.

Key design decisions:

  • hybridParam() takes exactly two arguments (name, type); all values
    must be provided via ENGINE SETTINGS, keeping the API surface minimal
    and eliminating default-value complexity.
  • Watermark names must start with 'hybrid_watermark_' and exactly match
    a declared hybridParam() in the predicates. Typos in both CREATE SETTINGS and ALTER are rejected.
  • Values are validated against the declared type at CREATE and ALTER
    time, so invalid values never reach the runtime substitution path.
  • Only hybrid_watermark_* settings are allowed on Hybrid tables;
    regular DistributedSettings and RESET SETTING are rejected.
  • Runtime watermark state uses MultiVersionstd::map for lock-free
    reads and deterministic serialization order in SHOW CREATE TABLE.
  • In StorageDistributed::alter(), the watermark snapshot is published
    before in-memory metadata to prevent concurrent readers from
    observing new metadata with stale watermark values.
  • Predicate validation at CREATE time substitutes the effective
    SETTINGS value (not the type default) so value-sensitive expressions
    are checked against realistic data.

Example:

CREATE TABLE t 
ENGINE = Hybrid(
    cluster('cluster1', currentDatabase(), 'hot'),
        ts > hybridParam('hybrid_watermark_day')::Date,
    cluster('cluster2', currentDatabase(), 'cold'),
        ts <= hybridParam('hybrid_watermark_day')::Date
)
SETTINGS hybrid_watermark_day = '2026-04-01';

ALTER TABLE t
MODIFY SETTING hybrid_watermark_day = '2026-04-02';

Syntax sugar:

CREATE TABLE t 
ENGINE = Hybrid(
    cluster('cluster1', currentDatabase(), 'hot'),
        ts > hybrid_watermark_day::Date,
    cluster('cluster2', currentDatabase(), 'cold'),
        ts <= hybrid_watermark_day::Date
)
SETTINGS hybrid_watermark_day = '2026-04-01';

Changelog category (leave one):

  • New Feature

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

Added support for moving Hybrid table watermarks

CI/CD Options

Exclude tests:

  • Fast test
  • Integration Tests
  • Stateless tests
  • Stateful tests
  • Performance tests
  • All with ASAN
  • All with TSAN
  • All with MSAN
  • All with UBSAN
  • All with Coverage
  • All with Aarch64
  • All Regression
  • Disable CI Cache

Regression jobs to run:

  • Fast suites (mostly <1h)
  • Aggregate Functions (2h)
  • Alter (1.5h)
  • Benchmark (30m)
  • ClickHouse Keeper (1h)
  • Iceberg (2h)
  • LDAP (1h)
  • Parquet (1.5h)
  • RBAC (1.5h)
  • SSL Server (1h)
  • S3 (2h)
  • S3 Export (2h)
  • Swarms (30m)
  • Tiered Storage (2h)

mkmkme added 2 commits April 17, 2026 13:40
Introduce hybridParam('name', 'type') pseudo-function for Hybrid engine
predicates, allowing segment boundaries to be changed at runtime via
ALTER TABLE ... MODIFY SETTING without recreating the table.

Key design decisions:
- hybridParam() takes exactly two arguments (name, type); all values
  must be provided via ENGINE SETTINGS, keeping the API surface minimal
  and eliminating default-value complexity.
- Watermark names must start with 'hybrid_watermark_' and exactly match
  a declared hybridParam() in the predicates. Typos in both CREATE
  SETTINGS and ALTER are rejected.
- Values are validated against the declared type at CREATE and ALTER
  time, so invalid values never reach the runtime substitution path.
- Only hybrid_watermark_* settings are allowed on Hybrid tables;
  regular DistributedSettings and RESET SETTING are rejected.
- Runtime watermark state uses MultiVersion<std::map> for lock-free
  reads and deterministic serialization order in SHOW CREATE TABLE.
- In StorageDistributed::alter(), the watermark snapshot is published
  before in-memory metadata to prevent concurrent readers from
  observing new metadata with stale watermark values.
- Predicate validation at CREATE time substitutes the effective
  SETTINGS value (not the type default) so value-sensitive expressions
  are checked against realistic data.

Tests:
- Stateless tests covering CREATE, ALTER, DETACH/ATTACH persistence,
  multi-watermark, type conflict, invalid values, typo rejection,
  RESET SETTING rejection, and DistributedSettings rejection.
- Integration tests covering single-node, cross-node, and cluster()
  table function flows, each self-contained with own setup/teardown.

Documentation updated with hybridParam() syntax, ALTER examples,
multi-watermark usage, and restriction notes.

Made-with: Cursor
@mkmkme mkmkme added antalya port-antalya PRs to be ported to all new Antalya releases hybrid antalya-26.1 labels Apr 17, 2026
@github-actions
Copy link
Copy Markdown

Workflow [PR], commit [5c36b26]

@mkmkme
Copy link
Copy Markdown
Collaborator Author

mkmkme commented Apr 17, 2026

AI audit note: This review comment was generated by AI (gpt-5.4).

Audit update for PR #1659 (ALTER TABLE MODIFY SETTING support for Hybrid watermarks):

Confirmed defects:

Medium: ALTER MODIFY SETTING skips full predicate revalidation
Impact: a watermark change can be accepted and persisted even though the resulting Hybrid predicate becomes invalid for query rewrite/execution, leaving the table unreadable until the watermark is changed again.
Anchor: src/Storages/StorageDistributed.cpp / StorageDistributed::checkAlterIsPossible() vs Hybrid CREATE validation path
Trigger: change a watermark used in a value-sensitive predicate expression to a same-typed value that is semantically invalid for that expression.
Why defect: CREATE substitutes effective watermark values and revalidates every predicate with ExpressionAnalyzer, but ALTER only checks that the new setting parses as the declared type and never reruns predicate validation, so ALTER can admit states that CREATE would reject.
Fix direction (short): build the post-ALTER effective watermark map and reuse the same substituted-predicate validation used during CREATE.
Regression test direction (short): add a Hybrid predicate where only some same-typed watermark values are valid, and assert both CREATE and ALTER reject the bad value.

Example:

CREATE TABLE t
ENGINE = Hybrid(
    remote('localhost:9000', currentDatabase(), 'hot'),
        dateDiff(hybridParam('hybrid_watermark_unit', 'String'), ts, now()) < 30,
    remote('localhost:9000', currentDatabase(), 'cold'),
        dateDiff(hybridParam('hybrid_watermark_unit', 'String'), ts, now()) >= 30
)
SETTINGS hybrid_watermark_unit = 'day'
AS hot;

This CREATE should pass because 'day' is a valid String and also a semantically valid first argument for dateDiff().

But this ALTER is currently accepted too:

ALTER TABLE t MODIFY SETTING hybrid_watermark_unit = 'banana';

ALTER only validates that 'banana' parses as the declared type String. It does not revalidate the full predicate with substituted effective values. The next read then rewrites the predicate to:

dateDiff('banana', ts, now()) < 30

which is semantically invalid for dateDiff(). So CREATE and ALTER are not validation-equivalent for hybridParam() used in nontrivial expressions.

Coverage summary:

Scope reviewed: last commit in this PR, focusing on StorageDistributed Hybrid create/alter/read paths plus the added stateless/integration tests and docs
Categories failed: alter-time validation parity with create-time predicate validation
Categories passed: watermark name/type checks, missing-setting rejection, metadata persistence across detach/attach, multi-watermark update merge, concurrent read snapshot publication
Assumptions/limits: static audit only; I did not run the new tests or a live repro against ClickHouse

@mkmkme
Copy link
Copy Markdown
Collaborator Author

mkmkme commented Apr 17, 2026

The above comment is a very good point by /audit-review. I think we should fix it. TBH I didn't think of such a use case when implementing the feature.

FWIW the previous /audit-report run before the PR went smoothly without any discovered defects.

{
auto * arg_list = func->arguments->as<ASTExpressionList>();
const auto & param_name = arg_list->children[0]->as<ASTLiteral>()->value.safeGet<String>();
const auto & type_name = arg_list->children[1]->as<ASTLiteral>()->value.safeGet<String>();
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does make sense to check children size before to avoid out of bound error?

if (auto * func = node->as<ASTFunction>(); func && func->name == "hybridParam")
{
auto * arg_list = func->arguments ? func->arguments->as<ASTExpressionList>() : nullptr;
if (arg_list && arg_list->children.size() >= 2)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

throw error when size != 2?

if (name_lit && type_lit)
result.emplace(name_lit->value.safeGet<String>(), type_lit->value.safeGet<String>());
}
return;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Am I right that method collects only type for first hybridParam in predicate and exits here? Technically condition can have several parameters.

std::optional<NameDependencies> name_deps{};
for (const auto & command : commands)
{
if (command.type == AlterCommand::Type::MODIFY_SETTING)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT: I think now better to use switch-case instead of multiple ifs here.

@alex-zaitsev alex-zaitsev mentioned this pull request Apr 19, 2026
32 tasks
@UnamedRus
Copy link
Copy Markdown

Kinda work with ALIAS column, but it's not smart enough to correctly recognize (_table = xxxx AND ....) OR (_table = yyy AND ....)

https://fiddle.clickhouse.com/c667b318-1b36-45e7-ad14-b6db1fb55ff7

@alsugiliazova
Copy link
Copy Markdown
Member

Usability Audit: ALTER TABLE MODIFY SETTING for Hybrid watermarks (PR #1659)

Scope: Altinity/ClickHouse PR #1659 @ 5c36b262f71 — adds hybridParam('name','type') pseudo-function and runtime watermark modification via ALTER TABLE ... MODIFY SETTING on Hybrid engine tables.
Persona: Operator / DBA running tiered-storage or cluster-migration Hybrid tables in production.
Environment: Distributed cluster with at least one Hybrid table whose predicate uses hybridParam(); readers run concurrently with the DBA.
Source checked: branch mkmkme/hybrid/alter-watermark fetched into /Volumes/workspace/altinity-clickhouse/ClickHouse as pr-1659; full diff (git diff pr-1659~2..pr-1659) of 11 changed files; full PR conversation thread (body + 2 issue comments + 4 inline review comments from ianton-ru).

Summary of dedup vs PR comments

Before filing, every issue below was checked against the full PR thread (gh pr view 1659 --comments, gh api repos/Altinity/ClickHouse/pulls/1659/comments, gh api repos/Altinity/ClickHouse/issues/1659/comments).

# Issue Status
2.1 ALTER accepts a value that makes the predicate semantically invalid Already raised — AI /audit-review comment by mkmkme @ 2026-04-17; author agreed in follow-up
2.2 Failure after bad ALTER surfaces as cryptic function-level error, no pointer to the responsible watermark New in this audit
2.3 Once set, a watermark cannot be removed except by recreating the table New in this audit
1.1 Read path indexes hybridParam() arguments without bounds-checking Already raisedianton-ru inline on src/Storages/StorageDistributed.cpp:1246
1.2 collectHybridParamTypes() silently skips malformed hybridParam during ALTER validation Already raisedianton-ru inline on src/Storages/StorageDistributed.cpp:1762
1.3 Segment pruning doesn't recognise (_table = 'A' AND …) OR (_table = 'B' AND …) disjunctions Already raisedUnamedRus issue comment @ 2026-04-20, fiddle
X.1 No first-class visibility of current effective watermark values (only via SHOW CREATE TABLE) New in this audit
X.2 Watermark type is locked into predicates; type change requires recreating the table New in this audit

Other PR-thread items noted but not listed above:

  • ianton-ru inline on cpp:1769: question about whether collectHybridParamTypes visits all hybridParam nodes or only the first. After tracing the recursion in the diff, the logic is correct — the return; inside the hybridParam branch only prevents descending into the param's literal children; sibling hybridParams elsewhere in the tree are still reached via the parent's for (child : children) walk(child). Noted as a code-clarity concern, not a defect.
  • ianton-ru inline on cpp:1785: NIT to use switch/case instead of multiple ifs. Style only, not a usability issue.

Primary flows (ranked by expected usage)

  1. Create a tiered Hybrid table with a watermark and SELECT against it, e.g. ts > hybridParam('hybrid_watermark_hot','DateTime') hot / cold split. (most common — the feature's advertised use case in docs/en/engines/table-engines/special/hybrid.md)
  2. Move the boundary at runtime via ALTER TABLE t MODIFY SETTING hybrid_watermark_hot = '...'. (the whole point of the PR)
  3. Update multiple watermarks in one ALTER (documented in the diff).
  4. DETACH / ATTACH / server restart with watermark metadata persisted (explicit integration test test_hybrid_watermarks/test.py).
  5. Readers running concurrently with ALTER (explicit in PR body: "snapshot published before metadata").

Issues

Flow 2: Runtime watermark modification

2.1 ALTER accepts a watermark value that makes the predicate semantically invalid; next read fails

Status: already raised (AI /audit-review; author agreed).

  • Symptom: ALTER TABLE t MODIFY SETTING hybrid_watermark_unit = 'banana' succeeds silently; the next SELECT fails because the predicate rewrites to dateDiff('banana', ts, now()) < 30, which is not a valid first argument for dateDiff. The table becomes unreadable until the operator ALTERs again to a valid value. (src/Storages/StorageDistributed.cpp StorageDistributed::checkAlterIsPossible() vs CREATE path in registerStorageHybrid @ 5c36b26.)
  • Trigger: A watermark used in a value-sensitive expression (dateDiff, toStartOfInterval, parseDateTimeBestEffort, etc.) is ALTERed to a same-typed but semantically invalid string.
  • Signal quality: Delayed — ALTER returns success; failure appears later on reads only.
  • Severity: High.
  • Recovery: Engineer / DBA must identify the offending watermark and ALTER it again. No automatic rollback.

2.2 Failure from a bad ALTER surfaces as a cryptic function-level error on read

Status: new in this audit. Not in PR comments.

  • Symptom: After 2.1, users and ops dashboards see Illegal argument for function dateDiff (or similar) coming from the rewritten predicate; no mention of which watermark caused it, nor of the last ALTER.
  • Trigger: Any read that reaches 2.1.
  • Signal quality: Confusing — the error is about a function, not about settings.
  • Severity: Medium.
  • Recovery: Operator must correlate the read failure with the most recent ALTER MODIFY SETTING manually. substitute_watermarks in read() has the name/value in scope and could wrap the exception.

2.3 Once set, a watermark cannot be removed except by recreating the table

Status: new in this audit. Not in PR comments.

  • Symptom: ALTER TABLE t RESET SETTING hybrid_watermark_hot is explicitly rejected by checkAlterIsPossible() (src/Storages/StorageDistributed.cpp, AlterCommand::Type::RESET_SETTING branch). There is no "unset" path. The only way to drop a watermark is to recreate the table.
  • Trigger: Migration roll-back or cleanup after a staged rollout.
  • Signal quality: Clear (error message is explicit).
  • Severity: Low.
  • Recovery: Recreate the table.

Flow 1: Default Hybrid with watermark (create + SELECT)

1.1 Read path indexes hybridParam() arguments without bounds-checking

Status: already raised (ianton-ru inline on src/Storages/StorageDistributed.cpp:1246, unresolved in the branch).

  • Symptom: substitute_watermarks lambda in StorageDistributed::read() accesses arg_list->children[0]->as<ASTLiteral>()->value.safeGet<String>() and children[1] unconditionally. If an ASTFunction named hybridParam reaches this path with fewer than two children (e.g. corrupted metadata on reload, restored from a corrupted replica, or a future code path producing a malformed AST), the server crashes instead of throwing a clean error.
  • Trigger: Malformed hybridParam call reaching read(). Unreachable via CREATE today, but the symmetry with CREATE-time validation is missing.
  • Signal quality: Silent (crash).
  • Severity: Medium (unconfirmed at runtime; inferred from the diff and reviewer comment).
  • Recovery: Server restart; metadata surgery if repeatable.

1.2 Multi-watermark predicate: collectHybridParamTypes() silently skips malformed hybridParam calls

Status: already raised (ianton-ru inline on src/Storages/StorageDistributed.cpp:1762, author reacted 👍 but no fix in the branch).

  • Symptom: If a hybridParam node has children.size() != 2, collectHybridParamTypes() skips it silently (if (arg_list && arg_list->children.size() >= 2)). That name then does not appear in declared_types, and a matching ALTER is rejected with "does not match any declared hybridParam(); check for typos in the watermark name" — which is misleading because the real cause is a structural problem.
  • Trigger: Partially corrupt or hand-edited metadata, or a future refactor that emits a malformed node.
  • Signal quality: Confusing (error message points at typo, not structure).
  • Severity: Low.
  • Recovery: DBA has to read the DDL to realise the "typo" message is misleading.

1.3 Planner does not recognise (_table = 'A' AND …) OR (_table = 'B' AND …) for segment pruning

Status: already raised (UnamedRus issue comment @ 2026-04-20).

  • Symptom: When using a _table-based alias/filter scheme over Hybrid, the planner cannot restrict segment reads for disjunctions of _table = …. Hybrid reads more segments than logically required.
  • Trigger: Analytic queries that combine per-segment filters with OR.
  • Signal quality: Silent (wrong-performance, not wrong-results).
  • Severity: Medium. (Strictly out of scope of this PR, but directly affects the same persona on the same primary flow — tiered / migration reads.)
  • Recovery: Rewrite the query as UNION ALL of per-segment subqueries.

Cross-flow

X.1 No first-class visibility of current effective watermark values

Status: new in this audit. Not in PR comments.

  • Symptom: Operator wants to know "what is the current value of hybrid_watermark_hot on this table right now?". The only way is SHOW CREATE TABLE (which registerStorageHybrid rebuilds from the runtime snapshot) or parsing system.tables.create_table_query. No row in system.settings, no dedicated system.* view for Hybrid engine settings.
  • Trigger: Routine monitoring, alerting, or runbook that needs to confirm the live watermark.
  • Signal quality: Awkward (requires parsing DDL text).
  • Severity: Low.
  • Recovery: Monitoring integrates a DDL parser.

X.2 Declared watermark type is locked into predicates; changing type requires recreating the table

Status: new in this audit. Not in PR comments.

  • Symptom: Type appears in every hybridParam(name, type) call in the predicate ASTs. MODIFY SETTING changes the value only; there is no path to change the type. The same watermark name with two different types across predicates is rejected at CREATE (hybridParam() type conflict).
  • Trigger: Schema drift; migration from Date to DateTime at cut-over; need to widen a numeric watermark type.
  • Signal quality: Clear at CREATE; silent at migration time (no tooling).
  • Severity: Low.
  • Recovery: Recreate the table.

Readiness verdict

  • Flow 1 (create + SELECT) in isolation: ready with caveats — exposed to Update README.md #1.1 under corrupted-metadata failure modes and to Update README.md #1.3 for _table-disjunction query patterns.
  • Flow 2 (runtime watermark modification) in isolation: not ready, because of Update README.md #2.1 (ALTER can leave the table unreadable until another ALTER lands) and Update README.md #2.2 (error message does not point at the responsible watermark).
  • Flow 3 (multi-watermark ALTER in one statement) in isolation: ready; no flow-specific blockers found. The snapshot is replaced atomically by loadHybridWatermarkParams.
  • Flow 4 (DETACH / ATTACH / restart persistence) in isolation: ready; covered by the integration test test_hybrid_watermarks/test.py and by the SETTINGS-AST rebuild in registerStorageHybrid.
  • Flow 5 (concurrent readers during ALTER) in isolation: ready; alter() publishes the watermark snapshot via MultiVersion::set before setInMemoryMetadata, so readers cannot observe new metadata with stale values.
  • Overall in production (typos in runbooks, network blips, concurrent admin ops, scale): not ready; not yet safe for broad production use without guardrails, because of Update README.md #2.1 (availability risk on valid-type but semantically-bad ALTER) and Update README.md #1.1 (unchecked AST child access on the read path). Both are correctness / availability risks surfaced to the operator without any indication at the point of failure.

Operator guidance (until fixes land)

  • Avoid hybridParam() in value-sensitive positions (dateDiff(hybridParam(...), ...), parseDateTimeBestEffort(hybridParam(...)), string-enum arguments, etc.) until Update README.md #2.1 is fixed. Prefer watermarks in pure comparison positions (ts > hybridParam(...), date <= hybridParam(...)) where any same-typed value is semantically valid.
  • Dry-run before ALTER. Before issuing ALTER TABLE ... MODIFY SETTING hybrid_watermark_*, run a representative read against a staging copy of the table with the candidate value substituted manually. ALTER itself is not a sufficient validation for value-sensitive predicates (Update README.md #2.1).
  • Smoke-check after ALTER. Immediately run SELECT ... LIMIT 0 on the Hybrid table after every watermark ALTER; a function-level error on the smoke query is a bad-value indicator (Update README.md #2.2) — roll the watermark back.
  • Use a maintenance window for watermark ALTERs even though concurrent reads are snapshot-safe. Update README.md #2.1 + Update README.md #1.1 are availability risks, not visibility risks.
  • Avoid _table = 'A' OR _table = 'B' on Hybrid tables; rewrite as UNION ALL for correct segment pruning (Update README.md #1.3).
  • Capture SHOW CREATE TABLE periodically into monitoring so the effective watermark is always known to ops, regardless of who ALTERed it last (X.1).

Recommended fixes

  • Update README.md #2.1 (blocker for production readiness): in StorageDistributed::checkAlterIsPossible() for MODIFY_SETTING on Hybrid, build the post-ALTER effective watermark map (existing values + incoming changes), substitute into each predicate AST, and run the same TreeRewriter + ExpressionAnalyzer path as registerStorageHybrid's validate_predicate. This makes CREATE and ALTER validation-equivalent. Fix direction already agreed by the PR author.
  • Update README.md #2.2: when the substituted predicate throws on read, wrap the exception with context like "while evaluating Hybrid predicate with watermark '<name>' = '<value>'". substitute_watermarks has the name/value in scope.
  • Update README.md #1.1: add an arity/type check in substitute_watermarks symmetric to the one already in registerStorageHybrid::collect_hybrid_params — throw BAD_ARGUMENTS on malformed hybridParam.
  • Update README.md #1.2: throw instead of skip in collectHybridParamTypes(); the reviewer's inline comment at cpp:1762 already suggests this.
  • X.1: expose current effective watermark values via system.tables' settings column or a dedicated system.hybrid_watermarks view (name, value, declared_type, table).
  • Update README.md #1.3 (out of this PR): file a separate issue for planner-level _table-disjunction pruning in Hybrid; not a blocker for this PR's feature surface but affects the same operator journey.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

antalya antalya-26.1 hybrid port-antalya PRs to be ported to all new Antalya releases

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants