Vanillaxi commented on PR #1139: URL: https://github.com/apache/incubator-seata-go/pull/1139#issuecomment-5305449301
> Thanks for the update. I re-reviewed the pr. The generic single-transaction execution flow is clear, but I think the following issues should be addressed before merging. > > ### Blocking issues > 1. **The public Batch API drops per-item results and transaction outcomes** > In [`executeBatch`](https://github.com/apache/incubator-seata-go/blob/66399bdfe53bba68ffee8a80d233a604fda08f23/pkg/datasource/sql/batch.go#L152-L158), each `sql.Result` is discarded, while both exported APIs return only `error`. > This means callers cannot obtain ordered `RowsAffected`/`LastInsertId` values, the failed item index, or the results of items completed before a failure. Commit errors and rollback failures are also flattened into ordinary errors, so callers cannot reliably distinguish `rolled back`, `rollback failed`, or `commit outcome unknown`. > This conflicts with the planned PR1 scope in [#1128](https://github.com/apache/incubator-seata-go/issues/1128#issuecomment-4977508663), which explicitly includes per-item results and a transaction state model. Since these are public, release-noted APIs, adding this later would require breaking the signatures or introducing another API. > I suggest defining an ordered `BatchResult`/`ItemResult` and a typed error or outcome containing the failed index, execution phase, and transaction state before publishing the API. > 2. **AT commit precondition failures can leave the underlying MySQL transaction open** > This is not introduced by this PR, but the new DB-owned API relies on it at [`tx.Commit()`](https://github.com/apache/incubator-seata-go/blob/66399bdfe53bba68ffee8a80d233a604fda08f23/pkg/datasource/sql/batch.go#L115-L117). > [`ATTx.commitOnAT`](https://github.com/apache/incubator-seata-go/blob/66399bdfe53bba68ffee8a80d233a604fda08f23/pkg/datasource/sql/tx_at.go#L57-L79) returns directly when branch registration, undo-manager lookup, or undo-log flushing fails, without rolling back the underlying local transaction. However, `database/sql.Tx.Commit` marks the transaction as done before invoking the driver, so the caller cannot roll it back afterward. Unless the error is `driver.ErrBadConn`, the connection may also be returned to the pool in an unsafe transactional state. > The AT commit path should roll back on every failure before the local commit. If rollback fails or the connection state is uncertain, the returned error should include `driver.ErrBadConn` so the connection is discarded. > 3. **The DB-owned path lacks panic-safe transaction cleanup** > After [`BeginTx`](https://github.com/apache/incubator-seata-go/blob/66399bdfe53bba68ffee8a80d233a604fda08f23/pkg/datasource/sql/batch.go#L100-L103), rollback only happens when `executeBatch` returns an error. A panic from a `driver.Valuer`, Seata executor, or underlying driver bypasses this path and can leave the transaction holding its connection indefinitely. > Please install a best-effort `defer tx.Rollback()` immediately after `BeginTx`. The existing explicit rollback can remain so ordinary rollback errors are still reported. > > ### Test issues > * [`sql.Open`](https://github.com/apache/incubator-seata-go/blob/66399bdfe53bba68ffee8a80d233a604fda08f23/pkg/datasource/sql/batch_seata_at_test.go#L44-L47) runs before the mock connector is injected. The tests therefore make real connections to `127.0.0.1:3306`. Locally, all three AT tests logged `Access denied` but still passed. The connector should be mocked before resource initialization. > * The test helper registers a global AT ResourceManager without restoring it. Please use the existing cleanup-aware test helper. > * All AT tests execute `SELECT ?`, which takes the plain-executor path and produces no images, lock keys, branch registration, or undo log. These tests verify transaction ordering, but not the documented AT branch lifecycle. An UPDATE/INSERT case should assert accumulated images/lock keys, one branch registration, and one undo-log flush. 感谢review 1. 已为两个 batch 的执行添加逐项结果与事务结果的返回`(BatchResult, error)` 2. 借鉴java侧“由事务拥有者统一清理 AT commit 失败”的思想，调整设计: 如果通过 `driver.ErrBadConn` 丢弃连接，则存在自动重试风险，`database/sql.DB.ExecContext` 遇到 `errors.Is(err, driver.ErrBadConn)` 会在新连接上重试。 因此在最新pr中，这里设计为将`ATTx.Commit()` 做成统一失败收口的 module，回滚失败/本地提交结果未知时设置 `invalid = true`。`database/sql` 会从连接池中丢弃该连接，同时保留原始业务错误，不触发整条 SQL 重试 3. 现在已调整为：将私有 `AT commit outcome` 映射到公共 `Batch transaction state`，同时`ExecBatchContext` 在 `BeginTx` 成功后立即安装 cleanup 4. 测试问题已处理 -- 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]
