Skip to content

feat(rule): add rule to check for uncommitted transactions before DDL…#3215

Open
BugsGuru wants to merge 1 commit intomainfrom
feat-rule-check-transactions
Open

feat(rule): add rule to check for uncommitted transactions before DDL…#3215
BugsGuru wants to merge 1 commit intomainfrom
feat-rule-check-transactions

Conversation

@BugsGuru
Copy link
Collaborator

@BugsGuru BugsGuru commented Mar 6, 2026

关联的 issue

close https://github.com/actiontech/sqle-ee/issues/2383

描述你的变更

增加MySQL规则:DDL执行前存在事务未提交

确认项(pr提交后操作)

Tip

请在指定复审人之前,确认并完成以下事项,完成后✅


  • 我已完成自测
  • 我已记录完整日志方便进行诊断
  • 我已在关联的issue里补充了实现方案
  • 我已在关联的issue里补充了测试影响面
  • 我已确认了变更的兼容性,如果不兼容则在issue里标记 not_compatible
  • 我已确认了是否要更新文档,如果要更新则在issue里标记 need_update_doc

@LordofAvernus LordofAvernus requested review from iwanghc and removed request for iwanghc March 6, 2026 06:57
@github-actions
Copy link

github-actions bot commented Mar 6, 2026

PR Reviewer Guide 🔍

(Review updated until commit e3a5fb0)

🎫 Ticket compliance analysis ✅

2383 - Fully compliant

Compliant requirements:

  • 增加MySQL规则检查未提交事务
⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

代码健壮性

请仔细确认在处理不同MySQL版本时逻辑分支的错误处理和边界条件处理是否完全覆盖,确保版本判断及后续查询不会因异常数据导致问题。

type checkTransactionNotCommittedBeforeDDLTableInfo struct {
	Schema string
	Table  string
}

func (c checkTransactionNotCommittedBeforeDDLTableInfo) String() string {
	if c.Table == "" {
		return c.Schema
	}
	if c.Schema == "" {
		return c.Table
	}
	return fmt.Sprintf("%s.%s", c.Schema, c.Table)
}

func checkTransactionNotCommittedBeforeDDL(input *RuleHandlerInput) error {
	switch input.Node.(type) {
	case ast.DDLNode:
	default:
		return nil
	}

	tables := extractDDLSchemaAndTable(input.Node)
	if len(tables) == 0 {
		return nil
	}

	const transactionExecSecs = 600

	version, err := input.Ctx.GetMySQLMajorVersion()
	if err != nil {
		return err
	}

	for _, table := range tables {
		if table.Schema == "" {
			table.Schema = input.Ctx.CurrentSchema()
		}
		if table.Schema == "" {
			continue
		}

		if input.Ctx.IsLowerCaseTableName() {
			table.Schema, table.Table = strings.ToLower(table.Schema), strings.ToLower(table.Table)
		}

		if version >= 8 {
			count, err := input.Ctx.CheckTableRelatedTransactionNotCommittedMySQL8(table.Schema, table.Table)
			if err != nil {
				return err
			}
			if count > 0 {
				addResult(input.Res, input.Rule, input.Rule.Name,
					fmt.Sprintf("performance_schema.data_locks存在%d条%s的相关记录", count, table))
				return nil
			}
		} else {
			count, ExecTimeoutCount, err := input.Ctx.CheckTransactionNotCommittedMySQL5(transactionExecSecs)
			if err != nil {
				return err
			}
			if count > 0 || ExecTimeoutCount > 0 {
				addResult(input.Res, input.Rule, input.Rule.Name,
					fmt.Sprintf("information_schema.innodb_trx存在%d条记录, %d条执行时长超过%d秒", count, ExecTimeoutCount, transactionExecSecs))
				return nil
			}
		}
	}

	return nil
}
错误处理

在获取MySQL版本和执行SQL查询转换过程中,建议进一步验证返回数据的合法性和字段有效性,防止因异常值导致转换错误。

// GetMySQLMajorVersion 返回 MySQL 主版本号(如 5、8),离线或无 executor 时返回 0。
func (c *Context) GetMySQLMajorVersion() (int, error) {
	if c.e == nil {
		return 0, nil
	}
	results, err := c.e.Db.Query("SELECT @@version AS version")
	if err != nil {
		return 0, err
	}
	if len(results) == 0 {
		return 0, nil
	}
	v, ok := results[0]["version"]
	if !ok || !v.Valid {
		return 0, nil
	}
	parts := strings.SplitN(v.String, ".", 2)
	if len(parts) == 0 {
		return 0, nil
	}
	major, err := strconv.Atoi(parts[0])
	if err != nil {
		return 0, err
	}
	return major, nil
}

@github-actions
Copy link

github-actions bot commented Mar 6, 2026

PR Code Suggestions ✨

Latest suggestions up to e3a5fb0
Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
全面处理所有表

考虑遍历处理所有符合条件的表,而不是在发现第一个未提交事务记录后立即返回。这样可以确保检测结果更全面,并避免遗漏其他表的异常情况。

sqle/driver/mysql/rule/rule.go [4946-4953]

 for _, table := range tables {
-    ...
-    if count > 0 || ExecTimeoutCount > 0 {
-        addResult(input.Res, input.Rule, input.Rule.Name,
-            fmt.Sprintf("information_schema.innodb_trx存在%d条记录, %d条执行时长超过%d秒", count, ExecTimeoutCount, transactionExecSecs))
-        return nil
+    if table.Schema == "" {
+        table.Schema = input.Ctx.CurrentSchema()
+    }
+    if table.Schema == "" {
+        continue
+    }
+    if input.Ctx.IsLowerCaseTableName() {
+        table.Schema, table.Table = strings.ToLower(table.Schema), strings.ToLower(table.Table)
+    }
+    if version >= 8 {
+        count, err := input.Ctx.CheckTableRelatedTransactionNotCommittedMySQL8(table.Schema, table.Table)
+        if err != nil {
+            return err
+        }
+        if count > 0 {
+            addResult(input.Res, input.Rule, input.Rule.Name,
+                fmt.Sprintf("performance_schema.data_locks存在%d条%s的相关记录", count, table))
+        }
+    } else {
+        count, ExecTimeoutCount, err := input.Ctx.CheckTransactionNotCommittedMySQL5(transactionExecSecs)
+        if err != nil {
+            return err
+        }
+        if count > 0 || ExecTimeoutCount > 0 {
+            addResult(input.Res, input.Rule, input.Rule.Name,
+                fmt.Sprintf("information_schema.innodb_trx存在%d条记录, %d条执行时长超过%d秒", count, ExecTimeoutCount, transactionExecSecs))
+        }
     }
 }
+return nil
Suggestion importance[1-10]: 8

__

Why: The suggestion addresses a logical flaw by removing the premature "return nil" inside the loop, ensuring that all tables are processed. This improvement fixes a potential bug where later tables could be skipped.

Medium

Previous suggestions

Suggestions up to commit d82104f
CategorySuggestion                                                                                                                                    Impact
Possible issue
全表事务检测改进


建议不要在检测到第一个存在未提交事务的表后立即返回,而是遍历所有相关表,收集所有问题信息。这样可以确保所有可能存在的问题都被报告,帮助运维人员全面了解DDL操作前的事务状态。

sqle/driver/mysql/rule/rule.go [4934-4967]

 for _, table := range tables {
-    ...
+    if table.Schema == "" {
+        table.Schema = input.Ctx.CurrentSchema()
+    }
+    if table.Schema == "" {
+        continue
+    }
+    if input.Ctx.IsLowerCaseTableName() {
+        table.Schema, table.Table = strings.ToLower(table.Schema), strings.ToLower(table.Table)
+    }
     if version >= 8 {
         count, err := input.Ctx.CheckTableRelatedTransactionNotCommittedMySQL8(table.Schema, table.Table)
         if err != nil {
             return err
         }
         if count > 0 {
             addResult(input.Res, input.Rule, input.Rule.Name,
                 fmt.Sprintf("performance_schema.data_locks存在%d条%s的相关记录", count, table))
-            return nil
         }
     } else {
         count, ExecTimeoutCount, err := input.Ctx.CheckTransactionNotCommittedMySQL5(transactionExecSecs)
         if err != nil {
             return err
         }
         if count > 0 || ExecTimeoutCount > 0 {
             addResult(input.Res, input.Rule, input.Rule.Name,
                 fmt.Sprintf("information_schema.innodb_trx存在%d条记录, %d条执行时长超过%d秒", count, ExecTimeoutCount, transactionExecSecs))
-            return nil
         }
     }
 }
+return nil
Suggestion importance[1-10]: 7

__

Why: The suggestion improves the error reporting by avoiding an early return upon detecting a single uncommitted transaction, allowing the loop to process all tables. This change enhances overall issue visibility, though altering the control flow may have side effects, which limits its score to a moderate impact.

Medium

@BugsGuru BugsGuru force-pushed the feat-rule-check-transactions branch from d82104f to e3a5fb0 Compare March 6, 2026 07:14
@github-actions
Copy link

github-actions bot commented Mar 6, 2026

Persistent review updated to latest commit e3a5fb0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant