syncer(dm): option for explicit auto-id-cache handling in create table DDLs

[michaelmdeng]

What problem does this PR solve?

Issue Number: close #12039

What is changed and how it works?

Adds a new create-table-auto-id-cache-size Syncer config. create-table refers to the fact that this config affects CREATE TABLE statements, and when set, DM incremental replication of new tables will update the create table statement w/ the corresponding auto-id-cache size. When unset (0 or default), the statement will be left as is.

syncers:
  <name>:
    auto-id-cache-size: 1 # This sets MySQL-compatiblity mode for new tables
    ...

...

I believe this change should not affect other DM components, including import for "full" or "all" mode, but would appreciate guidance here as well.

Check List

Tests

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

Questions

Will it cause performance regression or break compatibility?

No

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

Yes, needs documentation on the new config setting and its purpose/usage.

Release note

Allow control of `AUTO_ID_CACHE` size when DM in incremental mode creates new tables

[ti-chi-bot]

Hi @michaelmdeng. Thanks for your PR.

I'm waiting for a pingcap member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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 kubernetes-sigs/prow repository.

[sre-bot]

All committers have signed the CLA.

[lance6716]

/ok-to-test

[michaelmdeng]

@lance6716 sorry about the delay, just managed to fix the relevant integration tests, can you take a look and also check if the release notes and any docs/education look ok?

[OliverS929]

Since a new configuration is added in here, could you also add this option to OpenAPI so we can keep it aligned on both ends?

[OliverS929]

/retest

[OliverS929]

@michaelmdeng Just a quick reminder — one of the two Git email addresses you used for commits in this PR hasn’t signed the CLA yet. Please make sure to sign it so we can proceed with the review. Thanks!

[OliverS929]

/cc @D3Hunter @lance6716 @GMHDBJD

[OliverS929]

/hold

[lance6716]

rest lgtm

[michaelmdeng]

@lance6716 Thanks for the review! addressed your feedback and made a couple of naming changes based on discussion in #12039

[lance6716]

/retest

[lance6716]

/retest

[ti-chi-bot]

[LGTM Timeline notifier]

Timeline:

• 2025-05-05 07:52:54.452793279 +0000 UTC m=+255221.635696537: ☑️ agreed by lance6716.

[michaelmdeng]

@lance6716 can I get help w/ approval for the dm/config changes?

[lance6716]

ptal @D3Hunter @GMHDBJD @OliverS929

[michaelmdeng]

@OliverS929 thanks for taking a look! wanted to tag you because we were specifically discussing this in the upstream issue

[OliverS929]

Hi @michaelmdeng, thanks for tagging me again. I noticed that my earlier comment about default value initialization in the syncer config hasn't received a response yet.

I believe it's important to ensure default values are properly set when creating a new sync subtask. Could you take a look and consider making the suggested changes?

For reference, please see
https://github.com/pingcap/tiflow/blob/master/cmd/dm-syncer/config.go#L63
and
https://github.com/pingcap/tiflow/blob/master/cmd/dm-syncer/config.go#L146

[michaelmdeng]

Hi @michaelmdeng, thanks for tagging me again. I noticed that my earlier comment about default value initialization in the syncer config hasn't received a response yet.

I believe it's important to ensure default values are properly set when creating a new sync subtask. Could you take a look and consider making the suggested changes?

sorry @OliverS929 , not sure I understand the request here. those are configs for the dm-syncer cmd/binary, which appears to me to be unused?

and when/if dm-syncer is run, it should initialize configs either here or from parsing the config toml file, which uses the default SubTaskConfig and SyncerConfig struct initialization which will set AutoIDCacheSize to 0, which is the desired default behavior.

[OliverS929]

Hi @michaelmdeng, thanks for tagging me again. I noticed that my earlier comment about default value initialization in the syncer config hasn't received a response yet.
I believe it's important to ensure default values are properly set when creating a new sync subtask. Could you take a look and consider making the suggested changes?

sorry @OliverS929 , not sure I understand the request here. those are configs for the dm-syncer cmd/binary, which appears to me to be unused?

and when/if dm-syncer is run, it should initialize configs either here or from parsing the config toml file, which uses the default SubTaskConfig and SyncerConfig struct initialization which will set AutoIDCacheSize to 0, which is the desired default behavior.

I understand that this is handled by Go’s struct defaulting mechanism. I was just hoping that AutoIDCacheSize (and similar fields) could be explicitly initialized with comments, so it's more obvious to future readers and maintainers. That said, I agree it doesn't affect the functionality of this PR.

[OliverS929]

It seems that the new config "auto-id-cache-size" does not have a default value attached to it when it is not included in the config. I think it would be great if we could have a explicit default value here that aligns with behavior from previous versions, for clarity purpose.

[OliverS929]

rest LGTM

[ti-chi-bot]

@OliverS929: adding LGTM is restricted to approvers and reviewers in OWNERS files.

In response to this:

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 kubernetes-sigs/prow repository.

[ti-chi-bot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: lance6716, OliverS929
Once this PR has been reviewed and has the lgtm label, please assign yudongusa for approval. For more information see the Code Review Process.
Please ensure that each of them provides their approval before proceeding.

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

Needs approval from an approver in each of these files:

• dm/OWNERS [lance6716]
• dm/config/OWNERS

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

[OliverS929]

/cc @alastori. Because this PR involves modification to DM config, I think it would need approval from you and Ben.

[alastori]

/cc @alastori. Because this PR involves modification to DM config, I think it would need approval from you and Ben.

@OliverS929, thanks for asking. The longer key name create-table-auto-id-cache-size looks great and matches what we agreed on in the original issue discussion (link).

@michaelmdeng Tiny nit: the sample YAML in the PR description still shows the old
auto-id-cache-size. Could you update that snippet (and any other docs/tests) to use the new name so everything stays consistent before we merge?

Thanks!