Implement basic BATCH in Firebird Proxy#38197
Implement basic BATCH in Firebird Proxy#38197makssent wants to merge 13 commits intoapache:masterfrom
Conversation
terrymanu
left a comment
There was a problem hiding this comment.
Merge Verdict: Not Mergeable (latest head: 2544857)
- Reviewed Scope: Firebird batch packet parsing, batch executors/registry, BLOB binding path, related tests/CI.
- Not Reviewed Scope: real Firebird end-to-end runtime validation.
- Need Expert Review: Firebird protocol maintainer review is recommended.
- Blocker Wrong handle source in batch execute response (can cause NPE / wrong semantics)
- File: FirebirdBatchExecuteCommandExecutor.java:52 (/tmp/shardingsphere-pr38197/proxy/frontend/dialect/firebird/src/main/java/org/apache/shardingsphere/proxy/frontend/firebird/command/query/batch/FirebirdBatchExecuteCommandExecutor.java:52)
- Issue: nextStatementId(packet.getStatementHandle()) uses statement handle where API expects connection id.
- Contract: FirebirdStatementIdGenerator.java:61 (/tmp/shardingsphere-pr38197/proxy/frontend/dialect/firebird/src/main/java/org/apache/shardingsphere/proxy/frontend/firebird/command/query/statement/FirebirdStatementIdGenerator.java:61)
- Request: return the proper batch handle; do not generate statement id from packet.getStatementHandle().
- Blocker BLOB binding moved after backend execution (behavior regression)
- File: FirebirdExecuteStatementCommandExecutor.java:78 (/tmp/shardingsphere-pr38197/proxy/frontend/dialect/firebird/src/main/java/org/apache/shardingsphere/proxy/frontend/firebird/command/query/statement/execute/FirebirdExecuteStatementCommandExecutor.java:78),
FirebirdExecuteStatementCommandExecutor.java:90 (/tmp/shardingsphere-pr38197/proxy/frontend/dialect/firebird/src/main/java/org/apache/shardingsphere/proxy/frontend/firebird/command/query/statement/execute/FirebirdExecuteStatementCommandExecutor.java:90) - Issue: QueryContext is built/executed before blob-id-to-bytes conversion.
- Risk: incorrect update behavior or type mismatch for BLOB params.
- Request: bind BLOB params before building QueryContext; keep cleanup after execution.
- Blocker Incorrect BATCH_MSG frame length calculation
- File: FirebirdBatchSendMessageCommandPacket.java:65 (/tmp/shardingsphere-pr38197/database/protocol/dialect/firebird/src/main/java/org/apache/shardingsphere/database/protocol/firebird/packet/command/query/batch/FirebirdBatchSendMessageCommandPacket.java:65),
FirebirdPacketCodecEngine.java:133 (/tmp/shardingsphere-pr38197/database/protocol/dialect/firebird/src/main/java/org/apache/shardingsphere/database/protocol/firebird/codec/FirebirdPacketCodecEngine.java:133) - Issue: getLength() returns readable - 12; codec slices by that value, which truncates packets and can desync subsequent parsing.
- Request: return full packet length, or return <=0 to use variable-length fallback; add framing tests.
Notes:
- CI is green (148 checks), but coverage does not prove these protocol/ordering risks are safe.
|
I have addressed the comments for points 1 and 2. Point 3 has been left unchanged for now, as it currently implements the functionality required at this stage. I plan to revise it in the near future, and a corresponding TODO has already been added. If this is critical, please let me know and I will move the task back to Draft until the changes are made. Otherwise, I suggest merging the changes in their current state. As for a review from the Firebird maintainer, unfortunately, this is not possible. |
There was a problem hiding this comment.
Decision
- Merge Verdict: Not Mergeable
- Reviewed Scope: Firebird batch protocol packet parsing/length detection, batch command executors/registry, BLOB binding path, PR latest head cd7f081, PR checks status.
- Not Reviewed Scope: Real Firebird runtime E2E behavior against live Firebird server versions.
- Need Expert Review: Yes (Firebird protocol maintainer).
Positive Feedback
- This part is in the right direction, especially previous blockers #1 and #2 were fixed in latest head:
- Batch execute now uses the registered batch statement handle path instead of generating from packet handle directly: FirebirdBatchExecuteCommandExecutor.java#L41
- BLOB binding is now done before QueryContext construction in normal execute path: FirebirdExecuteStatementCommandExecutor.java#L75
Major Issues
- [Blocker][Root-cause unresolved] BATCH_MSG packet length calculation is still incorrect
- Symptom: getLength() returns readable - 12 with a production TODO: FirebirdBatchSendMessageCommandPacket.java#L62
- Risk: codec slices by expectedLength for positive values, so undercounted length can truncate one packet and desync next packet framing: FirebirdPacketCodecEngine.java#L131
- Recommended action: please implement actual BATCH_MSG frame length parsing (or return <=0 until fully parseable so codec consumes complete readable frame safely).
Newly Introduced Issues
- [High][Resource leak risk] Batch path binds BLOBs but never clears upload cache
- Symptom: batch message path calls binder and only keeps .getParams(), dropping blob IDs needed for cleanup: FirebirdSendBatchMessageCommandExecutor.java#L69
- Symptom: batch execute path has no clearBlobUploads(...) call: FirebirdBatchExecuteCommandExecutor.java#L43
- Cleanup API exists but is unused in batch flow: FirebirdBlobParameterBinder.java#L80
- Risk: uploaded BLOB payloads can accumulate until disconnect, increasing memory pressure.
- Recommended action: please carry blob IDs through batch state and clear them after successful batch execution (and on cancel/error rollback path).
- [High][Validation incomplete] No dedicated tests for newly added batch behavior
- Evidence: only two existing parser tests were touched; no new tests for BATCH_CREATE/BATCH_MSG/BATCH_EXEC/BATCH_CANCEL, registry lifecycle, framing, or BLOB cleanup mapping:
- FirebirdFetchStatementPacketTest.java
- FirebirdExecuteStatementPacketTest.java
- Risk: current CI green cannot prove protocol framing and batch resource lifecycle correctness.
- Recommended action: please add one-to-one tests for each fix point (length framing, batch execute response, BLOB cleanup in batch, cancel cleanup).
Next-step Suggestions
- Fix BATCH_MSG length parsing first (blocking protocol correctness).
- Add BLOB cleanup in batch execute/cancel flows and verify with dedicated tests.
- Add focused unit tests for new batch packet/executor paths before next review round.
Multi-round Comparison
- Fixed from previous round:
- Wrong batch handle source in execute response path: fixed.
- BLOB bind ordering in normal execute path: fixed.
- Unresolved from previous round:
- BATCH_MSG frame length calculation blocker: still unresolved.
- Newly introduced in this round:
- Missing BLOB cache cleanup in batch flow.
- Insufficient dedicated tests for newly added batch logic.
Short summary: previous two blockers were fixed, but packet framing for BATCH_MSG remains broken and batch BLOB cleanup is missing; with these unresolved risks and incomplete validation, this PR should not be merged yet.
Changes proposed in this pull request:
Before committing this PR, I'm sure that I have checked the following options:
./mvnw clean install -B -T1C -Dmaven.javadoc.skip -Dmaven.jacoco.skip -e.