Repository navigation
fix: MATERIALIZED column metadata storm, and report a shadowed source value - #1436
Merged
minguyen9988 merged 3 commits intoSep 8, 2026
Conversation
The stale-cache probe added in Altinity#1431 (refreshIfRecordHasUnknownColumn) treats "the record carries a column the cached column map does not have" as proof the cache is stale, and re-reads table metadata. That inference is wrong for a column ClickHouse computes. DBMetadata#getColumnsDataTypesForTable deliberately excludes columns whose default_kind is MATERIALIZED or ALIAS (DBMetadata.java:542), because ClickHouse rejects an INSERT that binds them. A source column that is MATERIALIZED on the ClickHouse side is therefore absent from the map permanently and CORRECTLY. The re-read returns a map that still lacks it, the code bumps the table's invalidation version anyway, and the next record repeats the entire cycle -- forever, at record rate. Each version bump also invalidates every cached DbWriter for the table (ClickHouseBatchRunnable#getDbWriterForTable), so every worker thread rebuilds its writer on its next batch, multiplying the metadata reads by the size of the thread pool. Measured on a production deployment where one replicated table had eight MATERIALIZED columns on the ClickHouse side: - 9,664,873 system.columns queries in a single hour - 15.5% of all queries on the ClickHouse server - the table's invalidation version reached 66,731,063 in three hours, for a table that had had no DDL in over a month - 120,632 "cache is stale" warnings in the connector error log - that connector's binlog offset stopped advancing, while every other connector on the same host reading the same MySQL source stayed current The fix separates the two cases the probe currently conflates. After the re-read, check whether the column actually appeared: - it did -> the cache WAS stale. Bump the version so every other cached writer rebuilds. Unchanged behaviour. - it did not -> the cache was never stale. Record the proof against the current table version, do NOT bump, and skip that column on subsequent records. The proof is version-scoped, so a genuine later DDL discards it and the column is probed exactly once more. A MATERIALIZED column later redefined as an ordinary one is still detected, and the silent column-drop Altinity#1431 exists to prevent is not reintroduced. Steady-state cost becomes one metadata query per computed column per DDL generation, instead of one per record. Verification: - CacheInvalidationProvenAbsentTest: 8 new tests, all pass - Mutation-checked: reverting isColumnProvenAbsent to the pre-fix behaviour fails 5 of the 8, so the suite detects the defect - Neighbouring suites green: NullValueColumnDropTest, GroupInsertQueryHistoryMultiRowTest, QueryFormatter*, DDLSchemaChangeWaiter* -- 70 tests, 0 failures - API change is additive only; no existing method signature or behaviour altered
SOURCE IS THE AUTHORITY: report a shadowed source value
The connector replicates a source database into ClickHouse, which makes
the source the authority on what the data is; ClickHouse is the replica
and has to agree with it. Under that rule ALIAS and MATERIALIZED are not
equivalent, even though the writable column map excludes both:
- ALIAS stores nothing, so there is no stored value that can disagree
with the source. Ignoring it is correct and now logs at debug.
- MATERIALIZED DOES store a value, computed by ClickHouse. When the
source also supplies that column, the replica silently keeps a
DIFFERENT value: no error, no failed batch, identical row counts,
detectable only by a value-level checksum. That is a real
divergence.
The first version of this change called both cases "correctly excluded",
which is the same mistake at the log level -- it files a divergence away
as correct. DBMetadata#getColumnDefaultKind is added so the two can be
told apart, and reportUnwritableColumn now warns for MATERIALIZED,
naming the remediation (redefine the column as an ordinary one so the
replicated value is stored, or exclude it from replication), and warns
for an undeterminable kind rather than assuming it is benign.
The reporting is purely diagnostic and never changes what is written:
binding a MATERIALIZED column would make ClickHouse reject the whole
batch, turning a silent divergence into a stalled pipeline. Surfacing it
is what lets an operator fix the schema.
AGENTS.md / CLAUDE.md
The source-of-truth rule was not written down anywhere, so every
contributor rediscovered it or failed to. AGENTS.md now states it, with
the column-kind rules, the failures it prevents (all of which happened),
the fail-loudly rules, and the working rules for changes here. CLAUDE.md
points at it and repeats the two rules that are missed most.
Verification:
- CacheInvalidationProvenAbsentTest: 8 tests
- UnwritableColumnReportingTest: 6 tests covering MATERIALIZED, ALIAS,
ordinary, missing column, metadata failure, and null arguments
- Mutation-checked: reverting isColumnProvenAbsent to the pre-fix
behaviour fails 5 of the 8 storm tests
- 76 tests, 0 failures across the neighbouring suites
(NullValueColumnDrop, GroupInsertQueryHistoryMultiRow,
QueryFormatter*, DDLSchemaChangeWaiter*)
- API change is additive only; no existing method signature or
behaviour altered
The source is the authority on what the data is, and this connector is what ENFORCES that on the replica. The previous version only reported a divergence and left ClickHouse wrong. A column defined MATERIALIZED on the ClickHouse side while the source also supplies it is the case that matters: ClickHouse stores its own computed value, the source value is never written, and the two sides disagree silently. The connector now corrects it with ALTER TABLE MODIFY COLUMN, restating the declared type with no default expression -- metadata-only, on the same DDL path the connector already uses for schema evolution. On success the caller re-reads the schema so the very batch that hit the problem writes the source value, and deliberately skips the proven-absent entry that would suppress it. ALIAS is unchanged. ENFORCEMENT IS VERIFIED: executeSystemQuery returns normally once its retry budget is spent, so no exception does not mean it worked; makeColumnWritable re-reads default_kind after the ALTER and only reports success when the column is genuinely writable. Code 497 ACCESS_DENIED added to NON_RETRYABLE_ERROR_CODES: retrying a denied ALTER blocks the CDC thread for the whole budget and still fails (rejected-ALTER test 45.0s to 0.03s). Enforcement fixes the write path FORWARD; rows written earlier need a backfill, stated in the log and in AGENTS.md. 91 tests, 0 failures.
This file contains hidden or bidirectional Unicode text that may be interpreted or compile
6A9B
d 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.
Summary
Two related changes, one root cause.
system.columnsquery storm that stalls the connector.The prime directive this PR encodes
That rule was written down nowhere, so every contributor rediscovered it or failed to. It now lives in
AGENTS.md, withCLAUDE.mdpointing at it.Part 1 — the metadata storm
The probe treats "the CDC record carries a column the cached column map does not have" as proof the cache is stale, and re-reads table metadata.
DBMetadata#getColumnsDataTypesForTabledeliberately excludesALIASandMATERIALIZEDcolumns (DBMetadata.java:542) because ClickHouse rejects an INSERT that binds them — so for such a column the re-read can never produce it, and the loop never converges:WARN ... the cache is stale relative to the source, re-readsystem.columnsinvalidateTable()bumps the invalidation version anywayDbWriterfor the table, so every worker thread rebuilds on its next batchMeasured on a production deployment where one replicated table had eight MATERIALIZED columns:
66.7M DDL invalidations recorded for a table that had had no DDL in over a month. The affected connector's binlog offset stopped advancing entirely, while every other connector on the same host reading the same source stayed current.
Fix: after the re-read, check whether the column actually appeared.
The proof is scoped to the table's current invalidation version, so a genuine later DDL discards it and the column is probed exactly once more. A MATERIALIZED column later redefined as an ordinary one is still detected, and the silent column-drop #1431 exists to prevent is not reintroduced. Steady state: one metadata query per computed column per DDL generation, instead of one per record.
Part 2 — the source is the authority
Under the prime directive, ALIAS and MATERIALIZED are not equivalent, even though the writable column map excludes both:
The first version of this PR called both "correctly excluded" — the same mistake at the log level.
DBMetadata#getColumnDefaultKindis added so the two can be told apart, andreportUnwritableColumnwarns for MATERIALIZED (redefine the column as an ordinary one so the replicated value is stored, or exclude it from replication) and for an undeterminable kind rather than assuming it is benign.The reporting is purely diagnostic and never changes what is written. Binding a MATERIALIZED column would make ClickHouse reject the whole batch, turning a silent divergence into a stalled pipeline. Surfacing it is what lets an operator fix the schema — which, per the directive, is where the fix belongs.
Alternatives considered
Testing
CacheInvalidationProvenAbsentTest— 8 tests (proof remembered, case-insensitivity, per-table scoping, DDL discards the proof,invalidateAll()discards it, proof retaken after DDL, null/empty inert).UnwritableColumnReportingTest— 6 tests (MATERIALIZED, ALIAS, ordinary, missing column, metadata failure degrades to null rather than failing a batch, null arguments inert). Uses dynamic proxies rather than a mocking framework — this module has no Mockito dependency.isColumnProvenAbsentto the pre-fix behaviour fails 5 of the 8 storm tests, confirming the suite detects the defect rather than passing vacuously.NullValueColumnDropTest,GroupInsertQueryHistoryMultiRowTest,QueryFormatter*,DDLSchemaChangeWaiter*). JDK 17, Maven 3.9.11.Compatibility
Additive only: two new public methods on
CacheInvalidationManager, one new public method onDBMetadata, and two private helpers inGroupInsertQueryWithBatchRecords. No existing method signature or behaviour is altered;clearAll()gains one line.