[bug] proxy: session cannot be transferred after load data local infile

[volgariver6]

User description

What type of PR is this?

• API-change
• BUG
• Improvement
• Documentation
• Feature
• Test and CI
• Code Refactoring

Which issue(s) this PR fixes:

issue https://github.com/matrixorigin/mo-cloud/issues/4088

What this PR does / why we need it:

In the case of "load data local infile" statement, client sends the
first packet, then server sends response, which is "0xFB + filename",
after that, client sends content of filename and an empty packet, at
last, server sends OK packet. The sequence ID of this OK packet is not
1, and will cause the session cannot be transferred after this stmt finished.
So, the solution is: when server sends 0xFB and the sequence ID of
next packet is 3 bigger than last one, the next packet MUST be an
OK packet, and the transfer is allowed.

---

PR Type

Bug fix, Tests

---

Description

• Fixed a bug where session transfer was not possible after executing "load data local infile" due to incorrect sequence ID handling.
• Introduced a new logic to determine when a session transfer is allowed by checking the sequence ID difference.
• Added a new function isLoadDataLocalInfileRespPacket to identify specific server response packets.
• Enhanced test coverage by adding new test cases for the modified logic and new function.

---

Changes walkthrough 📝

	Relevant files
Bug fix	tunnel.go Fix session transfer issue for "load data local infile" statement pkg/proxy/tunnel.go Added logic to handle sequence ID for "load data local infile" statement. Introduced mustOK flag to manage session transfer conditions. Modified checkTxnStatus and handleOKPacket functions to accept mustOK parameter. +29/-8
Tests	tunnel_test.go Add tests for transaction status with mustOK flag                pkg/proxy/tunnel_test.go Added test cases for checkTxnStatus with mustOK parameter. Enhanced test coverage for transaction status checks. +60/-20  util_test.go Add tests for load data local infile response detection    pkg/proxy/util_test.go Added tests for isLoadDataLocalInfileRespPacket function. +14/-0
Enhancement	util.go Introduce function to detect load data local infile response pkg/proxy/util.go Added isLoadDataLocalInfileRespPacket function to identify specific response packets. +9/-0

---

💡 PR-Agent usage:
Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

[qodo-merge-pro]

PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

PR Reviewer Guide 🔍

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪

🧪 PR contains tests

🔒 No security concerns identified

⚡ Key issues to review Logic Complexity The new logic for handling "load data local infile" statements adds complexity to the kickoff function. Consider extracting this logic into a separate function for better readability and maintainability. Error Handling The new logic doesn't include explicit error handling for edge cases or unexpected packet sequences. Consider adding appropriate error checks and handling.

[qodo-merge-pro]

PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

PR Code Suggestions ✨

Category	Suggestion	Score
Possible issue	Add error handling for unexpected input lengths Consider adding error handling for the case when tempBuf is less than 4 bytes long, as accessing tempBuf[3] might cause a panic. pkg/proxy/tunnel.go [569-571] if len(tempBuf) > 3 { currSeq = int16(tempBuf[3]) +} else { + // Handle the case when tempBuf is too short + return false, moerr.NewInternalErrorf(errutil.ContextWithNoReport(ctx, true), + "tempBuf is too short: %d", len(tempBuf)) } Apply this suggestion Suggestion importance[1-10]: 9 Why: Adding error handling for cases where tempBuf is too short prevents potential runtime panics, addressing a possible issue in the code.	9
Maintainability	Extract complex logic into a separate function Consider extracting the logic for handling "load data local infile" into a separate function to improve readability and maintainability of the kickoff function. pkg/proxy/tunnel.go [524-528] var firstCond bool var currSeq int16 var lastSeq int16 = -1 var rotated bool +handleLoadDataLocalInfile := func() (mustOK bool) { + if !firstCond { + firstCond = isLoadDataLocalInfileRespPacket(tempBuf) + } else { + mustOK = currSeq-lastSeq == 3 + firstCond = false + } + return mustOK +} prepareNextMessage := func() (terminate bool, err error) { if terminate := func() bool { Apply this suggestion Suggestion importance[1-10]: 8 Why: Extracting the logic for handling "load data local infile" into a separate function enhances readability and maintainability by simplifying the kickoff function.	8
	Use a named constant for magic numbers in conditions Consider using a constant for the sequence ID difference (3) in the condition for mustOK. This would improve readability and maintainability. pkg/proxy/tunnel.go [596] -mustOK = currSeq-lastSeq == 3 +const loadDataLocalInfileSeqDiff = 3 +mustOK = currSeq-lastSeq == loadDataLocalInfileSeqDiff Apply this suggestion Suggestion importance[1-10]: 7 Why: Using a named constant for the sequence ID difference improves code readability and maintainability by making the purpose of the number clear.	7
Enhancement	Add test cases for edge cases in input data Consider adding test cases for edge cases in the TestCheckTxnStatus function, such as when the packet length is exactly 4 bytes or when it's empty. pkg/proxy/tunnel_test.go [694-697] func TestCheckTxnStatus(t *testing.T) { t.Run("mustOK false", func(t *testing.T) { inTxn, ok := checkTxnStatus(nil, false) require.True(t, ok) require.True(t, inTxn) + // Test with empty packet + inTxn, ok = checkTxnStatus([]byte{}, false) + require.True(t, ok) + require.True(t, inTxn) + + // Test with packet of exactly 4 bytes + inTxn, ok = checkTxnStatus([]byte{0, 0, 0, 0}, false) + require.True(t, ok) + require.True(t, inTxn) + Apply this suggestion Suggestion importance[1-10]: 8 Why: Including test cases for edge cases improves test coverage and ensures the function handles all possible input scenarios correctly.	8

[aylei]

lgtm