Skip to content

mysql: retain safe mode at the ReplicatingTs boundary - #6780

Merged
ti-chi-bot[bot] merged 2 commits into
pingcap:masterfrom
wk989898:safe-mode-fix
Nov 5, 2026
Merged

ti-chi-bot[bot] merged 2 commits into
pingcap:masterfrom
wk989898:safe-mode-fix

Conversation

@wk989898

@wk989898 wk989898 commented Nov 2, 2026 •

Copy link
Copy Markdown
Collaborator

What problem does this PR solve?

Issue Number: close #6779

What is changed and how it works?

Treat events with CommitTs == ReplicatingTs as being in replay safe mode by changing the timestamp condition from < to <= in normal SQL generation, per-event batch generation, and cross-event batch generation.

This aligns SQL generation with the existing batch eligibility check, which classifies events into CommitTs <= ReplicatingTs and CommitTs > ReplicatingTs. It prevents an equality event at the beginning of a batch from causing subsequent replay events to use ordinary INSERT semantics.

Per-event evaluation is preserved: events that have crossed their own ReplicatingTs boundary continue to use ordinary SQL semantics unless another safe-mode condition is active.

Check List

Tests

  • Unit test
  • Integration test

Questions

Will it cause performance regression or break compatibility?
Do you need to update user documentation, design documentation or monitoring documentation?

Release note

Fix inconsistent replay safe-mode SQL generation at the replication timestamp boundary in the new TiCDC MySQL sink.

Summary by CodeRabbit

  • Bug Fixes
    • Corrected safe-mode handling for changes committed at the replication timestamp. These changes now use replacement semantics, consistent with changes committed before that timestamp.
    • Ensured batched and individual database writes handle events consistently at the timestamp boundary, avoiding inconsistent write behavior for changes arriving exactly when replication reaches that point.

Signed-off-by: wk989898 <nhsmwk@gmail.com>
@ti-chi-bot ti-chi-bot Bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. do-not-merge/needs-triage-completed labels Nov 2, 2026
@coderabbitai

coderabbitai Bot commented Nov 2, 2026 •

Copy link
Copy Markdown
Contributor

Explain Change →

No actionable comments were generated in the recent review.

Walkthrough

The MySQL DML SQL-generation paths now treat CommitTs == ReplicatingTs as safe mode. Tests cover the boundary across normal and batch SQL generation and verify prepareDMLs behavior.

Changes: MySQL safe-mode boundary

Layer / File(s) Summary
Apply the boundary across SQL generation and DML preparation
pkg/sink/mysql/mysql_writer_dml_batch.go, pkg/sink/mysql/mysql_writer_dml_test.go
The normal, per-event batch, and cross-event batch generators now treat CommitTs == ReplicatingTs as safe mode. Tests check the resulting REPLACE or INSERT statements and prepareDMLs behavior with batch DML enabled and disabled.

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: Medium

Possibly related PRs

  • pingcap/ticdc#6506: Introduced the MySQL batch-DML safe-mode path that this change updates at the equality boundary.

Suggested reviewers: lidezhu

Merge Risk: Minimal · up to ca3fa

The change consistently handles replication-boundary events without changing later events to replay-safe SQL. No actionable merge-blocking issue remains; merge readiness is subject to normal checks.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat… Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Title check Passed The title names the main change: the MySQL sink safe-mode condition now includes the ReplicatingTs boundary (CommitTs equal to ReplicatingTs). It is short, specific, and matches the raw summary and ob…
Description check Passed The description follows the template. It gives an issue line (Issue Number: close #6779), a clear 'What is changed and how it works' section explaining the < to <= change and per-event preservation, l…
Linked Issues check Passed Issue #6779 requires one replay-safe-mode predicate at the ReplicatingTs boundary and per-event evaluation. The production diff changes generateNormalSQL, generateBatchSQLsPerEvent, and `generat…
Out of Scope Changes check Passed The production change is limited to the three SQL-generation predicates required by issue #6779. The added tests exercise the boundary and mixed-event batching behavior. No unrelated change is shown i…
Full details: Title check

Explanation

The title names the main change: the MySQL sink safe-mode condition now includes the ReplicatingTs boundary (CommitTs equal to ReplicatingTs). It is short, specific, and matches the raw summary and objectives, which describe changing the comparison from < to <= in SQL generation.

Full details: Description check

Explanation

The description follows the template. It gives an issue line (Issue Number: close #6779), a clear 'What is changed and how it works' section explaining the < to <= change and per-event preservation, lists unit and integration tests in the checklist, and includes a release note. The two template questions on performance/compatibility and documentation are left blank, and the test checklist does not say which tests were run beyond the test names in the summary. These are minor gaps.

Full details: Linked Issues check

Explanation

Issue #6779 requires one replay-safe-mode predicate at the ReplicatingTs boundary and per-event evaluation. The production diff changes generateNormalSQL, generateBatchSQLsPerEvent, and generateBatchSQL to use CommitTs &lt;= ReplicatingTs. The added tests cover before, equal, and after cases for all three generators. They also cover mixed events, batch eligibility, and an update at equality.

Full details: Out of Scope Changes check

Explanation

The production change is limited to the three SQL-generation predicates required by issue #6779. The added tests exercise the boundary and mixed-event batching behavior. No unrelated change is shown in the reviewed diff.

Full details: Docstring Coverage

Explanation

Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 2 functions across 2 files.

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

Finishing Touches
Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

A rabbit checks the timestamp line,
At equality, safe writes shine.
REPLACE guards the rows in flight,
Tests confirm the boundary right.
The batch paths follow suit,
Then hops away, content and cute.

Recent review info
Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: dc58454b-bb62-4cfb-a3fe-993885ec6dfd
Commits

Reviewing files that changed from the base of the PR and between 828700a and ca3fa88.

Files selected for processing: 1
  • pkg/sink/mysql/mysql_writer_dml_test.go
Files skipped from review as they are similar to previous changes: 1
  • pkg/sink/mysql/mysql_writer_dml_test.go

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

@ti-chi-bot ti-chi-bot Bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 2, 2026
@ti-chi-bot ti-chi-bot Bot added needs-cherry-pick-release-nextgen-202609 Should cherry pick this PR to release-nextgen-202609 branch. and removed do-not-merge/needs-triage-completed labels Nov 4, 2026
@ti-chi-bot ti-chi-bot Bot added needs-1-more-lgtm Indicates a PR needs 1 more LGTM. approved labels Nov 4, 2026
@asddongmen asddongmen added the needs-cherry-pick-release-8.5 Should cherry pick this PR to release-8.5 branch. label Nov 5, 2026
@ti-chi-bot ti-chi-bot Bot added the lgtm label Nov 5, 2026
@ti-chi-bot

ti-chi-bot Bot commented Nov 5, 2026

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: asddongmen, lidezhu

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ti-chi-bot ti-chi-bot Bot removed the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Nov 5, 2026
@ti-chi-bot

ti-chi-bot Bot commented Nov 5, 2026

Copy link
Copy Markdown

[LGTM Timeline notifier]

Timeline:

  • 2026-11-04 11:38:55.442029102 +0000 UTC m=+666660.067179375: ☑️ agreed by lidezhu.
  • 2026-11-05 02:17:15.829996555 +0000 UTC m=+719360.455146824: ☑️ agreed by asddongmen.

@ti-chi-bot
ti-chi-bot Bot merged commit a39a643 into pingcap:master Nov 5, 2026
40 checks passed
@ti-chi-bot

Copy link
Copy Markdown
Member

In response to a cherrypick label: new pull request created to branch release-8.5: #6805.

@ti-chi-bot

Copy link
Copy Markdown
Member

In response to a cherrypick label: new pull request created to branch release-nextgen-202609: #6807.

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

Labels

approved lgtm needs-cherry-pick-release-8.5 Should cherry pick this PR to release-8.5 branch. needs-cherry-pick-release-nextgen-202609 Should cherry pick this PR to release-nextgen-202609 branch. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

inconsistent safe-mode checks at ReplicatingTs boundary

4 participants