Fix swapped names in PostgreSQL undefined column error message#39125
Fix swapped names in PostgreSQL undefined column error message#39125thswlsqls wants to merge 1 commit into
Conversation
terrymanu
left a comment
There was a problem hiding this comment.
Summary
Review Result: Not Mergeable
Feedback Mode: Needs Discussion
Reason: The patch correctly reorders arguments for the dialect exception in isolation, but that exception is not produced by the PostgreSQL SELECT binding path described in #39123. The linked issue’s root-cause model and the implementation scope need to be reconciled before this can close the reported bug.
Issues
P1: The changed mapper is not on the reported SELECT failure path
- Problem: #39123 reports selecting a nonexistent column. However, the relevant dialect
ColumnNotFoundExceptionis constructed by PostgreSQL only inside the prepared-statementINSERTDescribe branch at PostgreSQLComDescribeExecutor.java:106. A normal projected column is bound through ColumnProjectionSegmentBinder.java:46, where ColumnSegmentBinder.java:216 throws the separate infraColumnNotFoundException. That exception extendsShardingSphereSQLExceptionand is converted before the dialect-mapper branch at SQLExceptionTransformEngine.java:49. The new test at PostgreSQLDialectExceptionMapperTest.java:83 mocks the mapper input directly, so it does not establish the reported Proxy path. - Impact: Merging with
Fixes #39123could close the reportedSELECTbug while only changing the extended-protocolINSERTDescribe behavior. - Discussion Needed: Please pause the current implementation direction and confirm the exact SQL and protocol phase that reproduces #39123. If it is a
SELECT, the discussion should identify the binder/error-transformation owner. If it is actually anINSERTDescribe failure, the issue and PR scope should first be narrowed accordingly. Thetype: discussionlabel is appropriate until this path is agreed.
Review Details
- Review Focus: Code Correctness Review — CI not reviewed by request.
- Reviewed Scope: All three changed files:
RELEASE-NOTES.md,PostgreSQLDialectExceptionMapper.java, andPostgreSQLDialectExceptionMapperTest.java; supporting exception definitions, production construction sites, PostgreSQL Describe handling, column binding, and exception transformation were traced. Latest head:0eddd07cef46ead1ee6bbab3f34107e13ee32221; base:masterat7568693e8ee02637a47d3cca8acbf449797932e5; local merge-base:6778bfbe08c1ff59574874575a6ab4b932066dba. The local triple-dot file list matched GitHub’s/pulls/39125/fileslist. - Not Reviewed Scope: GitHub Actions/CI and live Proxy/client reproduction were not reviewed. Unrelated modules were inspected only where necessary to trace the exception path.
- Verification: GitHub PR, file, commit, issue, comment, and review evidence was accessible. The bounded review inventory, merge-base comparison, exact-head source inspection, and repository-wide usage searches completed successfully. Maven was not run because the active checkout was not the PR head; no stale local test result was used. This does not affect the blocker because the path mismatch is established directly from the latest-head production call graph.
- Release Note / User Docs: A release-note entry is present, but its applicability depends on the resolved issue scope. Separate user documentation is not required because no configuration, API, migration, or compatibility contract changes.
|
Agreed on the mechanics: the mapper only sees the dialect That said, the swapped |
Fixes #39123.
Changes proposed in this pull request:
PostgreSQLVendorError.UNDEFINED_COLUMNinPostgreSQLDialectExceptionMapper.convert()so the reason formatColumn "%s" of table "%s" does not existreceives the column name first and the table name second. A missing columnuser_idin tablet_ordernow producesColumn "user_id" of table "t_order" does not existinstead of the reversed message.PRIVILEGE_NOT_GRANTED,INVALID_PARAMETER_VALUE,PROTOCOL_VIOLATION) that already match field order to format order.assertConvertColumnNotFoundMessageasserting the formatted message body; the existingassertConvertonly checked sqlState and severity, so it missed the swap.Before committing this PR, I'm sure that I have checked the following options:
./mvnw clean install -B -T1C -Dmaven.javadoc.skip -Dmaven.jacoco.skip -e.