terrymanu commented on PR #39225: URL: https://github.com/apache/shardingsphere/pull/39225#issuecomment-5261816985
### Result **Review Result: Not Mergeable** Feedback Mode: Needs Discussion **Blocking Issues: 4** The general ownership direction is reasonable: PostgreSQL extended-protocol state should own parameter-type resolution, schema metadata may supply native type names, and Binder should remain the semantic owner. However, the current implementation cannot represent arbitrary PostgreSQL types and treats incomplete metadata inference as complete. The replacement state model and fallback contract must be agreed before further implementation. ### Decisions Required Before Further Implementation The PR needs agreement on: 1. How each prepared-statement parameter represents standard, JSON, JSONB, and dynamic custom types, including native type name, wire OID, decoding strategy, and unresolved state. 2. When raw values in Bind-first flows are converted, and how resolved state is reused by repeated Bind and batch execution. 3. What constitutes complete Binder/schema-metadata resolution, and when Describe must fall back to JDBC metadata. 4. How RETURNING projections and PostgreSQL/openGauss composite fields obtain their actual wire-level OIDs. ### Blocking Issues #### 1. The parameter state model cannot preserve native custom type identity **Evidence** - [`PostgreSQLServerPreparedStatement`](https://github.com/apache/shardingsphere/blob/1e6473ac5e8afce4619ead50d164273b8061f775/proxy/frontend/dialect/postgresql/src/main/java/org/apache/shardingsphere/proxy/frontend/postgresql/command/query/extended/PostgreSQLServerPreparedStatement.java#L49-L62) stores only `List<PostgreSQLBinaryColumnType>`. - [`PostgreSQLBinaryColumnType.valueOfJDBCType`](https://github.com/apache/shardingsphere/blob/1e6473ac5e8afce4619ead50d164273b8061f775/database/protocol/dialect/postgresql/src/main/java/org/apache/shardingsphere/database/protocol/postgresql/packet/command/query/extended/PostgreSQLBinaryColumnType.java#L225-L244) maps every unrecognized `Types.OTHER` value to `VARCHAR`. - [`PostgreSQLPreparedStatementParameterTypeResolverTest`](https://github.com/apache/shardingsphere/blob/1e6473ac5e8afce4619ead50d164273b8061f775/proxy/frontend/dialect/postgresql/src/test/java/org/apache/shardingsphere/proxy/frontend/postgresql/command/query/extended/PostgreSQLPreparedStatementParameterTypeResolverTest.java#L300-L339) supplies `my_custom_type` and `my_enum` as native type names but expects both to become `VARCHAR`. - `JSONB` uses [`PostgreSQLJsonValueParser`](https://github.com/apache/shardingsphere/blob/1e6473ac5e8afce4619ead50d164273b8061f775/database/protocol/dialect/postgresql/src/main/java/org/apache/shardingsphere/database/protocol/postgresql/packet/command/query/extended/bind/protocol/text/impl/PostgreSQLJsonValueParser.java#L29-L36), which always creates a `PGobject` whose type is `json`. - PostgreSQL requires `ParameterDescription` to contain each parameter’s data-type object ID. [PostgreSQL protocol specification](https://www.postgresql.org/docs/current/protocol-message-formats.html#PROTOCOL-MESSAGE-FORMATS-PARAMETERDESCRIPTION) **Impact** Enums, domains, and other native types reported as `Types.OTHER` are described and subsequently parsed as `VARCHAR`. JSONB can be bound as JSON. Describe-first, repeated Bind, and batch execution therefore cannot preserve the actual native type identity required by the PR. **Discussion Needed** Agree on a per-parameter state contract capable of representing both fixed protocol types and database-specific native types. The decision must also define raw Bind-first handling, wire-OID resolution, and reuse across repeated and batched execution. Extending the current enum with more special cases cannot represent arbitrary custom types. #### 2. Per-marker schema ownership is ignored **Evidence** - [`ColumnSegmentBoundInfo`](https://github.com/apache/shardingsphere/blob/1e6473ac5e8afce4619ead50d164273b8061f775/parser/sql/statement/core/src/main/java/org/apache/shardingsphere/sql/parser/statement/core/segment/generic/bound/ColumnSegmentBoundInfo.java#L32-L69) records the original database, schema, table, and column for each bound column. - [`tryResolveFromBoundAST`](https://github.com/apache/shardingsphere/blob/1e6473ac5e8afce4619ead50d164273b8061f775/proxy/frontend/dialect/postgresql/src/main/java/org/apache/shardingsphere/proxy/frontend/postgresql/command/query/extended/PostgreSQLPreparedStatementParameterTypeResolver.java#L120-L164) selects one statement-wide schema from `TablesContext` and uses only each marker’s table and column names. - If the selected schema contains a same-named table and column, the resolver reports complete success and skips JDBC fallback even when the marker belongs to another schema. **Impact** Schema-qualified and cross-schema statements can resolve parameters against the wrong table. This becomes externally visible when same-named columns have different native types or OIDs. **Discussion Needed** Define whether marker resolution must always use the complete bound owner or whether some statement-level fallback is permitted. The completeness contract must prevent a lookup in a different same-named schema from being treated as a successful resolution. #### 3. The DML Describe path treats partial inference as complete **Evidence** - [`tryDescribePreparedStatement`](https://github.com/apache/shardingsphere/blob/1e6473ac5e8afce4619ead50d164273b8061f775/proxy/frontend/dialect/postgresql/src/main/java/org/apache/shardingsphere/proxy/frontend/postgresql/command/query/extended/describe/PostgreSQLComDescribeExecutor.java#L100-L111) ignores the result of `tryResolveFromBoundAST` and unconditionally bypasses JDBC metadata for every `INSERT`, `UPDATE`, and `DELETE`. - Binder propagation currently covers only selected direct expression forms. For example, [`AssignmentSegmentBinder`](https://github.com/apache/shardingsphere/blob/1e6473ac5e8afce4619ead50d164273b8061f775/infra/binder/core/src/main/java/org/apache/shardingsphere/infra/binder/engine/segment/dml/assign/AssignmentSegmentBinder.java#L80-L86) annotates the value only when it is directly a `ParameterMarkerExpressionSegment`. - [`ExpressionSegmentBinder`](https://github.com/apache/shardingsphere/blob/1e6473ac5e8afce4619ead50d164273b8061f775/infra/binder/core/src/main/java/org/apache/shardingsphere/infra/binder/engine/segment/dml/expression/ExpressionSegmentBinder.java#L82-L149) does not bind `TypeCastExpression`; unsupported expressions are returned unchanged. - [`describeReturning`](https://github.com/apache/shardingsphere/blob/1e6473ac5e8afce4619ead50d164273b8061f775/proxy/frontend/dialect/postgresql/src/main/java/org/apache/shardingsphere/proxy/frontend/postgresql/command/query/extended/describe/PostgreSQLComDescribeExecutor.java#L147-L170) selects the first table and uses it for every RETURNING projection. - PostgreSQL permits RETURNING expressions to reference tables from `UPDATE ... FROM` and `DELETE ... USING`. [UPDATE documentation](https://www.postgresql.org/docs/current/sql-update.html), [DELETE documentation](https://www.postgresql.org/docs/current/sql-delete.html) **Impact** Nested parameters can remain `UNSPECIFIED` while Describe still returns without JDBC fallback. Multi-table RETURNING projections can receive the type of an unrelated same-named column or fall back to `VARCHAR`. Clients may consequently receive incorrect ParameterDescription and RowDescription packets. **Discussion Needed** Agree on the completeness boundary for metadata-only DML Describe. In particular, determine whether unsupported parameter or projection shapes must trigger whole-statement JDBC fallback, and establish which existing semantic owner supplies each RETURNING projection’s table and type. Continuing to add isolated AST cases would leave the same completeness problem. #### 4. PostgreSQL/openGauss composite type OID resolution is removed **Evidence** - [`populateColumnTypes`](https://github.com/apache/shardingsphere/blob/1e6473ac5e8afce4619ead50d164273b8061f775/proxy/frontend/dialect/postgresql/src/main/java/org/apache/shardingsphere/proxy/frontend/postgresql/command/query/extended/describe/PostgreSQLComDescribeExecutor.java#L211-L232) no longer loads actual column OIDs from the database. - [`PostgreSQLColumnDescription`](https://github.com/apache/shardingsphere/blob/1e6473ac5e8afce4619ead50d164273b8061f775/database/protocol/dialect/postgresql/src/main/java/org/apache/shardingsphere/database/protocol/postgresql/packet/command/query/PostgreSQLColumnDescription.java#L49-L58) converts the JDBC type through `PostgreSQLBinaryColumnType`; `Types.STRUCT` consequently becomes `VARCHAR`. - The PR removes the PostgreSQL and openGauss dialect-specific `ColumnTypeOIDResolver` wiring and `ColumnTypeOIDLoader` usage established by [#39241](https://github.com/apache/shardingsphere/pull/39241) and [#39253](https://github.com/apache/shardingsphere/pull/39253). - PostgreSQL RowDescription requires the field’s actual data-type object ID. [PostgreSQL protocol specification](https://www.postgresql.org/docs/current/protocol-message-formats.html#PROTOCOL-MESSAGE-FORMATS-ROWDESCRIPTION) **Impact** Composite fields can be reported as `VARCHAR` OID 1043 instead of their actual database OID, causing clients to select an incorrect result decoder. **Discussion Needed** Confirm whether removing dynamic composite OID resolution is an intentional compatibility change. If no compatibility break is intended, the agreed Describe design must preserve equivalent PostgreSQL and openGauss behavior in both JDBC-backed and metadata-only result-description paths. ### Test Boundary Several resolver and Describe tests manually inject `ColumnSegmentBoundInfo`, including [`PostgreSQLPreparedStatementParameterTypeResolverTest`](https://github.com/apache/shardingsphere/blob/1e6473ac5e8afce4619ead50d164273b8061f775/proxy/frontend/dialect/postgresql/src/test/java/org/apache/shardingsphere/proxy/frontend/postgresql/command/query/extended/PostgreSQLPreparedStatementParameterTypeResolverTest.java#L426-L435) and [`PostgreSQLComDescribeExecutorTest`](https://github.com/apache/shardingsphere/blob/1e6473ac5e8afce4619ead50d164273b8061f775/proxy/frontend/dialect/postgresql/src/test/java/org/apache/shardingsphere/proxy/frontend/postgresql/command/query/extended/describe/PostgreSQLComDescribeExecutorTest.java#L601-L607). These tests bypass the changed Binder production path and therefore cannot establish end-to-end semantic resolution. The agreed design should define expected behavior for: - Describe-first, Bind-first, repeated Bind, and batch execution; - ordinary `VARCHAR`, JSON, JSONB, and arbitrary custom `Types.OTHER`; - schema-qualified and cross-schema multi-table statements; - nested expressions, `UPDATE ... FROM`, `DELETE ... USING`, and RETURNING; - PostgreSQL and openGauss composite type RowDescription OIDs; - the already-resolved text fast path without additional routing, connection acquisition, or JDBC metadata access. ### Coverage - Reviewed head: `1e6473ac5e8afce4619ead50d164273b8061f775` - Base: `master` at `ec31978cbc7eeccd59c86c57a7f073595f7d0749` - Scope: all 24 files in GitHub’s authoritative changed-file list across 11 commits. - Requirements: [issue #36978](https://github.com/apache/shardingsphere/issues/36978), the PR description, and the public [extended-protocol ownership boundary](https://github.com/apache/shardingsphere/pull/39225#issuecomment-5242574708). - Behavior clusters: shared native column metadata and YAML persistence; PostgreSQL protocol type mapping and parameter state; Binder semantic propagation; PostgreSQL/openGauss Describe, Bind, fallback, and tests. - Review lenses completed: root cause and behavior; blast radius and contracts; tests, runtime, and operations. - No unresolved evidence gap remains that could change the blocker set. - This is a Code Correctness Review only. GitHub Actions and CI were not reviewed. -- 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]
