Copilot commented on code in PR #1137:
URL:
https://github.com/apache/incubator-seata-go/pull/1137#discussion_r3549919775
##########
pkg/datasource/sql/exec/executor.go:
##########
@@ -67,6 +67,15 @@ func BuildExecutor(dbType types.DBType, transactionMode
types.TransactionMode, q
hooks = append(hooks, commonHook...)
hooks = append(hooks, hookSolts[parseContext.SQLType]...)
+ // For XA mode, use a plain executor without AT-specific hooks
+ // XA transactions don't need undo logs - they use the two-phase commit
protocol managed by TC
+ // Note: undo_log_hook.go already handles skipping undo log generation
for XA mode
+ if transactionMode == types.XAMode {
+ e := &BaseExecutor{}
+ e.Interceptors(hooks)
+ return e, nil
+ }
Review Comment:
BuildExecutor’s XA fast-path is bypassed when parser.DoParser fails for
PostgreSQL: the function returns newATExecutor(dbType, nil) before reaching the
XA branch. That means XA mode may still construct an AT executor (and require
AT executor registration) for PostgreSQL-specific SQL that the shared parser
can’t parse.
##########
pkg/datasource/sql/conn_xa.go:
##########
@@ -204,29 +202,38 @@ func (c *XAConn) createOnceTxContext(ctx context.Context)
bool {
func (c *XAConn) createNewTxOnExecIfNeed(ctx context.Context, f func()
(types.ExecResult, error)) (types.ExecResult, error) {
var (
- tx driver.Tx
- err error
+ tx driver.Tx
+ err error
+ xaRollbacked bool // Track if XA rollback was already done to
avoid duplicate rollback
)
defer func() {
recoverErr := recover()
- if recoverErr != nil {
- log.Errorf("conn xa rollback recoverErr:%v", recoverErr)
- if tx != nil {
- if rollbackErr := tx.Rollback(); rollbackErr !=
nil {
- log.Errorf("conn xa rollback error:%v",
rollbackErr)
- }
- return
- }
- if c.tx != nil {
- if rollbackErr := c.Rollback(ctx); rollbackErr
!= nil {
- log.Errorf("conn xa rollback error:%v",
rollbackErr)
+ // Check if error is ErrSkip - don't rollback for this special
error
+ isErrSkip := err != nil && errors.Is(err, driver.ErrSkip)
+
+ if (err != nil && !isErrSkip) || recoverErr != nil {
+ // Prefer XATx.Rollback so a registered branch reports
phase-1 failure to
+ // the TC; fall back to the raw connection rollback for
non-autoCommit paths.
+ if !xaRollbacked {
+ if tx != nil {
+ if rollbackErr := tx.Rollback();
rollbackErr != nil {
+ log.Errorf("defer rollback xa
branch error:%v", rollbackErr)
+ }
+ xaRollbacked = true
+ } else if c.xaActive {
Review Comment:
The new driver.ErrSkip handling returns ErrSkip without rolling back, but
XAConn.ExecContext/QueryContext defer-reset txCtx when autoCommit=true. That
means the subsequent database/sql fallback (prepare+exec on the same Conn) will
likely run with txCtx reset to Local while an XA branch may already be
active/registered, causing inconsistent state and potential branch leaks or
protocol errors.
##########
pkg/datasource/sql/hook/undo_log_hook.go:
##########
@@ -44,6 +44,12 @@ func (h *undoLogSQLHook) Before(ctx context.Context, execCtx
*types.ExecContext)
return nil
}
+ // Skip undo log generation for XA mode - XA transactions don't need
undo logs
+ // Undo logs are only needed for AT mode's automatic compensation
+ if execCtx.TxCtx != nil && execCtx.TxCtx.TransactionMode ==
types.XAMode {
+ return nil
+ }
Review Comment:
The XA-mode skip added here is likely never reached in normal execution
because undoLogSQLHook.Type() returns SQLTypeUnknown, and exec.RegisterHook
ignores SQLTypeUnknown hooks (so the undo log hook is never registered into
hookSolts). As a result, this XA skip (and undo log generation in general) may
not run at all.
##########
pkg/datasource/sql/tx.go:
##########
@@ -149,6 +149,17 @@ func (tx *Tx) Rollback() error {
}
}
+ // In XA mode, target might be nil (set with withOriginTx(nil))
+ // Only allow nil target when explicitly in XA mode; otherwise,
+ // treat it as an error to avoid masking unexpected
driver/initialization bugs
+ if tx.target == nil {
+ if tx.tranCtx != nil && tx.tranCtx.TransactionMode ==
types.XAMode {
+ // XA transactions are managed separately
+ return nil
+ }
+ return fmt.Errorf("sql.Tx Rollback: underlying transaction is
nil in non-XA mode")
+ }
Review Comment:
Returning nil when tx.target is nil in XA mode will silently treat rollback
as successful even if this Tx was created without an xaConn (e.g., Conn.BeginTx
sets withOriginTx(nil) and does not set withXAConn). That can mask incorrect
transaction wiring; consider delegating to xaConn when present, and otherwise
returning an explicit error in XA mode too.
##########
changes/dev.md:
##########
@@ -26,6 +26,7 @@
### feature：
- [[#123](https://github.com/apache/incubator-seata-go/pull/123)] add two
phase and dubbo
+ - support XA branch enrollment for autoCommit statements in global
transaction
Review Comment:
This changelog entry claims support for XA branch enrollment/reuse for
autoCommit statements, but the code changes in this PR don’t appear to
introduce the advertised mechanisms (e.g., IsAutoCommitXABranch / XA branch
reuse registry) and conn_xa.go still commits a branch per statement in
autoCommit mode.
--
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]
