Copilot commented on code in PR #1133:
URL:
https://github.com/apache/incubator-seata-go/pull/1133#discussion_r3538572947
##########
pkg/datasource/sql/undo/executor/mysql_undo_insert_executor.go:
##########
@@ -58,12 +58,15 @@ func (m *mySQLUndoInsertExecutor) ExecuteOn(ctx
context.Context, dbType types.DB
defer stmt.Close()
afterImage := m.sqlUndoLog.AfterImage
for _, row := range afterImage.Rows {
- pkValueList := make([]interface{}, 0)
+ pkList, err := util.GetOrderedPkList(afterImage, row, dbType)
+ if err != nil {
+ return err
+ }
+
Review Comment:
ExecuteOn now relies on util.GetOrderedPkList to produce the PK values in
deterministic order, but GetOrderedPkList currently matches PK columns using
substring logic (`strings.Index(col.ColumnName, pkName) > -1`). For composite
keys like (tenant_id, id), pkName "id" will also match column "tenant_id",
causing duplicated/out-of-order PK fields and a mismatch between SQL
placeholders and exec args (or incorrect row targeting). GetOrderedPkList
should use exact (preferably case-insensitive) equality when matching column
names, and should avoid producing duplicates.
##########
pkg/datasource/sql/undo/executor/utils.go:
##########
@@ -72,22 +72,24 @@ func rowListToMap(rows []types.RowImage, primaryKeyList
[]string) map[string]map
for _, row := range rows {
fieldMap := make(map[string]interface{}, 0)
var rowKey string
- var firstUnderline bool
+ pkValues := make(map[string]interface{})
Review Comment:
rowListToMap now builds rowKey using pkValues[key] for every PK name. If a
PK column isn't present in the row image (e.g., due to escaping/case mismatch
or malformed images), pkValues[key] is nil, so rowKey includes "<nil>" and can
cause key collisions/row overwrites in rowMap (breaking IsRecordsEquals/data
validation). Consider falling back to a unique sentinel per row+PK when a PK
value is missing so the map remains collision-free and comparisons fail safely.
##########
pkg/datasource/sql/exec/at/insert_executor.go:
##########
@@ -722,11 +722,10 @@ func (i *insertExecutor) getPkValuesByAuto(ctx
context.Context, execCtx *types.E
}
func canAutoIncrement(pkMetaMap map[string]types.ColumnMeta) bool {
- if len(pkMetaMap) != 1 {
- return false
- }
for _, meta := range pkMetaMap {
- return meta.Autoincrement
+ if meta.Autoincrement {
+ return true
+ }
}
return false
}
Review Comment:
This PR updates canAutoIncrement here to support composite PKs, but there is
a second canAutoIncrement implementation in
pkg/datasource/sql/undo/builder/mysql_insert_undo_log_builder.go that still
returns false unless len(pkMetaMap)==1. Since the undo log builder is
registered for inserts, batch inserts with composite PK + autoincrement may
still behave inconsistently depending on code path, which contradicts the
PR/issue goal of systematic composite-PK support. Consider aligning the builder
implementation (or deduplicating into a shared helper) and adding/adjusting
tests for that path.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
