[fix](protocol) Prevent Connector/J cursor fetch from hanging on empty results - #67520
[fix](protocol) Prevent Connector/J cursor fetch from hanging on empty results#67520CalvinKirs wants to merge 4 commits into
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review status: REQUEST_CHANGES. I completed the full review on head 53cf2d7d400f508f08c514df3934fbeeba4d3157; the bounded review loop converged after two rounds with two distinct P1 findings.
Critical checkpoint conclusions:
- Goal and proof: The new packet fixes the direct Connector/J 8.0.28 empty-result hang, but the implementation does not complete the stated pre-9.5 compatibility goal for legacy 5.1/6.0 clients (first inline finding), and forwarded zero-placeholder executions can still return malformed non-empty results (second inline finding).
- Scope and clarity: The per-command flag, optional forwarding field, shared metadata helper, and focused tests are otherwise small and localized.
- Concurrency: The new state is connection-local and commands are processed serially on a connection; no new shared mutable state, locking, lock-order, or deadlock concern was found.
- Lifecycle and initialization: The cursor flag survives local/RPC retries and is cleared after command finalization. The new static constants have no cross-initialization dependency. The zero-placeholder forwarding lifecycle remains incomplete as described inline.
- Configuration: No configuration item is added.
- Compatibility: Thrift field 1008 is optional and wire-skippable during rolling FE upgrades. Protocol compatibility is incomplete for the verified legacy Connector/J identities and versions.
- Parallel paths: FE-local and coordinator/cache/short-circuit metadata paths both use the helper exactly once, and proxy packet ordering preserves the compatibility marker before rows and the real final marker afterward. The follower-to-master zero-placeholder path fails to restore binary execute mode.
- Conditional checks: Cursor-bit masking and the deprecated-EOF gate are sound and locally explained. The client-name/version condition is too narrow for affected released drivers.
- Test coverage: The unit test checks the inserted packet shape, and the regression covers direct-master Connector/J 8.0.28 empty and one-row results. Coverage misses legacy driver attributes and a follower-to-master non-empty zero-placeholder result, which correspond to the two findings.
- Test results: The checked-in output matches the suite's two queries, but I did not execute or regenerate tests because this review environment explicitly prohibits builds and test execution.
- Observability: This narrowly gated packet path does not need new metrics or INFO logging; existing connection/packet diagnostics are sufficient.
- Transactions, persistence, and data writes: None are introduced or modified.
- FE/BE variable propagation: The new field is FE-to-FE. Existing
mysql_row_binary_formatpropagation to BE depends on the master's command state and is wrong in the forwarded zero-placeholder case. - Performance: One connection-attribute lookup and one precompiled regex match per result metadata sequence is negligible; no material CPU or memory regression was found.
- Other issues and user focus: No additional user focus was supplied, and no other distinct correctness, error-handling, memory-safety, cloud-mode, or coverage issue survived the final changed-file and candidate sweep.
| return false; | ||
| } | ||
| Map<String, String> connectAttributes = context.getConnectAttributes(); | ||
| if (!MYSQL_CONNECTOR_J.equals(connectAttributes.get("_client_name"))) { |
There was a problem hiding this comment.
[P1] Cover the legacy Connector/J cursor consumers
Connector/J 5.1.49 and 6.0.6 have the same cursor-probe behavior this workaround targets: when Doris advertises CLIENT_DEPRECATE_EOF, they consume the packet after column definitions for a cursor request. Those releases send _client_name=MySQL Connector Java (and 5.x/6.x versions), so this predicate never inserts the compatibility terminator. An empty result therefore still consumes its only final OK and waits for another packet; a non-empty result can consume its first binary row as status. Please include the verified legacy identity/version families (or classify by the affected protocol behavior) and add a boundary test.
| if (null != ctx.getPrepareExecuteBuffer()) { | ||
| params.setPrepareExecuteBuffer(ctx.getPrepareExecuteBuffer()); | ||
| } | ||
| params.setCursorFetchRequested(ctx.isCursorFetchRequested()); |
There was a problem hiding this comment.
[P1] Preserve binary execute mode without parameters
For a zero-placeholder prepared SELECT, handleExecute never sets prepareExecuteBuffer, so this forwards cursor_fetch_requested=true without any marker that makes the master take its prepared-execute branch. proxyExecute then leaves the fresh context as COM_SLEEP; both coordinator paths set mysql_row_binary_format=false, and non-empty rows are serialized as text even though Connector/J is reading a COM_STMT_EXECUTE binary result. The new regression uses getServerPrepareJdbcUrl, which connects directly to the master, so it misses this path. Please forward execute/binary-result intent independently of parameter bytes (or always carry an empty execute buffer) and cover a follower-to-master non-empty result.
### What problem does this PR solve?
Issue Number: None
Problem Summary: When CLIENT_DEPRECATE_EOF is negotiated, Connector/J 8.x with useCursorFetch consumes the empty result set end marker while probing for a server cursor. Track CURSOR_TYPE_READ_ONLY across FE forwarding and send a targeted compatibility metadata terminator for affected Connector/J versions so the final marker remains available.
### Release note
Fix empty prepared-statement results hanging with MySQL Connector/J 8.x when cursor fetching is enabled.
### Check List (For Author)
- Test:
- Unit Test: StmtExecutorTest#testCursorFetchMetadataTerminatorDependsOnConnectorJVersion
- Regression test: prepared_stmt_p0/test_cursor_fetch_empty_result
- Manual test: Connector/J 8.2.0 and 9.5.0 empty/non-empty cursor fetch
- Behavior changed: Yes. A compatibility metadata terminator is sent only for affected Connector/J cursor requests when CLIENT_DEPRECATE_EOF is negotiated.
- Does this need documentation: No
### What problem does this PR solve? Issue Number: None Problem Summary: Add negative compatibility coverage so the cursor-fetch workaround remains limited to affected Connector/J versions and negotiated protocol conditions. ### Check List (For Author) - Test: StmtExecutorTest#testCursorFetchMetadataTerminatorDependsOnConnectorJVersion - Behavior changed: No - Does this need documentation: No
Scope the OK-packet padding added by apache#61050 to negotiated CLIENT_DEPRECATE_EOF clients, preserve result-set warning/status fields, and select the Connector/J cursor metadata workaround by client version. Fail ambiguous cursor clients fast instead of allowing an empty-result hang. Propagate the cursor protocol context through FE forwarding and protect rolling upgrades from replaying packets serialized with an unconfirmed EOF mode. Add protocol unit coverage plus Connector/J and Arrow Flight SQL regression cases.
b745f71 to
12e48c4
Compare
CalvinKirs
changed the title
|
/review |
There was a problem hiding this comment.
Review status: capped/incomplete — the third and final convergence round added an accepted scoping constraint, which is included in the P1 old-master finding below; the workflow permits no fourth round.
Request changes. The direct Connector/J classifier and compatibility-terminator sequence are localized and coherent, but the end-to-end goal is not met across the production handshake and mixed-version FE paths.
Critical checkpoints:
- Goal and proof: direct empty/nonempty cursor results and classifier boundaries have tests, but real serializer negotiation, real proxy EOF status, both mixed-version directions, and old-master warning/info are not proved. The already-threaded zero-placeholder forwarding path also remains broken.
- Scope and clarity: production changes are focused on MySQL classification, connection state, packet framing, and FE forwarding. The Arrow Flight DDL smoke test is ancillary and does not validate this fix.
- Concurrency and lifecycle: the new state is connection-thread-owned, reset per command, and copied into fresh per-RPC proxy contexts. No new lock, deadlock, shared-mutation, circular-ownership, or static-initialization risk was found.
- Configuration and compatibility: no configuration is added. Optional Thrift fields are wire-skippable, but their absent-value and runtime-result semantics fail in both rolling-upgrade directions.
- Parallel paths and conditions: direct, new/new, new/old, old/new, coordinator, point-query, cached, FE-local, parameterized, zero-placeholder, legacy/deprecated EOF, affected/standard/unknown, and multi-statement paths were traced. The special gates have the inline defects below.
- Tests and results: the classifier, packet, request, and context tests are internally consistent and the regression output is deterministic, but mocks bypass the production capability and EOF-status flows. I did not run builds or tests because this review environment explicitly prohibits them.
- Observability and performance: the compatibility errors are actionable; no new metric is needed, and the classifier/boolean/packet overhead is negligible.
- Transactions, persistence, writes, and transport: no EditLog, storage format, FE-BE variable, or atomicity change exists. Forwarded DML executes once, but its protocol-visible result is degraded; FE-to-FE transport has the mixed-version defects below.
- Other correctness: warning/status serialization, binary/text selection, packet order, partial errors, and multi-result sequencing were checked. Four distinct inline issues remain. Existing threads
r3930709957andr3930709964were treated as duplicate fences; the former is fixed on this head and the latter was not repeated.
User focus: no additional review focus was provided, so the full PR was reviewed.
| packet = executor.getOutputPacket(); | ||
| if (ctx.getMysqlChannel().clientDeprecatedEOF() | ||
| && !executor.isForwardedClientDeprecatedEofApplied() | ||
| && executor.getProxyStatusCode() == 0) { |
There was a problem hiding this comment.
[P1] Classify successful old-master reads without widening this guard
A real forwarded SELECT cannot satisfy this gate: result producers finish with QueryState.setEof(), while proxyExecute assigns status 0 only to OK and maps successful EOF to 1105. The follower therefore replays the unsafe cursor packets; the unit test mocks the impossible combination of query buffers plus status 0. Simply accepting EOF here would also reject safe old-master COM_QUERY and Connector/J 9.5+ results because this predicate never checks cursor intent or the compatibility class. Please recognize real result-set success, scope rejection to requests that need the cursor shim, test through real proxyExecute construction, and ensure a non-final multi-statement sends the local ERR only once.
| } else { | ||
| // An old master has already completed a DDL/DML operation. Rebuild its final OK locally | ||
| // instead of returning an upgrade error that could make the client retry side effects. | ||
| ctx.getState().setOk(executor.getForwardedAffectedRows(), 0, null); |
There was a problem hiding this comment.
[P2] Preserve the old master's complete OK result
This rolling-upgrade branch rebuilds a successful DML response with only affectedRows, discarding the warning count and info string already encoded in the master's final OK packet. A normal forwarded INSERT calls OlapInsertExecutor.setReturnInfo(), which reports filteredRows as warnings and includes label/status/txnId in info; through an old master this branch changes those to zero warnings and no message. Please preserve or decode all protocol-visible OK fields (or safely reuse the ordinary OK packet) and cover a response with nonzero warnings and nonempty info.
| if (request.isSetClientDeprecatedEOF() && request.isClientDeprecatedEOF()) { | ||
| ctx.getMysqlChannel().setClientDeprecatedEOF(); | ||
| } | ||
| ctx.setCursorFetchRequested(request.isSetCursorFetchRequested() |
There was a problem hiding this comment.
[P1] Handle cursor intent from old forwarding FEs
During a rolling upgrade an old follower cannot set the new optional cursor_fetch_requested field, so this silently records false. It still forwards CLIENT_DEPRECATE_EOF, Connector/J attributes, and, for parameterized statements, the execute buffer; the new master then emits binary rows but omits the compatibility metadata marker. An affected Connector/J cursor SELECT forwarded through that old FE can therefore still consume the final marker and hang. Please treat an absent cursor-intent field as an explicit mixed-version/unknown execute mode and fail safely when the affected combination cannot be disambiguated, with an old-sender/new-master parameterized cursor test.
| } | ||
| } else if (!Strings.isNullOrEmpty(infoMessage)) { | ||
| serializer.writeLenEncodedString(infoMessage); | ||
| } else if (capability.isDeprecatedEOF()) { |
There was a problem hiding this comment.
[P2] Read the negotiated EOF capability here
This condition is false in the new unit test, but it remains true for real legacy-EOF connections. MysqlProto.negotiate records the client's bit only in MysqlChannel, then sets the serializer capability to context.getServerCapability(); that default mask always includes CLIENT_DEPRECATE_EOF. ProxyMysqlChannel starts with the same default as well. Consequently an authenticated client that did not negotiate the flag still gets the trailing zero byte this change intends to remove. Please key this from the negotiated/channel capability, and propagate it to proxy serialization, or store the negotiated mask in the serializer, with a handshake-level test.
2 participants
What problem does this PR solve?
Issue Number: None
After
CLIENT_DEPRECATE_EOFsupport was enabled by #61050 and backported by #61062, MySQL Connector/J withuseCursorFetch=trueand a positive fetch size can hang when a server-prepared statement returns an empty result set.Affected Connector/J versions consume the first OK packet after column definitions while probing whether the server created a cursor. Doris does not implement MySQL server cursors. If the result is empty, that packet is also the only result-set end marker, so the driver waits indefinitely for a packet that will never arrive.
Real-driver testing and source inspection place the incompatible behavior in Connector/J 5.1, 6.0, 8.x, and 9.0 through 9.4. Connector/J 9.5+ uses the standard non-cursor path and must not receive the compatibility terminator.
This also explains why an older customer build can appear unaffected: the behavior depends on whether that exact build negotiates
CLIENT_DEPRECATE_EOF, not only on its release label. An Apache Doris 4.1.3 build that advertises the capability reproduces the timeout.What is changed?
CURSOR_TYPE_READ_ONLYfromCOM_STMT_EXECUTEand preserve it during FE forwarding._client_nameand_client_version.CLIENT_DEPRECATE_EOFclients on the standard path.connectionAttributes=none+ cursor +CLIENT_DEPRECATE_EOFcombination immediately with an actionable error. Connector/J 8.2 and 9.5 send indistinguishable capability/execute packets when attributes are suppressed, so silently choosing either packet sequence would break one of them.CLIENT_DEPRECATE_EOFclients; legacy OK packets remain byte-for-byte unchanged.Compatibility note
This is a targeted compatibility fallback, not an implementation of MySQL server-side cursors. Doris still streams rows inline and does not serve rows through
COM_STMT_FETCH; thereforefetchSizedoes not provide MySQL-style server-side batching.During a mixed-version FE rolling upgrade, a
CLIENT_DEPRECATE_EOFread routed from a new follower to an old master is rejected with a clear error until the master is upgraded or the client connects directly to the master. Mutating statements are not reported as failed after execution, preventing duplicate side effects from client retries.Clients using
connectionAttributes=nonetogether with cursor fetch must enable connection attributes or setuseCursorFetch=false.Release note
Fix empty prepared-statement results hanging with MySQL Connector/J 5.x, 6.x, 8.x, and 9.0-9.4 when cursor fetching is enabled, while preserving Connector/J 9.5+, legacy EOF, FE forwarding, and Arrow Flight SQL compatibility.
Check List (For Author)
./build.sh --fepassed all 73 modules.prepared_stmt_p0/cursor_fetch_empty_resultandarrow_flight_sql_p0/test_ddl.useCursorFetch=false, fetch size zero, client-side prepared statements, and MariaDB Connector/J 3.5.6 pass.