Ethan-Xingyue commented on PR #1140:
URL:
https://github.com/apache/incubator-seata-go/pull/1140#issuecomment-5289540513
Thanks for the work here — the overall shape is good. Validating the whole
plan *before* any before-image query or business SQL runs is the right
ordering, splitting plan construction (`multi_execution_plan.go`) from
execution reads well, and the table-driven tests for the plan builder cover a
nice range of cases.
I ran the following locally on `6bb8930` (go1.24.3, darwin/arm64):
- `go vet ./pkg/datasource/sql/...` — clean
- `gofmt -l` over every changed `.go` file — clean
- `go test ./pkg/datasource/sql/exec/at/... ./pkg/datasource/sql/util/...
-count=1` — pass
Comments below, roughly by importance. Nothing here is a hard blocker except
possibly 1, which I think is worth settling before merge.
---
### 1. The two execution paths report `RowsAffected` differently
The aggregate path issues one driver call with the full multi-statement
string and returns the driver's result (`multi_executor.go:89`). The sequential
path returns `lastResult` — the result of the **last** statement only
(`multi_sequential_executor.go:137,140`).
So for the same input SQL, `Result.RowsAffected()` depends on which path was
selected, and path selection depends on things the caller can't see: whether
the statements are parameterized, whether they target the same table, and
whether some unrelated hook happens to be registered.
Was this considered? If the divergence is intended, it would help to
document it on `ExecContext`. Otherwise, summing affected rows across
statements on the sequential path would make the two paths agree.
### 2. Any registered UPDATE/DELETE hook globally disables the aggregate path
```go
if plan.useAggregatePath && !hasStatementSpecificHooks(plan) { //
multi_executor.go:47
```
`hasStatementSpecificHooks` consults the **global** hook registry by SQL
type (`multi_executor.go:63`), not anything statement-local. So registering a
single UPDATE hook anywhere in the process silently flips every multi-statement
UPDATE from one round-trip to N round-trips, process-wide.
That's a config-driven performance cliff with no signal to the operator. Two
thoughts:
- At minimum, log once when the aggregate path is skipped for this reason,
so it's diagnosable.
- Better, if feasible: run the statement-specific hooks on the aggregate
path rather than abandoning the path entirely.
This coupling is also the only reason `exec.HooksForSQLType` had to become
exported API in `pkg/` — worth weighing.
### 3. `HooksForSQLType` allocates a slice copy just to test its length
```go
func HooksForSQLType(sqlType types.SQLType) []SQLHook { // hook.go:75
hooks := hookSolts[sqlType]
return append([]SQLHook(nil), hooks...) // hook.go:77
}
```
The only production caller is `len(hooksForSQLType(...)) != 0`
(`multi_executor.go:63`), so this copies a slice per statement per execution
and immediately discards it. An `exec.HasHooksForSQLType(sqlType) bool` would
avoid the allocation and express the intent better.
### 4. `rejectATPreparedMultiSQL` fails open, and parses on every `Prepare`
```go
parseCtx, err := sqlparser.DoParser(query)
if err != nil || parseCtx == nil {
return nil // conn_at.go:64
}
```
Two things:
- **Fails open on parse error.** SQL the parser can't handle is allowed
straight through to `Prepare`, which is exactly the case where AT interception
is least likely to behave. Is permitting it deliberate? A comment would help
either way.
- **Cost on the hot path.** This adds a full SQL parse to every
`Prepare`/`PrepareContext` on MySQL. Since a multi-statement query must contain
an interior `;`, an early `if !strings.Contains(query, ";") { return nil }`
would skip the parse for the overwhelming majority of statements.
### 5. `execSequential` re-parses every statement on every execution
Each statement's AST is restored to SQL and then handed back to
`parser.DoParser` (`multi_sequential_executor.go:63`), with the result used to
check that the executor type didn't change (`:68`). That's N extra full parses
per execution, on the hot path.
The child `*types.ParseContext` in `MultiStmt` already carries the AST. Is
the round-trip check load-bearing in practice, or could it be dropped (or moved
into plan construction / behind a debug assertion)?
### 6. `rowsWithStmt` now has three copies
```
pkg/datasource/sql/conn_at.go:36
pkg/datasource/sql/exec/at/insert_executor.go:249
pkg/datasource/sql/util/ctxutil.go:125 (new in this PR)
```
The new one is the best of the three — it nil-guards both fields and joins
both errors with `errors.Join`. The two older copies nil-deref if either field
is nil, and silently drop the statement-close error whenever the rows-close
error is non-nil.
Both other packages already import `util`. Exporting the new implementation
there and deleting the other two would remove the duplication and fix the
weaker behavior at the same time. Happy for that to be a follow-up if you'd
rather keep this PR focused — but three divergent copies of the same wrapper is
a trap for the next reader.
### 7. Error wrapping drops the cause
```go
return "", fmt.Errorf("%w: get table name for statement %d: %v",
ErrInvalidMultiSQL, index, err) // multi_execution_plan.go:120
```
`err` is formatted with `%v`, so the underlying error can't be recovered
with `errors.Is`/`errors.As`. Go 1.20+ supports multiple `%w` in one `Errorf`,
so `...: %w", ErrInvalidMultiSQL, index, err)` works here.
### 8. Are the exported error values intended as public API?
`ErrInvalidMultiSQL` and `ErrUnsupportedMultiSQL`
(`multi_execution_plan.go:28-29`) are exported from a `pkg/` package. If
they're only consumed internally and by tests, unexporting them keeps the
public surface smaller.
---
--
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]
