You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@shardingsphere.apache.org by "schizophrenicmaniac (via GitHub)" <gi...@apache.org> on 2026/10/27 21:04:51 UTC

[PR] Check Firebird statement handle of op_info_sql and op_free_statement as the server does [shardingsphere]

schizophrenicmaniac opened a new pull request, #40379:
URL: https://github.com/apache/shardingsphere/pull/40379

   Fixes #40373.
   
   Changes proposed in this pull request:
     - Track allocated statement handles per connection in `FirebirdStatementIdGenerator`, and release the handle on `op_free_statement` with `DSQL_drop`.
     - `op_info_sql` and `op_free_statement` now answer `isc_bad_req_handle` for a handle that was never allocated or was dropped. `op_info_sql` answers `isc_unprepared_stmt` for an allocated statement that isn't prepared, including after `DSQL_unprepare`.
     - Keep whether the connection accepted `ptype_lazy_send` (new `FirebirdAcceptPacket#isLazySend`) and the last object allocated on it in a new `FirebirdLastObjectRegistry`. Allocating a statement records it; starting a transaction or creating/opening a BLOB replaces it. On a lazy connection the handle `0xFFFF` resolves to the last statement, and gives `isc_bad_req_handle` when the last object is something else.
     - The errors are sent as error responses instead of thrown, like the batch release/cancel executors already do. Throwing would make `FirebirdFrontendEngine` mark the current transaction as failed, and the transaction has to stay usable.
   
   The issue asks for E2E cases, but the Firebird E2E is SQL-level over JDBC and can't send `op_info_sql` / `op_free_statement` with arbitrary handles, so the cases from the issue table are covered by unit tests of the executors instead. Happy to add protocol-level E2E coverage if there's a preferred way to do it.
   
   ---
   
   Before submitting this PR, I confirm that:
   - [x] My code follows the [code of conduct](https://shardingsphere.apache.org/community/en/involved/conduct/code/) of this project.
   - [x] I have reviewed my changes.
   - [ ] I have (or in comment I request) added corresponding labels for the pull request.
   - [x] I have passed the required local checks: `./mvnw clean install -B -T1C -Pcheck`.
   - [ ] I have updated the relevant documentation, if applicable.
   - [x] I have added or updated unit tests for my changes, if applicable.
   - [ ] I have updated the Release Notes of the current development version, if applicable. For more details, see [Update Release Note](https://shardingsphere.apache.org/community/en/involved/contribute/contributor/).
   
   AI disclosure: drafted with Claude Code (Claude Opus 5.5); files: InvalidRequestHandleException.java, UnpreparedStatementException.java, FirebirdDialectExceptionMapper.java, FirebirdVendorError.java, FirebirdDialectExceptionMapperTest.java and 22 more.


-- 
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: notifications-unsubscribe@shardingsphere.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Check Firebird statement handle of op_info_sql and op_free_statement as the server does [shardingsphere]

Posted by "schizophrenicmaniac (via GitHub)" <gi...@apache.org>.
schizophrenicmaniac commented on PR #40379:
URL: https://github.com/apache/shardingsphere/pull/40379#issuecomment-6461192569

   @terrymanu Thanks for the review, all four are addressed in the latest commit.
   
   For the handle normalization I only reduce negative handles to their unsigned 16-bit value, so both 0x0000FFFF and the sign-extended 0xFFFFFFFF now resolve to the last statement on a lazy connection, and DSQL_drop releases it. Positive handles are left as they are because the proxy's statement IDs come from an AtomicInteger and aren't limited to 16 bits like the server's. Masking every handle would make info/free fail for statements past 65535 on a long-lived Jaybird connection, which sends back the full int it was given. Both encodings are covered in FirebirdSQLInfoExecutorTest and FirebirdFreeStatementCommandExecutorTest with packets built from real payload bytes, plus registry tests for the sign-extended cases.
   
   I also moved the helpers after their first callers. In FirebirdStatementIdGenerator that includes getStatementCounter, so both helpers follow nextStatementId in call order. The three registry methods now have @HighFrequencyInvocation, and the test variable is renamed to actualReturnPacket.


-- 
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: notifications-unsubscribe@shardingsphere.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Check Firebird statement handle of op_info_sql and op_free_statement as the server does [shardingsphere]

Posted by "schizophrenicmaniac (via GitHub) via notifications" <no...@shardingsphere.apache.org>.
schizophrenicmaniac commented on PR #40379:
URL: https://github.com/apache/shardingsphere/pull/40379#issuecomment-6597513551

   @terrymanu Thanks, both are fixed in the latest commit.
   
   You're right about the keys. I moved the 16-bit conversion out of the resolver and into packet decoding. The new FirebirdPacketPayload#readObjectHandle reads a negative handle as its unsigned 16-bit value and leaves positive handles alone, like readBlobHandle does for BLOBs. Every packet that carries a statement handle now uses it: prepare, execute, fetch, free, info and the batch create/message/execute/release/cancel packets. So 0xFFFF8001 is 0x8001 everywhere: allocation checks, the prepared statement registry, the fetch cache and the batch registry. The resolver now only replaces 0xFFFF with the last statement on lazy connections. Since the info packet is shared, sign-extended blob and database info handles are decoded the same way too, which matches the server. The regression tests send a real 0xFFFF8001 payload through the prepare, info and free executors and check that prepare registers the statement under 0x8001, info returns its records, and DSQL_drop removes and releases 
 it.
   
   getAllocatedStatements now uses String.format for the message, and it's still built inside the supplier.


-- 
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: notifications-unsubscribe@shardingsphere.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Check Firebird statement handle of op_info_sql and op_free_statement as the server does [shardingsphere]

Posted by "schizophrenicmaniac (via GitHub)" <gi...@apache.org>.
schizophrenicmaniac commented on PR #40379:
URL: https://github.com/apache/shardingsphere/pull/40379#issuecomment-6475616897

   @terrymanu Thanks, all six are fixed in the latest commit.
   
   For the 65535 collision I went at it from the allocation side: FirebirdStatementIdGenerator now skips 0xFFFF when generating statement IDs, the same as the Firebird server, which never hands out INVALID_OBJECT as a handle. So 0xFFFF (and -1 after normalization) can only mean the lazy placeholder. If the last object isn't a statement, it never matches an allocated statement and gets isc_bad_req_handle. Changing only the resolver wouldn't have been enough, because a real statement with ID 65535 would then have been unreachable through its own handle. FirebirdStatementIdGeneratorTest covers the boundary by moving the counter to 0xFFFE and checking that the next ID is 0x10000 and that 0xFFFF isn't allocated.
   
   The other changes: InvalidRequestHandleExceptionTest and UnpreparedStatementExceptionTest check the message and handle, isStatementAllocated and releaseStatementId have @HighFrequencyInvocation, the two-case parameterized tests are now separate tests for the unsigned and sign-extended encodings, .writeInt(1) is back on the first line, and the variable is renamed to actualReturnPacket.


-- 
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: notifications-unsubscribe@shardingsphere.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Check Firebird statement handle of op_info_sql and op_free_statement as the server does [shardingsphere]

Posted by "schizophrenicmaniac (via GitHub) via notifications" <no...@shardingsphere.apache.org>.
schizophrenicmaniac commented on PR #40379:
URL: https://github.com/apache/shardingsphere/pull/40379#issuecomment-6771353088

   @terrymanu Thanks, all three are fixed in the latest commit: readObjectHandle now has @HighFrequencyInvocation, the helper variable is renamed to actualResponsePackets, and .writeInt(8) is back on the preceding line.


-- 
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: notifications-unsubscribe@shardingsphere.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org