Skip to content

sink/mysql: split DML and control DB pools - #5397

Merged
ti-chi-bot[bot] merged 13 commits into
pingcap:masterfrom
hongyunyan:codex/mysql-control-pool
Jun 17, 2026
Merged

sink/mysql: split DML and control DB pools#5397
ti-chi-bot[bot] merged 13 commits into
pingcap:masterfrom
hongyunyan:codex/mysql-control-pool

Conversation

@hongyunyan

@hongyunyan hongyunyan commented Jun 15, 2026

Copy link
Copy Markdown
Collaborator

What problem does this PR solve?

Issue Number: close #5360

The MySQL sink used one shared *sql.DB pool for DML writers and control-plane work such as DDL execution, DDL-ts metadata, syncpoint metadata, and active-active progress updates. When a DML writer held the only available connection, control-plane operations could block while waiting for a connection. In the reported failure, the DDL was received and dispatched but did not reach downstream because the DDL path could not acquire a connection from the shared pool.

What is changed and how it works?

This PR splits the MySQL sink connection usage into two independent bounded pools created from the same effective DSN:

  • The DML pool is used by DML writers, DML sessions, prepared statement cache use, and active-active sync stats session queries.
  • The control pool is used by the DDL writer, DDL-ts metadata, syncpoint metadata, active-active progress table updates, DDL status queries, and metadata bootstrap.
  • The existing NewMysqlConfigAndDB single-pool API is preserved for existing callers.
  • A new NewMysqlConfigAndDBs helper creates the sink-specific DML/control pools and closes the DML pool if control pool creation fails.
  • MySQLSinkForceSingleConnection now only constrains the DML pool, so the stress path can still simulate DML session starvation without starving DDL/control operations.
  • Sink.Close closes both pools and avoids double-closing when tests pass the same DB for both.

The control pool is bounded to 4 open and idle connections.

Check List

Tests

  • Unit test
  • Manual test (add detailed scripts or steps below)

Commands:

  • make fmt
  • go test --tags=intest ./pkg/sink/mysql ./downstreamadapter/sink/mysql

Not run:

  • make integration_test_mysql CASE=ddl_default_current_timestamp, because make check_third_party_binary fails in this worktree: the required third-party binaries under bin/ are not present.

Questions

Will it cause performance regression or break compatibility?

No compatibility break is expected. Existing single-pool factory callers keep the same API. The MySQL sink now opens a small additional control pool, which should reduce DDL/control starvation risk. The DML pool keeps one extra connection for prepared-statement cache misses instead of reserving the historical broader control-plane margin.

Do you need to update user documentation, design documentation or monitoring documentation?

No.

Release note

Fix a MySQL sink hang where DDL and metadata operations could be blocked by DML sessions when the shared downstream connection pool was exhausted.

Summary by CodeRabbit

  • Improvements

    • Enhanced the MySQL sink to use separate long-lived database connections for data changes versus control-plane tasks (DDL and metadata), reducing contention and improving parallelism.
    • Added configurable connection pool sizing for each path and improved verification to validate both connections.
  • Tests

    • Added integration-style coverage to confirm DDL/control-plane operations run on the control connection while batched DML runs on the dedicated DML connection.
    • Added a unit test to verify control connection pooling configuration behavior.

@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/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Jun 15, 2026
@ti-chi-bot

ti-chi-bot Bot commented Jun 15, 2026

Copy link
Copy Markdown

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@coderabbitai

coderabbitai Bot commented Jun 15, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

The MySQL sink is refactored from owning a single long-lived connection to managing a slice of owned DB pools. A new NewMysqlConfigAndDBs factory creates separate DML and control-plane pools with independent sizing via extracted helpers. The Sink struct stores ownedDBs, constructor methods wire to the correct pool based on operation type (DML vs DDL/syncpoint), and Close iterates over all owned pools. Tests verify connectivity for both pools and that DML and control-plane operations route correctly.

Changes

Separate DML and control-plane DB pools for MySQL sink

Layer / File(s) Summary
Connection pool factory and pool-sizing helpers
pkg/sink/mysql/config.go, pkg/sink/mysql/config_test.go
Adds defaultDMLExtraConns and defaultControlDBConns constants. Refactors newMysqlConfigAndDB to return DSN string. Extracts configureDMLDBConn and configureControlDBConn pool-sizing helpers with shared failpoint injection. Adds exported NewMysqlConfigAndDBs(...) that creates dmlDB, generates DSN, creates controlDB from DSN, and includes cleanup on control-DB creation failure. Adds test functions TestConfigureControlDBConn and TestConfigureDMLDBConn verifying pool-size configuration.
Sink struct, construction, and pool ownership
downstreamadapter/sink/mysql/sink.go
Changes Sink struct field from db *sql.DB to ownedDBs []*sql.DB. Updates New to call NewMysqlConfigAndDBs and build sink via newMySQLSinkWithControlDB. Refactors NewMySQLSink to delegate to shared newMySQLSinkWithControlDB constructor with backward compatibility (same DB for both pools). Assigns ownedDBs slice during sink construction.
Writer initialization and pool routing
downstreamadapter/sink/mysql/sink.go
Updates writer initialization to route DML writers to dmlDB and DDL/syncpoint/active-active progress writers to controlDB. Updates Close to iterate over ownedDBs and close each pool with warning logging on individual close errors.
Connectivity verification and separate-pool routing tests
downstreamadapter/sink/mysql/sink.go, downstreamadapter/sink/mysql/sink_test.go
Updates Verify to create and validate both DML and control-plane pools via NewMysqlConfigAndDBs. Adds getMysqlSinkWithSeparateDBs test helper creating Sink with two independent sqlmock instances. Adds expectCreateTableDDLFlow helper centralizing DDL-ts control-DB SQL expectations. Updates TestMysqlSinkBasicFunctionality to use centralized expectations. Adds TestMysqlSinkUsesSeparateDMLAndControlDBPools to assert that DDL-ts operations hit controlMock and DML inserts hit dmlMock.

Sequence Diagram(s)

sequenceDiagram
    participant Dispatcher
    participant Sink
    participant dmlDB as dmlDB<br/>(DML pool)
    participant controlDB as controlDB<br/>(Control pool)

    Dispatcher->>Sink: WriteEvents (DML rows)
    Sink->>dmlDB: batched INSERT statements
    dmlDB-->>Sink: row count

    Dispatcher->>Sink: WriteBlockEvent (DDL)
    Sink->>controlDB: upsert ddl_ts table
    Sink->>controlDB: execute DDL statement
    Sink->>controlDB: update syncpoint/progress
    controlDB-->>Sink: completion

    Dispatcher->>Sink: Close()
    Sink->>dmlDB: close pool
    Sink->>controlDB: close pool
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐇 Two pools now flow where one once ran,
DML and DDL on separate plan.
No more DDL stuck behind the queue,
Each writer hops to its own path true!
The rabbit cheers — connections split with care! 🎉

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 28.57% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely summarizes the main change: splitting DML and control DB pools in the MySQL sink.
Linked Issues check ✅ Passed The PR successfully addresses issue #5360 by splitting connection pools to prevent DDL operations from being blocked by exhausted DML connections.
Out of Scope Changes check ✅ Passed All code changes are focused on the dual-pool connection architecture and remain within the scope of fixing the DDL starvation issue.
Description check ✅ Passed The PR description includes all required template sections with detailed explanations of the problem, solution, testing approach, compatibility impact, and release notes.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

Comment @coderabbitai help to get the list of available commands and usage tips.

@ti-chi-bot ti-chi-bot Bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 15, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request introduces separate database connection pools for DML and control-plane operations in the MySQL sink to prevent control-plane tasks from being blocked by long-lived DML sessions. A critical issue was identified in newMysqlConfigAndDB where returning nil for the named return parameter db on error causes a nil pointer dereference panic in the deferred cleanup function, leading to a database connection leak. A code suggestion was provided to capture the connection in a local variable before deferring the cleanup.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread pkg/sink/mysql/config.go
@hongyunyan
hongyunyan marked this pull request as ready for review June 15, 2026 03:49
@ti-chi-bot ti-chi-bot Bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 15, 2026
@ti-chi-bot ti-chi-bot Bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 15, 2026
@hongyunyan

Copy link
Copy Markdown
Collaborator Author

/test all

@hongyunyan

Copy link
Copy Markdown
Collaborator Author

/test pull-cdc-mysql-integration-light

@hongyunyan

Copy link
Copy Markdown
Collaborator Author

/test pull-cdc-mysql-integration-light

@hongyunyan

Copy link
Copy Markdown
Collaborator Author

/test pull-cdc-mysql-integration-light

@hongyunyan

Copy link
Copy Markdown
Collaborator Author

/test pull-cdc-mysql-integration-light

@ti-chi-bot ti-chi-bot Bot added needs-1-more-lgtm Indicates a PR needs 1 more LGTM. approved labels Jun 17, 2026
@ti-chi-bot ti-chi-bot Bot added the lgtm label Jun 17, 2026
@ti-chi-bot

ti-chi-bot Bot commented Jun 17, 2026

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED

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

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 Jun 17, 2026
@ti-chi-bot

ti-chi-bot Bot commented Jun 17, 2026

Copy link
Copy Markdown

[LGTM Timeline notifier]

Timeline:

  • 2026-06-17 02:53:03.817472689 +0000 UTC m=+1533284.887790069: ☑️ agreed by wk989898.
  • 2026-06-17 03:53:18.381659596 +0000 UTC m=+1536899.451976986: ☑️ agreed by lidezhu.

@ti-chi-bot
ti-chi-bot Bot merged commit 5880b63 into pingcap:master Jun 17, 2026
26 checks passed
@hongyunyan

Copy link
Copy Markdown
Collaborator Author

/cherry-pick release-8.5

@ti-chi-bot

Copy link
Copy Markdown
Member

@hongyunyan: new pull request created to branch release-8.5: #5763.
But this PR has conflicts, please resolve them!

Details

In response to this:

/cherry-pick release-8.5

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

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

Labels

approved lgtm release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

dispatcher may be stuck when flushing ddl

4 participants