Repository navigation
session,txn: support file-based transaction (txn file) for large transactions - #70468
Conversation
Signed-off-by: Ping Yu <yuping@pingcap.com>
Signed-off-by: Ping Yu <yuping@pingcap.com>
Signed-off-by: Ping Yu <yuping@pingcap.com>
Signed-off-by: Ping Yu <yuping@pingcap.com>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Signed-off-by: Ping Yu <yuping@pingcap.com>
ti-chi-bot
Bot
added
release-note
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan includes up to 10 reviews per rolling hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe change adds transaction-file system variables and validation, refreshes Go dependencies, and adds a Docker Compose starter environment. The environment builds and runs TiDB, TiKV, PD, MinIO, and transaction-file integration tests. ChangesTransaction-file support
Estimated code review effort: 4 (Complex) | ~60 minutes Merge Risk: ⚪ Minimal · up to This change adds configurable file-based transaction handling for large transactions while preserving the existing path for smaller transactions; no actionable merge-blocking risk remains beyond normal checks and review. Sequence Diagram(s)sequenceDiagram
participant DockerComposeRunner
participant MinIO
participant PD
participant TiKVCluster
participant TiDB
participant StarterTests
DockerComposeRunner->>MinIO: Start and initialize the bucket
DockerComposeRunner->>PD: Start and create the startertest keyspace
DockerComposeRunner->>TiKVCluster: Start three TiKV nodes and the worker
DockerComposeRunner->>TiDB: Build, bootstrap, and activate TiDB
StarterTests->>TiDB: Run transaction-file integration tests
DockerComposeRunner->>StarterTests: Capture results and collect diagnostics
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description follows the required template. It includes the issue number, problem summary, implementation details, test checklist, side effects, documentation impact, and a release note. The description should correct the inconsistent variable name: the changes add tidb_disable_txn_file, not tidb_enable_txn_file. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
ti-chi-bot
Bot
added
the
do-not-merge/work-in-progress
ti-chi-bot
Bot
removed
the
do-not-merge/work-in-progress
|
/hold |
ti-chi-bot
Bot
added
the
do-not-merge/hold
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #70468 +/- ##
================================================
- Coverage 76.3195% 74.5304% -1.7892%
================================================
Files 2041 2103 +62
Lines 557578 586111 +28533
================================================
+ Hits 425541 436831 +11290
- Misses 131137 147457 +16320
- Partials 900 1823 +923
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
tests/realtikvtest/startertest/txn_file_test.go (1)
136-151: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDecouple the region scan from the positional column layout.
queryTxnFileRegionIDsscans 13 columns and discards 10 throughvar ignored [10]any. Read the required columns by name withrows.Columns()and scan all returned columns into a dynamically sized destination. This prevents added columns from causing a column-count mismatch.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/realtikvtest/startertest/txn_file_test.go` around lines 136 - 151, Update queryTxnFileRegionIDs to obtain column names via rows.Columns(), locate region_id, start_key, and end_key by name, and scan every returned column into a dynamically sized destination. Remove the fixed ignored array and positional 13-column assumptions while preserving the existing ID assignment logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/dxf/importinto/conflictedkv/collector_test.go`:
- Line 120: Update the codec initialization in GetCodec to handle the error
returned by tikv.NewCodecV2 instead of discarding it. If creation fails, panic
with a contextual error that preserves the underlying failure; otherwise
continue returning the successfully initialized codec.
In `@tests/realtikvtest/startertest/docker-compose/README.md`:
- Around line 111-115: Replace the incomplete fenced shell command in the
exit-trap documentation with prose describing the project-scoped cleanup, or
provide the full executable docker compose command including its project
configuration. Update the statement that currently claims the trap runs the
command exactly, keeping the cleanup options unchanged.
In `@tests/realtikvtest/startertest/docker-compose/run.sh`:
- Around line 166-175: Update the exit_code handling in wait_completed so only a
non-empty numeric value equal to 0 returns success; treat empty or non-numeric
values from docker inspect as failures, while preserving the existing nonzero
exit-code error path.
In `@tests/realtikvtest/startertest/txn_file_conflict_test.go`:
- Around line 1-30: Regenerate and commit Bazel metadata with make
bazel_prepare. In tests/realtikvtest/startertest/BUILD.bazel, ensure
txn_file_test.go, txn_file_conflict_test.go, and txn_file_metrics_test.go are
listed and prometheus/client_model plus prometheus/common dependencies are
declared; in pkg/sessionctx/variable/BUILD.bazel, update metadata for
TestTxnFileSysVars and the current shard/test count.
---
Nitpick comments:
In `@tests/realtikvtest/startertest/txn_file_test.go`:
- Around line 136-151: Update queryTxnFileRegionIDs to obtain column names via
rows.Columns(), locate region_id, start_key, and end_key by name, and scan every
returned column into a dynamically sized destination. Remove the fixed ignored
array and positional 13-column assumptions while preserving the existing ID
assignment logic.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a7eb9ba2-5883-4d70-a45c-a712362e6e1a
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (66)
DEPS.bzlbr/pkg/restore/split/split_test.gobr/pkg/rtree/rtree_test.gobr/pkg/task/meta_service_group_test.gobr/pkg/task/operator/crr_checkpoint_test.gobr/pkg/task/restore_nokit_test.gogo.modlightning/pkg/importer/meta_service_group_test.gopkg/autoid_service/autoid.gopkg/autoid_service/autoid_test.gopkg/ddl/label/rule_test.gopkg/domain/crossks/cross_ks_test.gopkg/domain/infosync/label_manager_test.gopkg/dxf/importinto/collect_conflicts_internal_test.gopkg/dxf/importinto/conflictedkv/collector_test.gopkg/dxf/importinto/conflictedkv/deleter_test.gopkg/dxf/importinto/job_testkit_test.gopkg/executor/importer/meta_service_group_test.gopkg/executor/infoschema_reader.gopkg/executor/infoschema_reader_keyspace_test.gopkg/executor/sample_test.gopkg/executor/test/infoschema/infoschema_test.gopkg/extworkload/client/client.gopkg/extworkload/manager_test.gopkg/ingestor/ingestctrl/duplicate_test.gopkg/meta/autoid/autoid_service.gopkg/metaservice/etcd_test.gopkg/server/handler/tests/dxf_test.gopkg/session/bootstrap_test.gopkg/session/session_test.gopkg/session/starter_bootstrap_file_test.gopkg/sessionctx/vardef/tidb_vars.gopkg/sessionctx/variable/sysvar.gopkg/sessionctx/variable/sysvar_test.gopkg/store/etcd_test.gopkg/store/gcworker/gc_worker_test.gopkg/store/helper/helper_test.gopkg/store/mockstore/mockcopr/executor_test.gopkg/store/mockstore/mockstore.gopkg/store/mockstore/unistore/cophandler/cop_handler.gopkg/store/mockstore/unistore/cophandler/mpp.gopkg/store/mockstore/unistore/cophandler/mpp_exec.gopkg/store/mockstore/unistore/pd.gopkg/store/mockstore/unistore/pd_test.gopkg/tablecodec/tablecodec_test.gopkg/testkit/mockstore.gotests/realtikvtest/scripts/next-gen/README.mdtests/realtikvtest/scripts/next-gen/run-starter-tests-with-server.shtests/realtikvtest/startertest/BUILD.bazeltests/realtikvtest/startertest/docker-compose/Dockerfile.testtests/realtikvtest/startertest/docker-compose/Dockerfile.test.dockerignoretests/realtikvtest/startertest/docker-compose/README.mdtests/realtikvtest/startertest/docker-compose/bootstrap/bootstrap-system.shtests/realtikvtest/startertest/docker-compose/bootstrap/create-keyspace.shtests/realtikvtest/startertest/docker-compose/bootstrap/init-minio.shtests/realtikvtest/startertest/docker-compose/configs/pd.tomltests/realtikvtest/startertest/docker-compose/configs/starter.tomltests/realtikvtest/startertest/docker-compose/configs/tikv-1.tomltests/realtikvtest/startertest/docker-compose/configs/tikv-2.tomltests/realtikvtest/startertest/docker-compose/configs/tikv-3.tomltests/realtikvtest/startertest/docker-compose/configs/tikv-worker.tomltests/realtikvtest/startertest/docker-compose/docker-compose.ymltests/realtikvtest/startertest/docker-compose/run.shtests/realtikvtest/startertest/txn_file_conflict_test.gotests/realtikvtest/startertest/txn_file_metrics_test.gotests/realtikvtest/startertest/txn_file_test.go
Signed-off-by: Ping Yu <yuping@pingcap.com>
Signed-off-by: Ping Yu <yuping@pingcap.com>
pingyu
changed the title
Signed-off-by: Ping Yu <yuping@pingcap.com> # Conflicts: # DEPS.bzl # go.mod # go.sum # pkg/dxf/importinto/conflictedkv/collector_test.go # pkg/extworkload/client/client.go
| }, GetGlobal: func(_ context.Context, _ *SessionVars) (string, error) { | ||
| return BoolToOnOff(vardef.EnableConnectionEventLog.Load()), nil | ||
| }}, | ||
| {Scope: vardef.ScopeGlobal | vardef.ScopeSession, Name: vardef.TiDBDisableTxnFile, Value: BoolToOnOff(vardef.DefTiDBDisableTxnFile), Type: vardef.TypeBool, SetSession: func(s *SessionVars, val string) error { |
There was a problem hiding this comment.
Could we use the positive form tidb_enable_txn_file instead? tidb_disable_txn_file=OFF is a double negative and means txn-file is allowed by default once the chunk writer is configured. Is this default-on behavior intentional? If explicit opt-in is preferred for this experimental feature, consider tidb_enable_txn_file=OFF; otherwise, tidb_enable_txn_file=ON preserves the current behavior with clearer semantics.
There was a problem hiding this comment.
tidb_disable_txn_file is necessary to compat with existing customers using Essential v1.
There was a problem hiding this comment.
@pingyu Could you explain a little bit more on how it is necessary?
There was a problem hiding this comment.
Essential v1 (TiDB-CSE) is using tidb_disable_txn_file currently. So if we change the variable, it will be difficult to upgrade to TiDB nextgen.
| } | ||
|
|
||
| func enableTxnFileSession(ctx context.Context, conn *sql.Conn) error { | ||
| statement := "SET SESSION tidb_txn_mode='optimistic', SESSION tidb_txn_assertion_level='OFF', SESSION tidb_disable_txn_file=OFF, SESSION tidb_txn_file_min_mutation_size=1048576" |
There was a problem hiding this comment.
These two settings appear to be required for txn-file, rather than test-only setup: client-go rejects pessimistic transactions and mutations carrying strict assertions, while NextGen defaults to pessimistic DML and STRICT assertions. As a result, ordinary UPDATE/DELETE transactions using the default settings will not take the txn-file path even after reaching the threshold. Please either support the default transaction settings or explicitly document the limitation and cover the default behavior.
There was a problem hiding this comment.
Mutations carrying strict assertions will be supported in next client-go PR.
| github.com/stretchr/testify v1.11.1 | ||
| github.com/tiancaiamao/appdash v0.0.0-20181126055449-889f96f722a2 | ||
| github.com/tikv/client-go/v2 v2.0.8-0.20260807103401-b75f6efa8ddd | ||
| github.com/tikv/client-go/v2 v2.0.8-0.20260811081704-86fe4e58c8a7 |
There was a problem hiding this comment.
Once txn-file is selected, KVTxn.Commit returns executeTxnFile errors directly; the dependency still has a TODO for falling back when tikv-worker is unavailable. Chunk uploads are retried, but if those retries are exhausted during buildTxnFiles, the transaction fails without trying normal 2PC even though txn-file prewrite has not started. Since #70467 lists fallback as required, is this intentionally deferred? If so, please track and document the limitation; otherwise, could we add a safe fallback for failures before txn-file prewrite?
There was a problem hiding this comment.
Fallback to normal 2PC when tikv-worker is unavailable will be supported in next client-go PR.
| mkdir -p /opt/tidb/bin && \ | ||
| mv bin/tidb-server /opt/tidb/bin/tidb-server && \ | ||
| mv /tmp/startertest.test /opt/tidb/bin/startertest.test && \ | ||
| git rev-parse HEAD > /opt/tidb/source-commit && \ |
There was a problem hiding this comment.
This build fails from a linked worktree: .git is a file containing an absolute host gitdir, so after COPY . ., git rev-parse points to a path that does not exist in the build container. Could run.sh pass the commit/status as build arguments or regular files instead, and exclude .git/.wt from the build context?
Signed-off-by: Ping Yu <yuping@pingcap.com>
|
/retest |
|
@wfxr: adding LGTM is restricted to approvers and reviewers in OWNERS files. DetailsIn 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. |
|
The changes LGTM except |
|
/retest |
Signed-off-by: Ping Yu <yuping@pingcap.com> # Conflicts: # DEPS.bzl # go.mod # go.sum
Signed-off-by: Ping Yu <yuping@pingcap.com>
|
/retest |
1 similar comment
|
/retest |
yudongusa
left a comment
yudongusa
left a comment
There was a problem hiding this comment.
Please also include this in the document PR
ti-chi-bot
Bot
added
approved
lgtm
and removed
needs-1-more-lgtm
|
/retest |
Signed-off-by: Ping Yu <yuping@pingcap.com>
|
/test pull-br-integration-test |
|
/retest |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bb7133, v01dstar, wfxr, yudongusa The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
6 participants
What problem does this PR solve?
Issue Number: close #70467
Problem Summary:
Large transactions are expensive in TiDB today: all mutations are buffered in TiDB memory and then prewritten/committed to TiKV through normal 2PC, which puts heavy cost and load on TiKV for big transactions (>= 16 MiB), forcing tight transaction size limits. Bulk-load / backfill-style workloads that naturally produce huge write batches cannot be expressed as a single atomic transaction.
This PR supports file-based transactions (txn file) on the TiDB side, building on the client-side support merged in tikv/client-go#1119. For transactions whose mutation size reaches a configurable threshold, mutations are built into chunks on an external chunk writer and ingested into TiKV, instead of being kept in memory and prewritten directly.
What changed and how does it work?
Bump
tikv/client-goto a revision that includes the file-based transaction implementation (tikv/client-go#1119).Add system variables to control the feature:
tidb_enable_txn_file(global/session, default OFF): switch to enable file-based transactions.tidb_txn_file_min_mutation_size(global/session, default 0, i.e. follow the client-go default; must be 0 or >= 1 MiB): the minimum mutation size for a transaction to use the file-based path.Wire the variables into session KV variables so they are passed down to the client-go transaction options; transactions at or above the threshold go through the txn-file path, smaller transactions behave exactly as today.
Add real-tikv starter tests under
tests/realtikvtest/startertestcovering txn-file semantics, conflict behavior, and metrics, together with a docker-compose based runner (tests/realtikvtest/startertest/docker-compose) that brings up PD/TiKV/tikv-worker and a starter server with a txn chunk writer for the tests.Depends on *: update kvproto and adapt keyspace fields #69942
Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit
New Features
Documentation
Tests