terrymanu commented on PR #39241: URL: https://github.com/apache/shardingsphere/pull/39241#issuecomment-5088002564
### Summary **Review Result: Mergeable** **Reason:** Within the agreed review scope, this PR correctly resolves and propagates dynamic PostgreSQL type OIDs for composite result columns and rejects unsafe composite results routed to multiple `ExecutionUnit`s before execution. No other blocking code, test, interface, performance, or formatting issues were found. This result represents code-scope readiness only; CI was not reviewed by request. ### Evidence - [`PostgreSQLColumnTypeOIDLoader`](https://github.com/apache/shardingsphere/blob/9ca8a67e328e84b73510e3f4fad4ef71526cb592/database/protocol/dialect/postgresql/src/main/java/org/apache/shardingsphere/database/protocol/postgresql/type/PostgreSQLColumnTypeOIDLoader.java#L48-L63) handles only JDBC `STRUCT` columns and resolves their actual OIDs through the corresponding PostgreSQL JDBC connection’s `TypeInfo`, avoiding static type mappings. - [`PostgreSQLQueryHeaderBuilder`](https://github.com/apache/shardingsphere/blob/9ca8a67e328e84b73510e3f4fad4ef71526cb592/proxy/backend/dialect/postgresql/src/main/java/org/apache/shardingsphere/proxy/backend/postgresql/response/header/query/PostgreSQLQueryHeaderBuilder.java#L53-L71) loads the OID from the JDBC connection associated with the actual result and propagates it through `QueryHeader.protocolAttributes`, without introducing a PostgreSQL-specific field into the shared header. - [`ProxySQLExecutor`](https://github.com/apache/shardingsphere/blob/9ca8a67e328e84b73510e3f4fad4ef71526cb592/proxy/backend/core/src/main/java/org/apache/shardingsphere/proxy/backend/connector/ProxySQLExecutor.java#L242-L251) performs the multiple-execution-unit metadata check before transaction hooks and actual SQL execution. - [`PostgreSQLPreparedStatementMetadataFactory`](https://github.com/apache/shardingsphere/blob/9ca8a67e328e84b73510e3f4fad4ef71526cb592/proxy/frontend/dialect/postgresql/src/main/java/org/apache/shardingsphere/proxy/frontend/postgresql/command/query/extended/PostgreSQLPreparedStatementMetadataFactory.java#L80-L94) applies the same boundary to Statement Describe and closes the newly created `PreparedStatement` when validation fails. - The latest implementation checks the number of `ExecutionUnit`s directly. If multiple execution units produce a `STRUCT` result, execution is rejected even when they belong to the same storage unit. - The relevant tests cover OID loading, column descriptions, pre-execution validation, query-header construction, Simple Query, Portal, prepared-statement metadata, and Statement Describe. ### Review Details - **Review Focus:** Code Correctness Review — CI not reviewed by request - **Reviewed Scope:** All 28 files changed at the latest PR head, `9ca8a67e328e84b73510e3f4fad4ef71526cb592`, including `RELEASE-NOTES.md`, the PostgreSQL protocol module, Proxy backend core and PostgreSQL modules, PostgreSQL and MySQL frontend modules, SPI registration, and related tests. The base and local merge-base are both `b084c23351448396c4c9e22b927268abd5748742`. The local triple-dot file list exactly matches GitHub `/pulls/39241/files`. - **Not Reviewed Scope:** Composite type OIDs for Binary Portal results and SQL Federation are explicitly deferred to follow-up PRs. GitHub Actions/check runs and live PostgreSQL client E2E tests were not reviewed. - **Verification:** - `./mvnw spotless:check -Pcheck -T1C`: exit code 0, `BUILD SUCCESS`. - `./mvnw checkstyle:check -Pcheck -T1C`: exit code 0, 0 Checkstyle violations. - 11 directly related test classes across 4 Maven modules: 153 tests, 0 failures, 0 errors, exit code 0. - **Release Note / User Docs:** `RELEASE-NOTES.md` contains an entry for the PostgreSQL composite-column type OID fix and is sufficient for the current PR scope. User-visible behavior for Binary Portal results and SQL Federation should be documented separately by their follow-up PRs. -- 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]
