txn: also update the `@@tidb_last_txn_info` for readyonly or rollback txns #61057

lcwangchao · 2025-05-09T09:53:17Z

What problem does this PR solve?

Issue Number: close #61056

What changed and how does it work?

also update the @@tidb_last_txn_info for readyonly or rollback transactions

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No need to test
- I checked and no code files have been changed.

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility

Documentation

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

@@tidb_last_txn_info will also be updated when readonly transaction ends or transaction rollback

lcwangchao · 2025-05-09T09:53:35Z

/hold

codecov · 2025-05-09T10:17:17Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 73.5264%. Comparing base (f4d2318) to head (d467fda).
Report is 19 commits behind head on master.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #61057        +/-   ##
================================================
+ Coverage   73.1387%   73.5264%   +0.3876%     
================================================
  Files          1724       1724                
  Lines        477000     480744      +3744     
================================================
+ Hits         348872     353474      +4602     
+ Misses       106705     105846       -859     
- Partials      21423      21424         +1

Flag	Coverage Δ
integration	`42.8106% <100.0000%> (?)`
unit	`72.4814% <100.0000%> (+0.1140%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
dumpling	`52.7804% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`46.7415% <ø> (-0.8126%)`	⬇️

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

ekexium · 2025-05-09T10:21:52Z

pkg/session/session.go

@@ -919,6 +919,7 @@ func (s *session) CommitTxn(ctx context.Context) error {
 	r, ctx := tracing.StartRegionEx(ctx, "session.CommitTxn")
 	defer r.End()

+	s.setLastTxnInfoBeforeTxnEnd()


How about putting it in onTxnEnd?

Seems onTxnEnd，the txn info has been cleared

After this line, in doCommitWithRetry -> doCommit, it seems it still have possibility to finally enter a rolled-back state or encounter error. Perhaps it's better to consider put it in a later place where the commit/rollback result is finally determined.

Seems if a txn comitted or fails to commit, the CommitHook will update it:

tidb/pkg/sessiontxn/isolation/base.go

Line 506 in c71ebaf

txn.SetOption(kv.CommitHook, func(info string, _ error) { sessVars.LastTxnInfo = info })

setLastTxnInfoBeforeTxnEnd in CommitTxn will be overridden at this time. So setLastTxnInfoBeforeTxnEnd is only affects some readonly txns.

Better to add comments to explain why it is put here.

… txns

lcwangchao · 2025-05-09T14:04:26Z

/retest

MyonKeminta · 2025-05-12T07:49:16Z

pkg/session/session.go

@@ -919,6 +919,7 @@ func (s *session) CommitTxn(ctx context.Context) error {
 	r, ctx := tracing.StartRegionEx(ctx, "session.CommitTxn")
 	defer r.End()

+	s.setLastTxnInfoBeforeTxnEnd()


After this line, in doCommitWithRetry -> doCommit, it seems it still have possibility to finally enter a rolled-back state or encounter error. Perhaps it's better to consider put it in a later place where the commit/rollback result is finally determined.

pkg/session/session.go

lcwangchao · 2025-05-12T14:05:20Z

/retest

lcwangchao · 2025-05-13T09:34:01Z

/unhold

pkg/session/session.go

Co-authored-by: cfzjywxk <[email protected]>

cfzjywxk

LGTM

cfzjywxk · 2025-05-13T11:21:24Z

pkg/session/session.go

@@ -919,6 +919,7 @@ func (s *session) CommitTxn(ctx context.Context) error {
 	r, ctx := tracing.StartRegionEx(ctx, "session.CommitTxn")
 	defer r.End()

+	s.setLastTxnInfoBeforeTxnEnd()


Better to add comments to explain why it is put here.

ti-chi-bot · 2025-05-13T13:21:57Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cfzjywxk, zyguan

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [cfzjywxk,zyguan]

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 · 2025-05-13T13:22:01Z

[LGTM Timeline notifier]

Timeline:

2025-05-13 11:21:40.306737111 +0000 UTC m=+444166.454698358: ☑️ agreed by cfzjywxk.
2025-05-13 13:21:59.834890015 +0000 UTC m=+451385.982851234: ☑️ agreed by zyguan.

ti-chi-bot bot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label May 9, 2025

lcwangchao added the compatibility-breaker Violation of forwards/backwards compatibility in a design-time piece. label May 9, 2025

ti-chi-bot bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 9, 2025

ti-chi-bot bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 9, 2025

ekexium reviewed May 9, 2025

View reviewed changes

lcwangchao force-pushed the last_txn_info branch from b919335 to 12f751e Compare May 9, 2025 13:11

txn: also update the @@tidb_last_txn_info for readyonly or rollback…

dfb71cb

… txns

lcwangchao force-pushed the last_txn_info branch from 12f751e to dfb71cb Compare May 9, 2025 13:14

MyonKeminta reviewed May 12, 2025

View reviewed changes

use json to marshal

0a408fa

lcwangchao force-pushed the last_txn_info branch from 86b5f8b to 0a408fa Compare May 13, 2025 00:33

cfzjywxk requested a review from zyguan May 13, 2025 09:26

lcwangchao removed the compatibility-breaker Violation of forwards/backwards compatibility in a design-time piece. label May 13, 2025

ti-chi-bot bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 13, 2025

cfzjywxk reviewed May 13, 2025

View reviewed changes

pkg/session/session.go Outdated Show resolved Hide resolved

pkg/session/session.go Show resolved Hide resolved

Update pkg/session/session.go

d467fda

Co-authored-by: cfzjywxk <[email protected]>

cfzjywxk approved these changes May 13, 2025

View reviewed changes

ti-chi-bot bot added approved needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels May 13, 2025

zyguan approved these changes May 13, 2025

View reviewed changes

ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels May 13, 2025

ti-chi-bot bot merged commit 41045f1 into pingcap:master May 13, 2025
24 checks passed

txn: also update the @@tidb_last_txn_info for readyonly or rollback txns #61057

txn: also update the @@tidb_last_txn_info for readyonly or rollback txns #61057

Uh oh!

Conversation

lcwangchao commented May 9, 2025

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

Uh oh!

lcwangchao commented May 9, 2025

Uh oh!

codecov bot commented May 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ekexium May 9, 2025

Choose a reason for hiding this comment

Uh oh!

lcwangchao May 9, 2025

Choose a reason for hiding this comment

Uh oh!

MyonKeminta May 12, 2025

Choose a reason for hiding this comment

Uh oh!

lcwangchao May 12, 2025

Choose a reason for hiding this comment

Uh oh!

cfzjywxk May 13, 2025

Choose a reason for hiding this comment

Uh oh!

lcwangchao commented May 9, 2025

Uh oh!

MyonKeminta May 12, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lcwangchao commented May 12, 2025

Uh oh!

lcwangchao commented May 13, 2025

Uh oh!

Uh oh!

Uh oh!

cfzjywxk left a comment

Choose a reason for hiding this comment

Uh oh!

cfzjywxk May 13, 2025

Choose a reason for hiding this comment

Uh oh!

ti-chi-bot bot commented May 13, 2025

Uh oh!

ti-chi-bot bot commented May 13, 2025

[LGTM Timeline notifier]

Uh oh!

Uh oh!

Uh oh!

txn: also update the `@@tidb_last_txn_info` for readyonly or rollback txns #61057

txn: also update the `@@tidb_last_txn_info` for readyonly or rollback txns #61057

codecov bot commented May 9, 2025 •

edited

Loading