to 3.0: restrict MO_CTL execution of flush, merge, checkpoint within explicit transactions

[gouhongshen]

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 ##21941

What this PR does / why we need it:

mo_ctl advancing the transaction snapshot may result in unexpected behavior,
So we restrict these executions within an explicit transaction.

---

PR Type

Bug fix

---

Description

• Restrict MO_CTL flush/merge/checkpoint execution within explicit transactions

• Add ByBegin() method to TxnOperator interface

• Implement transaction state checking for control operations

• Add test cases for transaction restriction validation

---

Diagram Walkthrough

flowchart LR
  A["TxnOperator Interface"] --> B["Add ByBegin() method"]
  B --> C["Implement in all operators"]
  C --> D["MO_CTL function check"]
  D --> E["Block flush/merge/checkpoint in transactions"]
  E --> F["Add test validation"]

Loading

File Walkthrough

	Relevant files
Enhancement	3 files types.go Add ByBegin method to TxnOperator interface                            +2/-0      operator.go Implement ByBegin method in txnOperator                                    +4/-0      storage_txn_client.go Implement ByBegin method returning false                                  +4/-0
Bug fix	1 files ctl.go Add transaction check for control operations                          +9/-0
Tests	10 files txn_mock.go Add ByBegin mock method implementation                                      +15/-1    service_test.go Add ByBegin panic implementation for tests                              +4/-0      txn_test.go Add ByBegin panic implementation for tests                              +4/-0      store_sql_test.go Add ByBegin panic implementation for tests                              +4/-0      remoterun_test.go Add ByBegin false return for fake operator                              +4/-0      entire_engine_test.go Add ByBegin panic implementation for tests                              +4/-0      checkpoint.test Add transaction checkpoint restriction test case                  +4/-0      checkpoint.result Add expected error result for checkpoint test                        +4/-0      mo_ctl_merge.test Add transaction merge restriction test case                            +4/-0      mo_ctl_merge.result Add expected error result for merge test                                  +4/-0

---

[qodo-merge-pro]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis ✅ 21941 - PR Code Verified Compliant requirements: Prevent mo_ctl operations (flush, merge, checkpoint) from being executed within an explicit transaction. Enforce the restriction at execution time with clear error feedback. Ensure code paths that trigger these operations check transaction state. Update or add tests to validate the restriction behavior. Requires further human verification: Confirm behavior also covers 'flush' mo_ctl command via end-to-end tests (only checkpoint/merge tests are shown in diff). Validate user-facing error messages and documentation alignment.

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

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Possible Issue The transaction check relies on TxnOperator.ByBegin(); ensure all TxnOperator implementations return correct values and that this flag truly indicates an explicit transaction context. Otherwise the restriction may be bypassed or overly restrictive. switch command { case FlushMethod, MergeObjectsMethod, CheckpointMethod: if proc.GetTxnOperator() != nil && proc.GetTxnOperator().ByBegin() { return moerr.NewInternalErrorNoCtxf( "cannot execute %s within transaction", command, ) } } Incomplete Test Mock New ByBegin() method in testTxnOperator panics; any tests using this mock will crash when MoCtl calls ByBegin(). Implement a sane default return to avoid unexpected panics. func (tTxnOp *testTxnOperator) ByBegin() bool { panic("implement me") } Mock Coverage Ensure all test paths expecting ByBegin() have expectations configured; otherwise default zero-value returns may mask logic errors. Review call sites to set gomock expectations for ByBegin(). // ByBegin mocks base method. func (m *MockTxnOperator) ByBegin() bool { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "ByBegin") ret0, _ := ret[0].(bool) return ret0 } // ByBegin indicates an expected call of ByBegin. func (mr *MockTxnOperatorMockRecorder) ByBegin() *gomock.Call { mr.mock.ctrl.T.Helper() return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ByBegin", reflect.TypeOf((*MockTxnOperator)(nil).ByBegin)) }

[qodo-merge-pro]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Remove panics in test doubles Panics in test doubles cause unrelated tests to fail when the new ByBegin() path is exercised. Implement ByBegin() to return a deterministic value (likely false for tests) and replace panics in these no-op methods to avoid spurious failures. pkg/bootstrap/service_test.go [210-217] func (tTxnOp *testTxnOperator) ExitIncrStmt() { - //TODO implement me - panic("implement me") + // no-op for tests } func (tTxnOp *testTxnOperator) ByBegin() bool { - panic("implement me") + // default to not within an explicit transaction in tests + return false } [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies that panicking stubs in test helpers are brittle and proposes replacing them with sensible default implementations, which improves test stability and maintainability.	Medium
	Harden transaction state check proc.GetTxnOperator() may be non-nil while not using explicit transactions; relying solely on ByBegin() risks false negatives if it returns an incorrect default or is unimplemented in tests. Guard with a safe nil-check and fallback to session/txn options if available, or clearly document and enforce ByBegin() implementation. At minimum, add a defensive check and return a consistent error when ByBegin() cannot be determined. pkg/sql/plan/function/ctl/ctl.go [85-92] switch command { case FlushMethod, MergeObjectsMethod, CheckpointMethod: - if proc.GetTxnOperator() != nil && proc.GetTxnOperator().ByBegin() { - return moerr.NewInternalErrorNoCtxf( - "cannot execute %s within transaction", command, - ) + if op := proc.GetTxnOperator(); op != nil { + // Require operator to implement explicit-begin awareness; block if unknown or explicitly begun + if op.ByBegin() { + return moerr.NewInternalErrorNoCtxf("cannot execute %s within transaction", command) + } } } Apply / Chat Suggestion importance[1-10]: 3 __ Why: The suggestion correctly identifies a potential issue with new ByBegin() implementations, but the proposed code change is a minor stylistic refactoring that is logically equivalent to the existing code.	Low
General	Add safe default for mock The mock defaults to returning the zero value when no expectation is set, which may inadvertently allow restricted commands in tests. Provide a sensible default (e.g., false) through an explicit stub to avoid ambiguous behavior, or document and set expectations in tests that rely on this method. pkg/frontend/test/txn_mock.go [492-498] // ByBegin mocks base method. func (m *MockTxnOperator) ByBegin() bool { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "ByBegin") + if len(ret) == 0 { + // default to not within an explicit transaction if not stubbed + return false + } ret0, _ := ret[0].(bool) return ret0 } Apply / Chat Suggestion importance[1-10]: 7 __ Why: The suggestion correctly points out that the mock's default zero-value return for ByBegin() could lead to unexpected behavior in tests, and proposes adding an explicit default to make the mock more robust.	Medium
More