WyRainBow commented on PR #1135:
URL:
https://github.com/apache/incubator-seata-go/pull/1135#issuecomment-5301334564
> # 中文 / Chinese
> 这是 #1074 的正确后继版本。核心的线格式顺序已修对 —— `branchType → resourceId → saveDays`，与
Java `UndoLogDeleteRequestCodec` 完全一致，解码失败返回 nil 也做了。processor 的质量明显更高：
>
> * 非 AT 分支跳过
> * **`saveDays <= 0` 在入口就拒绝** —— 这是关键安全点，否则 cutoff 会被推到今天/未来，把 undo log 全删掉
> * 资源不在本 client 缓存 → 跳过（正确处理 TC 广播语义）
> * 检查 undo_log 表存在
> * batch delete 循环 drain 到 `affected < batchSize` 为止
> * `safeGetResourceManager` recover 兜底
>
> 我倾向 approve，几点建议：
>
> ### 1.（建议合并前处理）`DELETE ... LIMIT` 是 MySQL 方言，PG 会报错
> ```go
> deleteSQL := fmt.Sprintf("DELETE FROM %s WHERE log_created <= ? LIMIT %d",
undoLogTable, batchSize)
> ```
>
> Postgres 不支持 `DELETE ... LIMIT`。PR body 说 PG 留 follow-up，但当前代码对任意 AT
资源都发这条 SQL —— 如果本 client 管理的是 PG AT 资源，每个 TC 清理周期都会报 SQL 错。既然已经拿到了
`res.GetDbType()`，建议**显式 gate 到 MySQL**，其它 dbType warn + skip，而不是发一条注定失败的
SQL。（如果 seata-go AT 目前只支持 MySQL，那实际不会触发，但防御性 gate 更稳，也为 PG follow-up 留好扩展点。）
>
> ### 2. `undoLogTable` 走 `fmt.Sprintf` 拼进 SQL
> `batchSize` 是 int，安全；`undoLogTable` 来自 config，可信度较高，但建议做一次标识符校验或保持
hardcode 默认，避免将来 config 被不受信来源污染时的注入面。
>
> ### 3. drain 循环没有上限 / 时间预算
> `for {}` 会一次性删光所有过期行。若积压极大（百万级），会长时间占用一条连接、在 processor goroutine
里同步循环很多轮。建议加一个 `ctx` deadline 或最大迭代数保护，避免单次 TC 请求把 client 拖住太久。
>
> ### 4. 建议加 golden-bytes 解码用例
> 和给 #1074 的建议一样：除了 round-trip，加一个用 Java 对拷的硬编码字节序列做 Decode
的用例，才能真正锁死线格式、防止顺序再回退。你们已经有 sqlmock 的 end-to-end 测试，这点补上就很完整了。
>
> ### 5. 关闭 #1074
> 既然本 PR "Continues #1074 / Closes #1134" 且修正了 #1074 的顺序 bug，建议合并本 PR 后关掉
#1074，避免 codec 重复注册。
>
> ## 小结
> 方向和实现都对，明显优于 #1074。第 1 点（PG 方言 gate）建议合并前处理，其余可 follow-up。
>
> # English
> This is the correct successor to #1074. The core wire order is now fixed —
`branchType → resourceId → saveDays`, matching Java `UndoLogDeleteRequestCodec`
exactly, and it returns nil on decode failure. The processor is noticeably
higher quality:
>
> * Skips non-AT branches
> * **Rejects `saveDays <= 0` at the entry** — a key safety point; otherwise
the cutoff moves to today/the future and deletes all undo logs
> * Resource not in this client's cache → skip (correctly handles TC
broadcast semantics)
> * Checks the undo_log table exists
> * Batch-delete loops until `affected < batchSize`
> * `safeGetResourceManager` recovers defensively
>
> I'm inclined to approve. A few suggestions:
>
> ### 1. (Worth handling before merge) `DELETE ... LIMIT` is MySQL-specific;
PG errors
> ```go
> deleteSQL := fmt.Sprintf("DELETE FROM %s WHERE log_created <= ? LIMIT %d",
undoLogTable, batchSize)
> ```
>
> Postgres doesn't support `DELETE ... LIMIT`. The PR body defers PG to a
follow-up, but the current code issues this SQL for any AT resource — if this
client manages a PG AT resource, every TC cleanup cycle throws a SQL error.
Since you already have `res.GetDbType()`, explicitly **gate to MySQL** and
warn+skip other dbTypes rather than emitting SQL that's guaranteed to fail. (If
seata-go AT only supports MySQL today it won't trigger, but a defensive gate is
safer and leaves a clean extension point for the PG follow-up.)
>
> ### 2. `undoLogTable` interpolated into SQL via `fmt.Sprintf`
> `batchSize` is an int (safe); `undoLogTable` comes from config (fairly
trusted), but consider an identifier validation or keeping a hardcoded default
to avoid an injection surface if config is ever populated from an untrusted
source.
>
> ### 3. No cap / time budget on the drain loop
> `for {}` deletes all expired rows in one go. With a huge backlog (millions
of rows), it holds one connection and loops many times synchronously in the
processor goroutine. Add a `ctx` deadline or a max-iteration guard so a single
TC request can't tie up the client too long.
>
> ### 4. Add a golden-bytes decode case
> Same suggestion I gave on #1074: beyond round-trip, add a Decode case fed
a hardcoded byte sequence copied from Java — that truly locks the wire format
against a future order regression. You already have the sqlmock end-to-end
test, so this rounds it out nicely.
>
> ### 5. Close #1074
> Since this PR "Continues #1074 / Closes #1134" and fixes #1074's order
bug, close #1074 after merging this one to avoid duplicate codec registration.
>
> ## Summary
> Right direction and implementation, clearly better than #1074. Item 1 (PG
dialect gate) is worth handling before merge; the rest can be follow-ups.
Thanks! Items 1, 3, 4 fixed in 6c42087.
--
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]
