Add prepared statement caching mechanism in proxy

[AsyncKurisu]

Fixes #37274

Changes proposed in this pull request:

Add a Firebird-only connection-held prepared statement cache for Proxy backend execution:

• Introduce PreparedStatementCacheContext with LRU eviction (LinkedHashMap with access order).
• Introduce PreparedStatementCacheKey and corresponding cache lifecycle hooks in shared backend-core only as internal generic infrastructure, while the current and only enabled usage remains Firebird.
• Reuse prepared statements in JDBCBackendStatement only when the Firebird frontend explicitly activates a cache key in ConnectionSession.
• Use a cache key composed of physical connection identity, SQL text, returnGeneratedKeys, and Firebird prepared statement identity.

Reuse prepared statements only while the backend connection is still held:

• Reuse cached prepared statements for repeated execution of the same Firebird prepared statement within a held Proxy connection.
• Call clearParameters() before rebinding parameters on every reuse.
• Do not claim reuse after physical connection close or across the normal auto-commit connection teardown path.

Keep Firebird statement-handle semantics and cleanup owned by the Firebird frontend:

• Activate and clear the current prepared statement cache key in FirebirdExecuteStatementCommandExecutor.
• Generate cache keys from Firebird statement handles in the Firebird frontend; the shared backend-core does not add MySQL, PostgreSQL, or openGauss prepared statement cache behavior.
• Invalidate the matching cached prepared statement on Firebird DROP / UNPREPARE.
• Keep CLOSE limited to cursor/fetch cleanup and do not invalidate prepared statement cache entries.
• Add same-handle PREPARE cleanup so re-preparing an existing Firebird statement handle performs implicit unprepare-style resource cleanup.
• Clean up fetch handlers together with Firebird free/reprepare resource release.
• Close all cached prepared statements in ProxyDatabaseConnectionManager#closeConnections(...) as the final connection cleanup step.

Add and extend tests for Firebird-only cache behavior and lifecycle safety:

• PreparedStatementCacheContextTest
• PreparedStatementCacheKeyTest
• JDBCBackendStatementTest
• ConnectionSessionTest
• ProxyDatabaseConnectionManagerTest
• StandardDatabaseProxyConnectorTest
• FirebirdExecuteStatementCommandExecutorTest
• FirebirdPrepareStatementCommandExecutorTest
• FirebirdFreeStatementCommandExecutorTest
• FirebirdFetchStatementCacheTest
• FirebirdFetchStatementCommandExecutorTest
• FirebirdStatementResourceCleanerTest

The intended scope of this change is:

• Firebird only
• connection-level cache
• reduced physical prepare count for repeated execution of the same Firebird statement in Connection Held scenarios
• no prepared statement reuse after physical connection close
• shared backend-core generic hooks added only to support the Firebird implementation in this pull request, without enabling cache reuse for other databases

---

Before committing this PR, I'm sure that I have checked the following options:

• My code follows the code of conduct of this project.
• I have self-reviewed the commit code.
• I have (or in comment I request) added corresponding labels for the pull request.
• I have passed maven check locally : ./mvnw clean install -B -T1C -Dmaven.javadoc.skip -Dmaven.jacoco.skip -e.
• I have made corresponding changes to the documentation.
• I have added corresponding unit tests for my changes.
• I have updated the Release Notes of the current development version. For more details, see Update Release Note

[terrymanu]

Decision

• Merge Verdict: Not Mergeable
• Reviewed Scope: Latest PR head 908ef62b5500e6a9ebd7f6e8a315c1d58495f3c7; all 9 changed files under proxy/backend/core; related Proxy lifecycle paths in proxy/frontend/core, Firebird execute/fetch/free, MySQL execute/close/reset, PostgreSQL close/portal; official MySQL and PostgreSQL protocol docs for close/reset semantics.
• Not Reviewed Scope: No live Proxy smoke against Firebird/MySQL/PostgreSQL/openGauss; no Maven/Spotless/Checkstyle run; no GitHub Actions or CI status used.
• Need Expert Review: Yes, Proxy protocol/resource lifecycle and performance review are needed because this changes a shared high-frequency prepared-statement path.

Positive Feedback

• The implementation is moving in a useful direction by adding a bounded LRU cache, checking closed cached statements before reuse, clearing parameters before rebinding, and adding focused unit tests for cache hit/eviction/close behavior.

Major Issues

• [P1] Cache does not survive the normal auto-commit command lifecycle, so the reported prepared-statement reuse path is not proven fixed (proxy/backend/core/src/main/java/org/apache/shardingsphere/proxy/backend/connector/ProxyDatabaseConnectionManager.java:266)

  • Symptom: CommandExecutorTask.finish() calls closeExecutionResources() after each frontend command (proxy/frontend/core/src/main/java/org/apache/shardingsphere/proxy/frontend/command/CommandExecutorTask.java:136). For non connection-held transactions, closeExecutionResources() calls closeConnections(false) (proxy/backend/core/src/main/java/org/apache/shardingsphere/proxy/backend/connector/ProxyDatabaseConnectionManager.java:269), and the PR closes the whole prepared-statement cache there (proxy/backend/core/src/main/java/org/apache/shardingsphere/proxy/backend/connector/ProxyDatabaseConnectionManager.java:352).
  • Risk: In the usual auto-commit Proxy path, the physical connection and cache are closed after the command, so a later execution of the same server prepared statement prepares again. The new test exercises two direct JDBCBackendStatement.createStorageResource() calls on the same mocked connection (proxy/backend/core/src/test/java/org/apache/shardingsphere/proxy/backend/connector/jdbc/statement/JDBCBackendStatementTest.java:98), but it bypasses the real command finish/connection close path.
  • Action: Please add production-path validation for repeated prepared-statement execution through the Proxy command lifecycle, especially Firebird auto-commit update/query scenarios. If the intended scope is only connection-held transactions or suspended cursors, please state that explicitly and avoid presenting it as a general Proxy prepared-statement cache.

• [P1] Cached backend statements are not invalidated when frontend prepared statements are closed/reset/unprepared (proxy/backend/core/src/main/java/org/apache/shardingsphere/proxy/backend/connector/StandardDatabaseProxyConnector.java:392)

  • Symptom: StandardDatabaseProxyConnector.closeStatements() now skips cached PreparedStatements (proxy/backend/core/src/main/java/org/apache/shardingsphere/proxy/backend/connector/StandardDatabaseProxyConnector.java:395), but frontend close/reset paths only remove server-side registry entries: MySQL COM_STMT_CLOSE (proxy/frontend/dialect/mysql/src/main/java/org/apache/shardingsphere/proxy/frontend/mysql/command/query/binary/close/MySQLComStmtCloseExecutor.java:40), MySQL reset (proxy/frontend/dialect/mysql/src/main/java/org/apache/shardingsphere/proxy/frontend/mysql/command/admin/MySQLComResetConnectionExecutor.java:41), PostgreSQL close prepared statement (proxy/frontend/dialect/postgresql/src/main/java/org/apache/shardingsphere/proxy/frontend/postgresql/command/query/extended/close/PostgreSQLComCloseExecutor.java:47), and Firebird DROP/UNPREPARE (proxy/frontend/dialect/firebird/src/main/java/org/apache/shardingsphere/proxy/frontend/firebird/command/query/statement/free/FirebirdFreeStatementCommandExecutor.java:46).
  • Risk: When a backend connection is held, the cached physical prepared statement can outlive the frontend prepared-statement lifecycle. This conflicts with the documented behavior that MySQL COM_STMT_CLOSE deallocates a prepared statement, MySQL reset releases prepared statements, and PostgreSQL Close releases prepared statement or portal resources. See MySQL COM_STMT_CLOSE, mysql_reset_connection(), and PostgreSQL protocol Close.
  • Action: Please define ownership clearly: either make this an internal connection-level cache with documented safe invalidation rules, or tie invalidation to frontend close/reset/unprepare. Add regression tests for MySQL close/reset, PostgreSQL Close S, and Firebird DROP/UNPREPARE while the backend connection is held.

• [P1] Shared Proxy prepared-statement behavior is enabled for all dialects without cross-dialect counterexample coverage (proxy/backend/core/src/main/java/org/apache/shardingsphere/proxy/backend/session/ConnectionSession.java:68)

  • Symptom: Every ConnectionSession now constructs JDBCBackendStatement(this) (proxy/backend/core/src/main/java/org/apache/shardingsphere/proxy/backend/session/ConnectionSession.java:85), and JDBCBackendStatement uses the cache for all prepared-statement storage resources (proxy/backend/core/src/main/java/org/apache/shardingsphere/proxy/backend/connector/jdbc/statement/JDBCBackendStatement.java:87).
  • Risk: The issue is Firebird-driven, but the change affects MySQL, PostgreSQL/openGauss extended protocol, Firebird, SQL federation, and batch/preview paths that reuse the same statement manager. There is no config-disabled path or dialect guard, and no non-target counterexample test showing that MySQL/PostgreSQL close/reset/resource behavior remains compatible.
  • Action: Please either scope the cache to the dialect/topology that needs it, add an explicit config/feature flag with enabled and disabled tests, or add cross-dialect tests proving no resource-lifecycle or protocol regression for MySQL and PostgreSQL/openGauss.

Next Steps

• Rework the cache lifetime so it matches the physical connection lifetime and the intended frontend prepared-statement lifecycle.
• Add one-to-one production-path tests for: repeated Firebird prepared execute in auto-commit, repeated execute inside a held transaction, MySQL COM_STMT_CLOSE, MySQL COM_RESET_CONNECTION, PostgreSQL Close prepared statement, and Firebird DROP/UNPREPARE.
• Include a shared-path blast-radius note in the PR explaining affected Proxy dialects/features and why the behavior is safe for each.
• Run scoped verification with dependent modules, for example: ./mvnw -pl proxy/backend/core,proxy/frontend/dialect/firebird,proxy/frontend/dialect/mysql,proxy/frontend/dialect/postgresql -am -DskipITs -Dspotless.skip=true test, then run the repository formatting/style gates required by CODE_OF_CONDUCT.md:17 and CODE_OF_CONDUCT.md:19.

Evidence Supplement

• PR metadata and diff were fetched from GitHub and local refs; latest reviewed head was 908ef62b5500e6a9ebd7f6e8a315c1d58495f3c7.
• Commands used for read-only review included git fetch apache master:refs/remotes/apache/master pull/38644/head:refs/remotes/apache/pr/38644 (exit 0), git diff --name-status apache/master...apache/pr/38644 (exit 0), GitHub API reads for PR/files/reviews/comments (exit 0), and git grep/rg searches for lifecycle and computeIfAbsent usage.
• No dependency manifests, parser grammar, docs, generated files, or target/ files are changed.
• No direct ConcurrentHashMap#computeIfAbsent use was introduced in the reviewed Proxy backend path.
• Minimum additional evidence needed: intended ShardingSphere Proxy topology, target database/driver versions, reproducible prepare/execute/close/reset sequences with expected vs actual prepare counts, and tests mapped one-to-one to each lifecycle fix point.

[terrymanu]

Decision

• Merge Verdict: Not Mergeable
• Reviewed Scope: Latest PR head b9b2f772a79b39ae51279501499dc017f2cedc77; base ref master; local merge-base e2f7b4995547fa4d7cf1d2f6d868fbacb9713388; all 14 GitHub /pulls/38644/files entries matched the local merge-base diff. Reviewed Proxy backend prepared-statement cache/session/connection lifecycle and Firebird execute/free/fetch lifecycle. Also checked the non-target MySQL, PostgreSQL, and openGauss guard coverage in the changed tests.
• Not Reviewed Scope: No live Proxy smoke against Firebird/Jaybird or other dialects; no Maven/Spotless/Checkstyle run; no GitHub Actions or CI status used. SQL parser grammar and parser-family rules are not triggered by this PR.
• Need Expert Review: Yes, Firebird protocol/resource lifecycle and Proxy high-frequency prepared-statement performance should get focused maintainer review.

Positive Feedback

• The latest revision is closer to the intended scope: the cache is no longer enabled for all dialects, it is keyed by Firebird statement id plus SQL/connection identity, it has bounded LRU eviction, and the tests now cover non-Firebird bypass paths.

Major Issues

• [P1] DROP / UNPREPARE leaves the Firebird fetch resource alive (proxy/frontend/dialect/firebird/src/main/java/org/apache/shardingsphere/proxy/frontend/firebird/command/query/statement/free/FirebirdFreeStatementCommandExecutor.java:46)

  • Symptom: For DROP and UNPREPARE, the executor removes the server prepared statement and invalidates the prepared-statement cache, but it does not unregister the FirebirdFetchStatementCache entry or unmark the in-use backend handler. That cleanup exists only in the CLOSE branch at FirebirdFreeStatementCommandExecutor.java:51. A later fetch still resolves the handler from FirebirdFetchStatementCache at proxy/frontend/dialect/firebird/src/main/java/org/apache/shardingsphere/proxy/frontend/firebird/command/query/statement/fetch/FirebirdFetchStatementCommandExecutor.java:52.
  • Risk: Firebird's wire protocol documents that DSQL_unprepare closes resources for the statement handle and that unprepare/drop also close the cursor. See the official Firebird wire protocol "Statements / Free" section: https://www.firebirdsql.org/file/documentation/html/en/firebirddocs/wireprotocol/firebird-wire-protocol.html#_free. With the current code, a query execution can mark a handler in use at proxy/frontend/dialect/firebird/src/main/java/org/apache/shardingsphere/proxy/frontend/firebird/command/query/statement/execute/FirebirdExecuteStatementCommandExecutor.java:79, then DROP / UNPREPARE leaves that handler in the fetch cache and in the connection manager's in-use set. In a connection-held transaction, ProxyDatabaseConnectionManager.closeHandlers(false) skips in-use handlers at proxy/backend/core/src/main/java/org/apache/shardingsphere/proxy/backend/connector/ProxyDatabaseConnectionManager.java:307, so this can leak the backend resource and allow stale fetch behavior after the frontend statement has been freed.
  • Action: Please make DROP and UNPREPARE perform the cursor/fetch cleanup as well as cache invalidation: unregister the fetch handler, unmark the resource in use, and ensure the handler is closed through the normal lifecycle. Please add regression tests for EXECUTE query -> DROP, EXECUTE query -> UNPREPARE, and FETCH after each free option to prove the stale handler cannot be used.

• [P1] The root-cause reuse path is still validated only through split mocks, not the Proxy command lifecycle (proxy/backend/core/src/test/java/org/apache/shardingsphere/proxy/backend/connector/jdbc/statement/JDBCBackendStatementTest.java:102)

  • Symptom: JDBCBackendStatementTest proves the cache works when getCurrentFirebirdPreparedStatementId() is stubbed and JDBCBackendStatement is called directly. FirebirdExecuteStatementCommandExecutorTest verifies the begin/finish context calls, but it mocks ProxyBackendHandlerFactory.newInstance(...) at proxy/frontend/dialect/firebird/src/test/java/org/apache/shardingsphere/proxy/frontend/firebird/command/query/statement/execute/FirebirdExecuteStatementCommandExecutorTest.java:152, so the test never reaches StandardDatabaseProxyConnector -> DriverExecutionPrepareEngine -> JDBCBackendStatement.
  • Risk: The original failure path is Firebird EXECUTE -> backend prepare -> command finish / connection-held cleanup -> repeated EXECUTE. The current tests do not prove that repeated execution of the same Firebird statement id inside a held Proxy connection performs one physical Connection.prepareStatement(...), nor that the cache survives only the intended held-connection lifecycle and is released by ProxyDatabaseConnectionManager.closeExecutionResources() / closeConnections() at proxy/backend/core/src/main/java/org/apache/shardingsphere/proxy/backend/connector/ProxyDatabaseConnectionManager.java:266.
  • Action: Please add a production-path or integration-style unit test that runs repeated Firebird prepared execute through the real backend statement manager with a held connection and mocked physical Connection, then asserts the physical prepare count, parameter clearing/rebinding, and cleanup. Please include counterexamples for DROP / UNPREPARE, reprepare of the same Firebird statement handle, and the stated auto-commit no-reuse path if that limitation is intentional.

Next Steps

• Fix Firebird DROP / UNPREPARE so cache invalidation and cursor/fetch resource cleanup are consistent with Firebird free-statement semantics.
• Add one-to-one tests for repeated Firebird execute in a connection-held Proxy lifecycle, FETCH after DROP, FETCH after UNPREPARE, same-handle reprepare, and auto-commit no-reuse if it remains out of scope.
• After the fix, run scoped verification with dependent modules, for example: ./mvnw -pl proxy/backend/core,proxy/frontend/dialect/firebird -am -DskipITs -Dspotless.skip=true -Dsurefire.failIfNoSpecifiedTests=false -Dtest=PreparedStatementCacheContextTest,JDBCBackendStatementTest,ConnectionSessionTest,ProxyDatabaseConnectionManagerTest,StandardDatabaseProxyConnectorTest,FirebirdExecuteStatementCommandExecutorTest,FirebirdFreeStatementCommandExecutorTest test.
• Then run the repository formatting/style gates required by CODE_OF_CONDUCT.md: ./mvnw spotless:apply -Pcheck -T1C and ./mvnw checkstyle:check -Pcheck -T1C.

Multi-Round Comparison

• Fixed: The previous all-dialect activation concern is mostly addressed. The cache path is now guarded by physical database type Firebird plus a current Firebird statement id, and MySQL/PostgreSQL/openGauss bypass tests were added.
• Partially fixed: The previous cache-lifetime concern is narrowed by the PR description to connection-held scenarios, but the latest tests still do not validate the full held-connection command lifecycle.
• Partially fixed: Firebird DROP / UNPREPARE now invalidates the prepared-statement cache, but it still misses the required cursor/fetch handler cleanup, so the lifecycle blocker remains.

Evidence Supplement

• Review commands used: GitHub API reads for PR metadata/files/reviews/comments (exit 0), git fetch apache master:refs/remotes/apache/master pull/38644/head:refs/remotes/apache/pr/38644 (exit 0), git diff --name-status $(git merge-base apache/master apache/pr/38644)..apache/pr/38644 (exit 0), and a GitHub/local file-list comparison with no differences (exit 0).
• Risk scan commands found no changed dependency manifests and no direct computeIfAbsent introduction in the reviewed production diff.
• Local Maven verification was not run because the latest code is already blocked by the protocol/resource-lifecycle issue above.
• Minimum additional evidence needed: confirmed Proxy topology, Firebird/Jaybird versions, a minimal PREPARE -> EXECUTE -> FETCH -> FREE sequence with expected vs actual resource state, and tests mapped one-to-one to each lifecycle fix point.

[terrymanu]

Decision

• Merge Verdict: Not Mergeable
• Reviewed Scope: Latest PR head 086c60976a69a224944c7cde1206805ed698897c; base ref master; local merge-base e2f7b4995547fa4d7cf1d2f6d868fbacb9713388; all 17 GitHub /pulls/38644/files entries matched the local merge-base diff. Reviewed Proxy backend prepared-statement cache/session/connection lifecycle, Firebird execute/free/fetch lifecycle, related Firebird prepare-handle reuse semantics, and non-target MySQL/PostgreSQL/openGauss cache bypass coverage.
• Not Reviewed Scope: No live Proxy smoke against Firebird/Jaybird or other dialects; no Maven/Spotless/Checkstyle run; no GitHub Actions or CI status used. SQL parser grammar and parser-family rules are not triggered by this PR.
• Need Expert Review: Yes, Firebird protocol/resource lifecycle and Proxy high-frequency prepared-statement performance should get focused maintainer review.

Positive Feedback

• The latest revision fixes the previous DROP / UNPREPARE fetch-resource cleanup gap, keeps the cache Firebird-scoped, adds a held-connection reuse test, and keeps MySQL/PostgreSQL/openGauss on the original non-cache path.

Major Issues

• [P1] Same-handle Firebird PREPARE bypasses cache and cursor cleanup (proxy/frontend/dialect/firebird/src/main/java/org/apache/shardingsphere/proxy/frontend/firebird/command/query/statement/prepare/FirebirdPrepareStatementCommandExecutor.java:117)

  • Symptom: The PR invalidates cached physical prepared statements only from Firebird DROP / UNPREPARE (proxy/frontend/dialect/firebird/src/main/java/org/apache/shardingsphere/proxy/frontend/firebird/command/query/statement/free/FirebirdFreeStatementCommandExecutor.java:48). However, a valid-handle PREPARE overwrites the server prepared-statement registry at FirebirdPrepareStatementCommandExecutor.java:117 without invalidating PreparedStatementCacheContext, unregistering the fetch handler, or unmarking the in-use backend handler. The later FETCH path still resolves the handler by statement id at proxy/frontend/dialect/firebird/src/main/java/org/apache/shardingsphere/proxy/frontend/firebird/command/query/statement/fetch/FirebirdFetchStatementCommandExecutor.java:52.
  • Risk: Firebird's official wire protocol says DSQL_unprepare closes resources for the statement handle and that it is not necessary to unprepare before preparing new statement text on the same handle; it also states an existing allocated statement handle can be reused by another prepare. See https://www.firebirdsql.org/file/documentation/html/en/firebirddocs/wireprotocol/firebird-wire-protocol.html#_free. With the current patch, PREPARE handle 1 SELECT -> EXECUTE -> PREPARE handle 1 UPDATE can leave the old physical PreparedStatement cached until LRU/connection close and leave the old fetch backend handler usable or marked in-use, which conflicts with Firebird resource semantics and can leak held-transaction resources.
  • Action: Please make valid-handle PREPARE perform the same statement-id cleanup needed for an implicit unprepare: invalidate cached prepared statements for that statement id, unregister the fetch handler, unmark the in-use backend handler when present, and then replace the registry entry. Please add regression tests for EXECUTE query -> PREPARE same handle different SQL, FETCH after same-handle reprepare, and repeated execute after same-handle reprepare to prove the old physical statement is closed and not reused.

• [P1] The root reuse path is still validated through split mocks, not the real Firebird execute-to-backend prepare chain (proxy/frontend/dialect/firebird/src/test/java/org/apache/shardingsphere/proxy/frontend/firebird/command/query/statement/execute/FirebirdExecuteStatementCommandExecutorTest.java:152)

  • Symptom: FirebirdExecuteStatementCommandExecutorTest verifies beginFirebirdPreparedStatementExecution(...) and finishFirebirdPreparedStatementExecution(...), but it mocks ProxyBackendHandlerFactory.newInstance(...), so it never reaches StandardDatabaseProxyConnector -> DriverExecutionPrepareEngine -> JDBCBackendStatement. The held-transaction reuse test directly constructs JDBCBackendStatement and stubs getCurrentFirebirdPreparedStatementId() (proxy/backend/core/src/test/java/org/apache/shardingsphere/proxy/backend/connector/ProxyDatabaseConnectionManagerTest.java:519), while JDBCBackendStatementTest verifies reuse on a mocked connection only (proxy/backend/core/src/test/java/org/apache/shardingsphere/proxy/backend/connector/jdbc/statement/JDBCBackendStatementTest.java:102).
  • Risk: The original issue is repeated Firebird prepared execution causing repeated costly physical prepare operations, but the current tests still do not prove that the actual Firebird command lifecycle wires the frontend statement id into backend JDBC preparation and preserves the cached statement across the intended held-connection cleanup path.
  • Action: Please add a production-path or integration-style unit test that drives repeated Firebird prepared execute through the real backend statement manager with a held connection and mocked physical Connection, then asserts one physical prepareStatement(...), parameter clearing/rebinding, and cleanup after free/reprepare/connection close.

Next Steps

• Fix valid-handle Firebird PREPARE so it invalidates the old statement id's cached physical statements and fetch resources before replacing the registry entry.
• Add one-to-one tests for same-handle reprepare, fetch after reprepare, repeated execute in a held connection through the real backend prepare path, and cleanup after free/reprepare/connection close.
• Run scoped verification with dependent modules, for example: ./mvnw -pl proxy/backend/core,proxy/frontend/dialect/firebird -am -DskipITs -Dspotless.skip=true -Dsurefire.failIfNoSpecifiedTests=false -Dtest=PreparedStatementCacheContextTest,JDBCBackendStatementTest,ConnectionSessionTest,ProxyDatabaseConnectionManagerTest,StandardDatabaseProxyConnectorTest,FirebirdExecuteStatementCommandExecutorTest,FirebirdFreeStatementCommandExecutorTest,FirebirdFetchStatementCacheTest,FirebirdFetchStatementCommandExecutorTest test.
• Then run the repository formatting/style gates required by CODE_OF_CONDUCT.md:17 and CODE_OF_CONDUCT.md:19: ./mvnw spotless:apply -Pcheck -T1C and ./mvnw checkstyle:check -Pcheck -T1C.

Multi-Round Comparison

• Fixed: The previous all-dialect cache activation concern remains fixed; the cache path is Firebird-only and tests cover MySQL, PostgreSQL, and openGauss bypass behavior.
• Fixed: The previous Firebird DROP / UNPREPARE stale fetch-handler blocker is addressed in the latest revision.
• Partially fixed: The held-connection reuse lifecycle now has a direct ProxyDatabaseConnectionManagerTest, but the test still bypasses the actual Firebird command-to-backend prepare assembly chain.
• Not fixed: Same-handle Firebird reprepare remains unhandled and untested, even though Firebird protocol allows preparing new statement text on an existing handle without a prior explicit unprepare.

Evidence Supplement

• Issue context: apache/shardingsphere#37274 states the Firebird JDBC driver lacks prepared-statement caching, causing repeated costly prepare operations during repeated prepared execution: #37274.
• PR intent: apache/shardingsphere#38644 scopes the cache to Firebird held-connection scenarios and claims lifecycle alignment for Firebird prepared statement and fetch resources: #38644.
• Review commands used: GitHub API reads for PR metadata/files/reviews/comments (exit 0), git fetch apache master:refs/remotes/apache/master pull/38644/head:refs/remotes/apache/pr/38644 (exit 0), git diff --name-status apache/master...apache/pr/38644 (exit 0), sorted GitHub/local file-list comparison (exit 0), and targeted git show / git grep / rg inspections.
• No dependency manifests, parser grammar, docs, generated files, or target/ files are changed. No direct ConcurrentHashMap#computeIfAbsent was introduced in the reviewed production diff.
• Local Maven verification was not run because the latest code is already blocked by the Firebird same-handle reprepare lifecycle issue and the remaining production-path validation gap.

[terrymanu]

Decision

• Merge Verdict: Not Mergeable
• Reviewed Scope: PR #38644 latest head 4972504084f0171141898325b5507fb3d08f0845; reviewed the changed production and test code under proxy/backend/core and proxy/frontend/dialect/firebird, focusing on Issue #37274's Firebird prepared statement cache root cause and the shared Proxy backend blast radius.
• Not Reviewed Scope: GitHub Actions/check-run status was not used. No full-repository clean install, live Firebird Proxy smoke test, performance benchmark, or Jacoco coverage run was performed.
• Need Expert Review: Please adjust the architecture first, then ask a Firebird protocol maintainer to review it, because the expected behavior depends on Firebird statement-handle lifecycle semantics.

Major Issues

[P1] Firebird-specific semantics leak into shared proxy/backend/core

The fix direction is correct: the root cause of #37274 is that the Firebird JDBC driver lacks prepared statement caching, so Proxy needs to avoid repeatedly executing expensive physical prepareStatement calls. However, the current implementation places Firebird protocol state into shared backend core classes, which are also used by MySQL, PostgreSQL, openGauss, and other Proxy execution paths.

Concrete issues:

• ConnectionSession now stores the Firebird-specific field currentFirebirdPreparedStatementId: proxy/backend/core/src/main/java/org/apache/shardingsphere/proxy/backend/session/ConnectionSession.java:74
• ConnectionSession exposes Firebird-named methods: beginFirebirdPreparedStatementExecution, getCurrentFirebirdPreparedStatementId, finishFirebirdPreparedStatementExecution, and invalidateFirebirdPreparedStatementCache: proxy/backend/core/src/main/java/org/apache/shardingsphere/proxy/backend/session/ConnectionSession.java:143
• JDBCBackendStatement directly checks the database type string "Firebird" in the shared JDBC backend path: proxy/backend/core/src/main/java/org/apache/shardingsphere/proxy/backend/connector/jdbc/statement/JDBCBackendStatement.java:100

Shared backend core can provide a generic prepared statement cache mechanism, but it should not know that the mechanism is Firebird-specific, and it should not store Firebird statement ids.

Suggested direction:

• Keep only generic capabilities in proxy/backend/core:
  • a generic prepared statement cache container
  • logic to skip closing cached prepared statements after execution
  • logic to close cached prepared statements when the connection is closed
• Move Firebird statement id ownership, Firebird execution lifecycle, and DROP/UNPREPARE/reprepare cleanup decisions back into proxy/frontend/dialect/firebird.
• If ConnectionSession needs shared entry points, use generic names and a generic key, for example:
  • beginPreparedStatementCache(...)
  • getCurrentPreparedStatementCacheKey()
  • finishPreparedStatementCache()
  • invalidatePreparedStatementCache(...)
• The key should be a non-null generic cache key/token. The Firebird module should create this key from packet.getStatementId().
• FirebirdExecuteStatementCommandExecutor should activate the generic key only during Firebird execute, and clear it in finally.
• FirebirdStatementResourceCleaner should invalidate the cache with the same generic key for Firebird DROP, UNPREPARE, and reprepare.
• JDBCBackendStatement should not check "Firebird". It should only check whether the current session has an active generic prepared statement cache key. If no key is active, it should use the original connection.prepareStatement(...) path.

This keeps the necessary shared hook in backend core while moving Firebird protocol decisions back into the Firebird module.

[P1] JDBCBackendStatement should not add a nullable constructor path

JDBCBackendStatement adds a no-arg constructor that delegates to this(null): proxy/backend/core/src/main/java/org/apache/shardingsphere/proxy/backend/connector/jdbc/statement/JDBCBackendStatement.java:46. Then getPreparedStatement treats null == connectionSession as “cache disabled”: proxy/backend/core/src/main/java/org/apache/shardingsphere/proxy/backend/connector/jdbc/statement/JDBCBackendStatement.java:88.

This has two problems:

• null becomes a hidden mode switch: a null session means “do not use prepared statement cache”.
• The class can be publicly constructed in a partially initialized state, but the constructor signature does not express that mode.

Production code already creates it from ConnectionSession via new JDBCBackendStatement(this), so a nullable ConnectionSession is not required for the Firebird cache feature. Please remove the this(null) path and make JDBCBackendStatement always hold a non-null ConnectionSession. Whether the cache is enabled should be represented by the presence or absence of an active generic cache key, not by a null session.

Tests that currently use new JDBCBackendStatement() should be updated to pass a mocked or real ConnectionSession with no active cache key.

[P1] The current implementation systematically uses null sentinels for cache state

The nullable constructor is not isolated. This PR has several related null-sentinel designs:

• ConnectionSession#getCurrentFirebirdPreparedStatementId() returns a nullable Integer: proxy/backend/core/src/main/java/org/apache/shardingsphere/proxy/backend/session/ConnectionSession.java:152
• ConnectionSession#finishFirebirdPreparedStatementExecution() uses set(null) to mean “no active statement”: proxy/backend/core/src/main/java/org/apache/shardingsphere/proxy/backend/session/ConnectionSession.java:160
• JDBCBackendStatement#getFirebirdPreparedStatementId() returns null for non-Firebird database types: proxy/backend/core/src/main/java/org/apache/shardingsphere/proxy/backend/connector/jdbc/statement/JDBCBackendStatement.java:100
• The no-statement-id overload of PreparedStatementCacheContext#getOrCreate(...) delegates by passing null to another overload: proxy/backend/core/src/main/java/org/apache/shardingsphere/proxy/backend/session/PreparedStatementCacheContext.java:63
• PreparedStatementCacheContext#getOrCreate(...) accepts a nullable Integer statementId: proxy/backend/core/src/main/java/org/apache/shardingsphere/proxy/backend/session/PreparedStatementCacheContext.java:77
• PreparedStatementCacheContext.CacheKey stores a nullable statementId and handles null specially in equals / hashCode: proxy/backend/core/src/main/java/org/apache/shardingsphere/proxy/backend/session/PreparedStatementCacheContext.java:203

Together, these indicate that the current design uses null to express “Firebird prepared statement cache is not active”. That is unclear and risky for shared backend core.

Please change this to:

• represent active/inactive state with explicit APIs or an Optional return, not nullable parameters/fields
• create a non-null cache key whenever caching is active
• let JDBCBackendStatement use the original connection.prepareStatement(...) path when no active key exists
• make PreparedStatementCacheContext accept a non-null generic cache key/token instead of a nullable statement id

Test Scope Feedback

The test diff is large, but most of it is justified because this PR changes cross-layer lifecycle behavior:

• PreparedStatementCacheContextTest is necessary because the PR adds a public production type.
• JDBCBackendStatementTest should cover same Firebird statement id reuse, different id non-reuse, invalidation, and non-Firebird cache bypass.
• StandardDatabaseProxyConnectorTest has heavy setup, but it covers the shared execution/cleanup path where cached prepared statements could otherwise be closed too early or leaked.
• Firebird prepare/free/fetch/execute tests need to cover reprepare, DROP, UNPREPARE, CLOSE, and fetch handler cleanup boundaries.

After the architecture changes above, please update the tests accordingly:

• shared backend tests should assert behavior only for generic cache key present/absent cases
• Firebird tests should assert that the Firebird module creates, activates, clears, and invalidates the generic cache key
• non-Firebird tests should not rely on new JDBCBackendStatement() or a null session; they should verify that cache is bypassed when no active cache key exists

Verification

• Reviewer-run module tests on detached PR-head worktree 4972504084f0171141898325b5507fb3d08f0845: ./mvnw -pl proxy/backend/core,proxy/frontend/dialect/firebird -am -DskipITs -Dspotless.skip=true -Dcheckstyle.skip=true -Dsurefire.failIfNoSpecifiedTests=false -Dtest=ProxyDatabaseConnectionManagerTest,StandardDatabaseProxyConnectorTest,JDBCBackendStatementTest,ConnectionSessionTest,PreparedStatementCacheContextTest,FirebirdExecuteStatementCommandExecutorTest,FirebirdFetchStatementCacheTest,FirebirdFetchStatementCommandExecutorTest,FirebirdFreeStatementCommandExecutorTest,FirebirdPrepareStatementCommandExecutorTest test passed, exit 0.
• Module-level Spotless/Checkstyle checks for proxy/backend/core and proxy/frontend/dialect/firebird passed, exit 0.
• These checks show that the current functional path basically runs, but they do not remove the design blockers above. The main issue is shared backend core scope pollution and null-sentinel design, not a simple test failure.

[terrymanu]

Decision

• Merge Verdict: Not Mergeable
• Reviewed Scope: PR #38644 latest head 9ae891c5feb9a22569f4ce6138438d61207db93d; local merge-base 440a930fc7687baf9ece33f48243bf85cb3691d9; all 24 GitHub /pulls/38644/files entries matched the local triple-dot diff. Reviewed changed files under proxy/backend/core, proxy/frontend/dialect/firebird, RELEASE-NOTES.md, linked issue #37274, previous GitHub-visible review rounds, and related Proxy command cleanup paths.
• Not Reviewed Scope: No GitHub Actions/check-run status used. No live Proxy smoke against Firebird/Jaybird. No Maven, Spotless, Checkstyle, full build, performance benchmark, or Jacoco run. SQL parser grammar/parser-family review is not triggered by this PR.
• Need Expert Review: Yes, Firebird protocol/resource lifecycle and Proxy backend cleanup should receive focused maintainer review after the cleanup ordering is fixed.

Positive Feedback

• The latest revision fixes several previous design blockers: shared backend core now uses generic prepared-statement cache APIs, JDBCBackendStatement no longer has a nullable no-arg constructor, Firebird-specific statement id ownership moved back to the Firebird frontend, and held-connection reuse now has stronger StandardDatabaseProxyConnector coverage.
• The Firebird DROP, UNPREPARE, and same-handle PREPARE paths are now routed through one cleanup helper, which is the right direction for keeping statement-handle lifecycle behavior consistent.

Major Issues

• [P1] Cache invalidation can close the physical statement before the old handler is closed (proxy/backend/core/src/main/java/org/apache/shardingsphere/proxy/backend/session/PreparedStatementCacheContext.java:112)
  • Symptom: FirebirdStatementResourceCleaner.clean(..., true) invalidates the prepared-statement cache first (proxy/frontend/dialect/firebird/src/main/java/org/apache/shardingsphere/proxy/frontend/firebird/command/query/statement/FirebirdStatementResourceCleaner.java:52), and cache invalidation removes the entry and closes the cached PreparedStatement (proxy/backend/core/src/main/java/org/apache/shardingsphere/proxy/backend/session/PreparedStatementCacheContext.java:112). Only after that does the cleaner unmark the old fetch backend handler as no longer in use (proxy/frontend/dialect/firebird/src/main/java/org/apache/shardingsphere/proxy/frontend/firebird/command/query/statement/FirebirdStatementResourceCleaner.java:57). At command finish, closeExecutionResources() closes unmarked handlers (proxy/backend/core/src/main/java/org/apache/shardingsphere/proxy/backend/connector/ProxyDatabaseConnectionManager.java:304), and StandardDatabaseProxyConnector.closeStatements() checks whether the statement is still cached before deciding to skip it (proxy/backend/core/src/main/java/org/apache/shardingsphere/proxy/backend/connector/StandardDatabaseProxyConnector.java:394). Because invalidation already removed the cache entry, the old handler can treat the same already-closed PreparedStatement as non-cached and call cancel() / close() on it (proxy/backend/core/src/main/java/org/apache/shardingsphere/proxy/backend/connector/StandardDatabaseProxyConnector.java:397).
  • Risk: For EXECUTE query -> DROP, EXECUTE query -> UNPREPARE, and EXECUTE query -> PREPARE same handle, the old query handler can still own the cached statement when Firebird cleanup runs. This makes cleanup depend on driver-specific behavior for cancel() on a closed statement and can surface a cleanup SQLException after the Firebird command response. Firebird's official wire protocol says DSQL_unprepare closes resources for the statement handle, DROP effectively closes the cursor and unprepares the text, and an allocated statement handle can be reused by another prepare: https://www.firebirdsql.org/file/documentation/html/en/firebirddocs/wireprotocol/firebird-wire-protocol.html#_free. These are exactly the lifecycle paths this PR needs to make safe.
  • Action: Please reorder or remodel cleanup so the owner handler is detached or closed while its statement is still recognized as cache-owned, then invalidate and physically close the cached prepared statement. Alternatively, make invalidation mark the entry stale and delay physical close until the owning handler has cleared its cachedStatements. Please add regression tests that run DROP, UNPREPARE, and same-handle PREPARE after a query execution with an open fetch handler, then call closeExecutionResources() and assert no cleanup exception and no stale fetch handler.

Next Steps

• Fix the Firebird cleanup ordering between FirebirdStatementResourceCleaner, PreparedStatementCacheContext.invalidate(...), and StandardDatabaseProxyConnector.closeStatements().
• Add one-to-one regression tests for:
  • EXECUTE query -> DROP -> closeExecutionResources
  • EXECUTE query -> UNPREPARE -> closeExecutionResources
  • EXECUTE query -> PREPARE same handle -> closeExecutionResources
  • a driver-sensitive counterexample where the cached PreparedStatement would throw if cancel() is called after cache invalidation already closed it.
• Re-run scoped verification with dependent modules, for example: ./mvnw -pl proxy/backend/core,proxy/frontend/dialect/firebird -am -DskipITs -Dspotless.skip=true -Dsurefire.failIfNoSpecifiedTests=false -Dtest=PreparedStatementCacheContextTest,JDBCBackendStatementTest,ConnectionSessionTest,ProxyDatabaseConnectionManagerTest,StandardDatabaseProxyConnectorTest,FirebirdStatementResourceCleanerTest,FirebirdExecuteStatementCommandExecutorTest,FirebirdFreeStatementCommandExecutorTest,FirebirdFetchStatementCacheTest,FirebirdFetchStatementCommandExecutorTest,FirebirdPrepareStatementCommandExecutorTest test.
• Then run the repository formatting/style gates required by CODE_OF_CONDUCT.md: ./mvnw spotless:apply -Pcheck -T1C and ./mvnw checkstyle:check -Pcheck -T1C.

Multi-Round Comparison

• Fixed: The previous shared-core Firebird semantic leakage is addressed. Shared backend code now uses generic cache key APIs, and Firebird creates the key in proxy/frontend/dialect/firebird.
• Fixed: The previous nullable JDBCBackendStatement constructor and null-sentinel mode switch are removed.
• Fixed: Same-handle Firebird PREPARE now invokes statement resource cleanup before replacing the server prepared-statement registry entry.
• Improved: The root reuse path now has a stronger held-connection StandardDatabaseProxyConnectorTest that verifies one physical prepareStatement(...) across two executions with an active cache key.
• Still blocking: The latest code still lacks a safe, validated cleanup order for invalidating a cached physical statement while an old in-use query handler still owns that statement.

Evidence Supplement

• Issue context: #37274 asks for Proxy prepared-statement caching because Firebird/Jaybird lacks the efficient prepared-statement caching available in some other drivers: #37274.
• PR context: #38644 scopes the implementation to Firebird held-connection prepared-statement reuse: #38644.
• Review commands used: GitHub API reads for PR metadata/files/issue/reviews/comments (exit 0), git fetch apache +refs/heads/master:refs/remotes/apache/master +refs/pull/38644/head:refs/remotes/apache/pr/38644 (exit 0), git diff --name-status $(git merge-base apache/master apache/pr/38644)..apache/pr/38644 (exit 0), and GitHub/local file-list comparison (exit 0, no differences).
• No dependency manifests, parser grammar, generated files, or target/ files are changed. No direct ConcurrentHashMap#computeIfAbsent was introduced in the reviewed production diff; the grep matches are pre-existing Firebird blob upload cache code outside the changed production files.
• Local Maven verification was not run because the latest code is already blocked by the cleanup-order issue above.

[terrymanu]

Summary

• Merge Decision: Mergeable
• Reason: The latest head fixes the Firebird held-connection prepared-statement reuse root cause, closes the previous lifecycle/design blockers, and passes scoped PR-head verification with dependent modules.

Evidence

• Root-cause fit: Issue #37274 asks for Proxy prepared-statement caching because Firebird/Jaybird lacks driver-side reuse; the PR now scopes the feature to Firebird held connections and records that behavior in RELEASE-NOTES.md:48.
• Shared backend ownership is clean now: proxy/backend/core/src/main/java/org/apache/shardingsphere/proxy/backend/session/ConnectionSession.java:143 exposes generic cache-key activation, and proxy/backend/core/src/main/java/org/apache/shardingsphere/proxy/backend/connector/jdbc/statement/JDBCBackendStatement.java:86 uses only the active generic key, not Firebird-specific type checks.
• Non-target dialect behavior is preserved: when no cache key is active, JDBCBackendStatement keeps the original Connection.prepareStatement(...) path, and proxy/backend/core/src/test/java/org/apache/shardingsphere/proxy/backend/connector/jdbc/statement/JDBCBackendStatementTest.java:127 covers MySQL, PostgreSQL, and openGauss bypass scenarios.
• Firebird lifecycle is aligned with statement-handle semantics: DROP, UNPREPARE, and same-handle PREPARE route through FirebirdStatementResourceCleaner, which unregisters fetch state, removes the in-use handler, closes the handler, and invalidates the cached physical statement after handler cleanup (proxy/frontend/dialect/firebird/src/main/java/org/apache/shardingsphere/proxy/frontend/firebird/command/query/statement/FirebirdStatementResourceCleaner.java:55). This matches the Firebird wire-protocol Free behavior for close/drop/unprepare: https://www.firebirdsql.org/file/documentation/html/en/firebirddocs/wireprotocol/firebird-wire-protocol.html#_free.
• The previous cleanup-order blocker is covered by a driver-sensitive counterexample: proxy/backend/core/src/test/java/org/apache/shardingsphere/proxy/backend/connector/StandardDatabaseProxyConnectorTest.java:650 verifies no cancel-after-close path after handler cleanup and cache invalidation.
• The root reuse path is covered through the Proxy backend connector path: proxy/backend/core/src/test/java/org/apache/shardingsphere/proxy/backend/connector/StandardDatabaseProxyConnectorTest.java:521 verifies one physical prepareStatement(...) across repeated held-connection execution, while :562 and :603 cover invalidation and physical connection close boundaries.
• Previous GitHub-visible blockers are resolved in the latest version: all-dialect activation, Firebird leakage into shared backend core, nullable constructor/null sentinel state, stale fetch resources after DROP/UNPREPARE, same-handle reprepare cleanup, and invalidation-before-handler-close ordering are fixed.
• Risk scan found no dependency manifest, parser grammar, generated-file, or target/ changes, and no new ConcurrentHashMap#computeIfAbsent use in the changed high-frequency Proxy DML/DQL path.

Review Details

• Reviewed Scope: PR #38644 latest head 87ef8728c9917e23c00edba348cae4ce60fda822; base ref master; local merge-base 440a930fc7687baf9ece33f48243bf85cb3691d9; all 24 GitHub /pulls/38644/files entries matched the local triple-dot diff. Reviewed RELEASE-NOTES.md, changed proxy/backend/core production/tests, changed proxy/frontend/dialect/firebird production/tests, linked issue #37274, previous GitHub-visible review rounds, and related MySQL/PostgreSQL/openGauss cache-bypass behavior.
• Not Reviewed Scope: No GitHub Actions/check-run status used. No live Proxy smoke against Firebird/Jaybird or other databases, no performance benchmark, no Jacoco coverage run, and no full-repository clean install. SQL parser grammar/parser-family review is not triggered by this PR.
• Verification: On detached PR-head worktree /tmp/shardingsphere-pr38644-87ef872: ./mvnw -pl proxy/backend/core,proxy/frontend/dialect/firebird -am -DskipITs -Dspotless.skip=true -Dcheckstyle.skip=true -Dsurefire.failIfNoSpecifiedTests=false -Dtest=PreparedStatementCacheContextTest,PreparedStatementCacheKeyTest,JDBCBackendStatementTest,ConnectionSessionTest,ProxyDatabaseConnectionManagerTest,StandardDatabaseProxyConnectorTest,FirebirdStatementResourceCleanerTest,FirebirdExecuteStatementCommandExecutorTest,FirebirdFreeStatementCommandExecutorTest,FirebirdFetchStatementCacheTest,FirebirdFetchStatementCommandExecutorTest,FirebirdPrepareStatementCommandExecutorTest test passed, exit 0. ./mvnw -pl proxy/backend/core,proxy/frontend/dialect/firebird -am spotless:check -Pcheck -T1C passed, exit 0. ./mvnw -pl proxy/backend/core,proxy/frontend/dialect/firebird -am checkstyle:check -Pcheck -T1C passed, exit 0.

[makssent]

👍