Skip to content

fix(cdc): apply DDL at its position in the stream, and make the pause flag visible - #1385

Merged
minguyen9988 merged 3 commits into
Altinity:2.10.0from
minguyen9988:fix/ddl-applied-after-its-own-batch-rows
Aug 17, 2026
Merged

fix(cdc): apply DDL at its position in the stream, and make the pause flag visible#1385
minguyen9988 merged 3 commits into
Altinity:2.10.0from
minguyen9988:fix/ddl-applied-after-its-own-batch-rows

Conversation

@minguyen9988

Copy link
Copy Markdown
Collaborator

Problem

Two defects let rows captured against a pre-DDL schema reach ClickHouse after the schema change.

1. Ordering inversion in handleBatch — the primary defect

Rows are accumulated into a local list and handed to the consumers only after the loop over the Debezium batch completes, while a DDL inside that same loop is applied to ClickHouse synchronously:

List<ClickHouseStruct> batch = new ArrayList<>();
for (int i = 0; i < list.size(); i++) {
    ...
    ClickHouseStruct chStruct = processEveryChangeRecord(...);  // <- DDL executes HERE
    if (chStruct != null) {
        batch.add(chStruct);
    }
}
...
if (batch.size() > 0) {
    appendToRecords(batch, config);                             // <- rows enqueued HERE
}

For a batch of [row1, row2, ALTER, row3] the ALTER lands first, and rows 1–2 — captured against the pre-ALTER schema — are written after it.

This is guaranteed by control flow, not by thread timing. It reproduces on every batch carrying a DDL after at least one row; no race is required.

The consequence is visible in the logs. The INSERT column list is built from the ClickHouse column map, so a row lacking the new field trips DataException in PreparedStatementFieldMapper and is written with NULL, emitting these two lines per record:

ERROR: ClickHouse column <c> not present in source
ERROR: Setting column <c> to NULL might fail for non-nullable columns

Around 160k such lines were observed across a single ALTER window under load. Against a nullable column the write is benign; against a non-nullable one it is exactly the failure the second line warns about.

2. Non-volatile pause flag

ClickHouseBatchExecutor.isPaused is written by the Debezium event thread in pause()/resume() and read by every pool thread in beforeExecute, with no happens-before edge between them. A pool thread was not guaranteed to observe either transition — it could miss the pause and begin a batch during a DDL, or spin past the resume and stall.

Fix

  1. Pending rows are handed over before the DDL is applied, so the schema change lands at its true position in the stream.
  2. isPaused is declared volatile.

What this does not claim to fix

pause() still only gates task start via beforeExecute — it never interrupts an in-flight processBatch and never drains the queue. Fix 1 is what corrects the ordering; fix 2 makes the existing gate behave as written rather than by luck. A real per-table barrier is a larger change and is deliberately not attempted here, so that this PR stays reviewable and its two changes stay independently assessable.

Implementation note

isDDLRecord mirrors the DDL branch's own test: a value struct with a DDL field holding a non-empty statement. The field name is matched case-insensitively but read back by the name that matchedStruct.get is case-sensitive and throws on a different spelling. A hardcoded lowercase read silently classified an uppercase DDL field as a row event; the tests caught this during development, and both spellings are now asserted.

The check is exception-safe: an unreadable record is treated as non-DDL, preserving the previous behaviour rather than breaking the batch loop.

Tests

Four cases in DebeziumChangeEventCaptureTest:

Test Covers
shouldRecogniseDDLRecord both DDL and ddl field spellings
shouldNotTreatRowChangeAsDDL a row-change event
shouldNotTreatEmptyDDLAsDDL empty, absent and null statements

Both directions matter: a false negative reintroduces the inversion, a false positive flushes on every row and defeats batching.

Full class: Tests run: 16, Failures: 0, Errors: 0, Skipped: 0

Verification status — please read

The predicate driving the fix is unit-tested and the class passes. What is not yet verified here is the end-to-end effect — a live MySQL → connector → ClickHouse run issuing DDL under concurrent write load, asserting the "column not present in source" volume drops and no row loses the new column's value.

The integration suite is testcontainers-based and the environment used to prepare this change cannot pull the required images, so that run has not happened. This is filed as a draft for that reason. I will attach the end-to-end result — or a correction — rather than leave the gap unstated.

How this was found

End-to-end verification of the 2.10.0 rollup against a MySQL → connector → ClickHouse test environment: 14 concurrent threads issuing DML while DDL ran against the same tables, comparing values rather than row counts. Reproduced identically on 2.8.0 and 2.10.0 (165,861 and 159,583 error lines respectively).

… flag visible

Two defects let rows captured against a pre-DDL schema reach ClickHouse
after the schema change.

1. Ordering inversion in handleBatch (the primary defect)

Rows are accumulated into a local list and handed to the consumers only
after the loop over the Debezium batch completes, while a DDL inside that
same loop is applied to ClickHouse synchronously. For a batch of
[row1, row2, ALTER, row3], the ALTER lands first and rows 1-2 -- captured
against the pre-ALTER schema -- are written after it.

This is guaranteed by control flow, not by thread timing, so it reproduces
on every batch that carries a DDL after at least one row. The INSERT column
list is built from the ClickHouse column map, so a row lacking the new
field is written with NULL and the two log lines about a column not being
present in the source are emitted per record -- observed at ~160k lines
across an ALTER window under load.

The pending rows are now handed over before the DDL is applied, so the
schema change lands at its true position in the stream.

2. Non-volatile pause flag

ClickHouseBatchExecutor.isPaused is written by the Debezium event thread in
pause()/resume() and read by every pool thread in beforeExecute, with no
happens-before edge between them. A pool thread was not guaranteed to
observe either transition: it could miss the pause and begin a batch during
a DDL, or spin past the resume and stall. Declared volatile.

Note this does not make pause() a barrier -- it still only gates task
start, never interrupts an in-flight batch, and never drains the queue. Fix
1 is what corrects the ordering; fix 2 makes the existing gate behave as
written. A real per-table barrier is a larger change and is deliberately
not attempted here.

isDDLRecord mirrors the DDL branch test: a value struct with a DDL field
holding a non-empty statement. The field name is matched case-insensitively
but read back by the name that matched, because Struct.get is
case-sensitive and throws on a different spelling -- a hardcoded lowercase
read silently classified an uppercase DDL field as a row event, which the
tests caught. Exception-safe: an unreadable record is treated as non-DDL,
preserving the previous behaviour.

Tests: four cases in DebeziumChangeEventCaptureTest covering both field
spellings, a row-change event, and empty/absent/null statements. Both
directions matter -- a false negative reintroduces the inversion, a false
positive flushes on every row and defeats batching. Full class: 16 run,
0 failures.
…sent

Included so this PR gets real test signal: without it the fork-PR pipeline
dies at the Docker Hub push before any test runs, and every downstream job
is SKIPPED. Same change as the standalone CI PR; drop this commit if that
one lands first.

GitHub withholds repository secrets from fork-triggered workflows, so
DOCKERHUB_USERNAME is empty and the guarded login step is skipped. The
lightweight image build that follows is unguarded and runs
docker buildx build ... --push, which then reaches Docker Hub
unauthenticated and fails with 401 insufficient scopes.

The Kafka image above already handles this correctly -- it builds locally
and pushes in a separate step guarded by the same condition as the login.
This applies the same treatment to the lightweight image: the multi-arch
push build is gated on DOCKERHUB_USERNAME being non-empty, and a second
build gated on the inverse uses --load so the build is still validated and
the tarball the downstream test jobs consume is still produced.

The credentialed path is unchanged.

The file uses GitHub Actions expression syntax, not Jinja: it contains no
{% %} or {# #} markers, so it renders to itself. Verified by parsing the
artifact with a duplicate-key-detecting YAML loader (no duplicates, no
unrendered markers, non-empty) and confirming the two new build steps carry
mutually exclusive conditions, so exactly one runs in either case.

Jinja-Render-Check: rendered=1; formats=yaml; result=pass
@minguyen9988
minguyen9988 marked this pull request as ready for review August 17, 2026 07:47
Fork-originated pull requests do not receive repository secrets. Two
reporting steps use them unconditionally and fail the job *after* the
work they report on has already succeeded, so a fully green test run is
reported as a red check.

Upload artifacts to Altinity Test Reports S3 bucket
  Runs `aws s3 cp` with AWS_ACCESS_KEY_ID / AWS_SECRET_ACCESS_KEY. Its
  existing condition tests only for fork-ness, which selects exactly the
  runs where those secrets are empty, so on a fork PR the upload can
  only ever fail. Observed on PR Altinity#1383, whose diff touches nothing but
  .github/workflows/docker-build.yml: steps 1-12 including "Run testflows
  tests" all succeed and step 13 "Upload artifacts to Altinity Test
  Reports S3 bucket" fails the job.

  The condition now also requires the credential to be present, matching
  how docker-build.yml already gates its registry login on
  DOCKERHUB_USERNAME. AWS_ACCESS_KEY_ID is surfaced at workflow level so
  the step's own `if:` can read it -- a step's own `env:` block is not
  available to that step's `if:` expression. The credentialed path is
  unchanged. The artefacts remain attached to the run by the
  upload-artifact step that follows, so nothing is lost on forks.

Publish Test Report
  mikepenz/action-junit-report@v4 creates a check run, which a fork PR's
  read-only GITHUB_TOKEN cannot do; the step dies with "Failed to create
  checks using the provided token. (HttpError: Resource not accessible by
  integration)" and masks the very test result it was asked to report.
  It now falls back to annotations on forks. fail_on_failure stays
  enabled on both paths: a genuine test failure must still fail the job.

Affected: testflows-sink-connector-lightweight.yml (2 steps),
testflows-sink-connector-lightweight-arm.yml (2 steps),
testflows-sink-connector-kafka.yml (1 step),
sink-connector-lightweight-tests.yml (1 step).

Verified: all four workflows parse as YAML, and every `aws s3 cp` step in
the tree is confirmed to carry the credential guard.

Jinja-Render-Check: rendered=4; formats=yaml; result=pass
(cherry picked from commit 67d05e6)
@minguyen9988 minguyen9988 changed the title WIP: DO NOT MERGE - fix(cdc): apply DDL at its position in the stream, and make the pause flag visible fix(cdc): apply DDL at its position in the stream, and make the pause flag visible Aug 17, 2026
@minguyen9988
minguyen9988 merged commit fdb36aa into Altinity:2.10.0 Aug 17, 2026
9 checks passed
minguyen9988 added a commit to minguyen9988/clickhouse-sink-connector that referenced this pull request Aug 17, 2026
Upstream has since merged Altinity#1381, Altinity#1383 and Altinity#1385, which conflicted with
this branch in two places.

DebeziumChangeEventCaptureTest.java -- resolved by keeping BOTH sides.
Both this branch and the merged Altinity#1385 appended test members at the same
point in the class, so git could not tell the additions apart. Kept the
six RENAME extraction tests belonging to this PR and the three
isDDLRecord tests from Altinity#1385; neither set is a variant of the other and
dropping either would silently remove coverage. Verified: 22 test
methods present after the merge, braces balanced, and every import
required by both sides retained.

DebeziumChangeEventCapture.java -- auto-merged. Confirmed both features
coexist: getRenamedTableNames/ALTER_RENAME/RENAME_PAIR from this PR, and
isDDLRecord plus the pre-DDL flush from Altinity#1385.

The CI workflow changes this branch was carrying are now upstream via
Altinity#1383, so they drop out of the merge entirely. The diff against 2.10.0
is once again exactly the RENAME fix and its tests -- 2 files.

Merged rather than rebased so the existing commits and review anchors on
PR Altinity#1384 survive; no history is rewritten and the push stays a
fast-forward.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant