Ethan-Xingyue commented on PR #1139:
URL:
https://github.com/apache/incubator-seata-go/pull/1139#issuecomment-5301308465
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.
--
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]
