-
Notifications
You must be signed in to change notification settings - Fork 6.9k
Fix/postgresql custom types PostgreSQL Custom Types Parsing Issue in ShardingSphere-Proxy #37216
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix/postgresql custom types PostgreSQL Custom Types Parsing Issue in ShardingSphere-Proxy #37216
Conversation
… 2. 其他数据库的MetaDataLoader ColumnMetaData 占位. 3. ColumnMetaData 和ShardingSphereColumn 新增type_name 字段. 用以Pg 识别自定义Type
… 2. 其他数据库的MetaDataLoader ColumnMetaData 占位. 3. ColumnMetaData 和ShardingSphereColumn 新增type_name 字段. 用以Pg 识别自定义Type
…语句 支持识别typeName. 4. ParseExec 阶段 支持TypeName 读取. final: 稳定版本. 自定义UDT 类型 update,insert ,select ,delete 本地项目无问题.
terrymanu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
• Problem Understanding
Thank you for tackling PostgreSQL custom types/VARBIT, but the current PR is very large (93 files, 878 additions, 470 deletions) and contains several regression risks. We need to narrow the scope and fix key issues before moving forward.
Root Cause
- Types.OTHER is globally remapped to a fabricated UDT_GENERIC OID (and it absorbs JSON/JSONB), so extended-protocol parameter/column OIDs become wrong; clients may mis-type or fail to bind (JSON must stay at OIDs 114/3802).
- PostgreSQLColumnType stores typeName by mutating the enum singleton (withTypeName), which risks cross-session/cross-statement leakage and concurrency issues.
- PostgreSQLUDTValueParser builds PGobject without setType; Bind also assumes typeName is non-null, so null typeName leads to Bind-time exceptions.
- Multiple metadata loaders/tests inject the Chinese placeholder "占位" as typeName, violating English-only readability and polluting metadata/YAML.
Analysis
- PostgreSQL extended protocol requires exact type OIDs; sending JSON/JSONB as a fabricated UDT OID breaks driver/client compatibility.
- Enums are global singletons; writing mutable state (typeName) onto them invites races and cross-request contamination.
- PGobject must have setType to carry the real type; absent typeName needs a safe fallback to avoid Bind failures.
- Large, unrelated edits (H2/MySQL/SQLServer/Firebird, etc.) only to add placeholders are unnecessary and enlarge regression surface.
Conclusion & Requests (please simplify and fix accordingly)
- Narrow changes strictly to PostgreSQL custom types/VARBIT; drop unrelated placeholder/typeName edits across other dialects/tests (avoid a 90+ file change).
- Restore correct JSON/JSONB OIDs; only attach column typeName as a side-car for non-JSON/VARBIT/UUID Types.OTHER, without altering enum mappings.
- Remove mutable state from the enum; keep typeName alongside parameters, not stored on the enum instance.
- In UDT/JSONB binding, always setType(resolvedTypeName) on PGobject and add a safe fallback when typeName is missing to prevent Bind-time NPE/SQLException.
- Remove all placeholder/non-English strings; use real column typeNames or leave blank without polluting metadata.
- Add/adjust tests to cover: JSON/JSONB OIDs unchanged, VARBIT path, UDT binding with/without typeName, and no shared state across statements/sessions.
Please resubmit with only the necessary PostgreSQL fixes and the strengthened tests so the review can proceed efficiently.
|
Thank you for the detailed review and clarification. I have now narrowed this PR strictly to the PostgreSQL custom type / VARBIT Summary of completed fixes:
These changes now address the core regressions described in your review. Remaining work: Regarding mvnw test failures: they are caused only by the reverted non-PG Thank you again for the review. The PR is now narrowly scoped, low-risk, |
terrymanu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your changes are even more extensive than last time, reaching 163 modifications. I don't think anyone can thoroughly review it or fully assess its impact in a short time. Could you please break this PR into smaller ones, keeping the changes within 10 classes?
…t-wide UDT loading Enhances PostgreSQL type consumption by adding typeName support across metadata, loaders, and prepared statements. Introduces loadUDTTypes in dialects and updates PG-specific bindings, parsing, describing, and column swapping logic.
d81392b to
33ebc87
Compare
|
@terrymanu Thank you again for the detailed review and guidance. This is my first time contributing to an open-source project of this size.If there is anything I am not handling correctly, please kindly point it out —I will strictly follow your suggestions. I really hope to contribute to thePostgreSQL custom UDT support together with the community. Regarding the PR size: After removing all unrelated dialect changes and restoring the enum to a At this point, the PR is coherent and its intention is clear.However, if you would still prefer to split it further into sub-PRs (each under 10 classes), I am absolutely willing to do so. Before proceeding, could you please confirm whether: • the current 18-file PR is acceptable for review, If splitting is required, I can reorganize the changes according to clear
Or I can follow any grouping you prefer. My goal is to keep the changes clear, focused, and low-risk so that the review process becomes easier for everyone. Thank you again for your patience and guidance. I will update the PR immediately according to your confirmation. |
|
Hi @terrymanu , thanks again for your guidance. I have now split the original large PR into two smaller and focused PRs, each within the 10-class limit: Metadata & typeName pipeline – 8 files Extended protocol fixes – 10 files Both PRs isolate the changes into clear functional boundaries (metadata vs. protocol) and avoid unnecessary modifications outside the PostgreSQL scope. The original parent PR (#37216) has also been updated to reference these two sub-PRs. If these PRs still do not meet the review expectations, could you please let me know the specific problems or concerns? I take this contribution very seriously and I’m willing to carefully adjust and improve the changes according to your suggestions. Thank you again for your time and review. 🙏 |
|
The CI is broken, could you fix it first? |
|
@terrymanu Thanks for the feedback. I will fix all CI failures first. The current failures are mainly caused by the introduction of For easier review, I plan to open a new PR that includes:
This new PR will ensure that the entire build passes, including: However, since many test classes need to be adjusted, that PR will Thanks again for your patience. I will update the PR soon. |
|
Hi @terrymanu, Following your feedback, I have created a new PR #37259, which includes:
These include the fixes for: PostgreSQL custom types (UDT/enum/domain/composite) VARBIT / BIT VARYING Extended protocol: Parse → Bind → Describe → Execute typeName propagation across metadata and prepared statements
Because typeName was added to: ColumnMetaData ShardingSphereColumn ColumnReviseEngine binder & extended protocol structures many unit tests and E2E tests needed adjustments. ./mvnw spotless:apply -Pcheck 📌 Purpose of splitting To keep this PR (#37216) focused and easy to review, I intentionally did not include the large amount of test modifications here. If you prefer, I can also merge the test updates back into this PR, but creating a separate complete PR seemed clearer based on your earlier suggestion of reducing change size. I’m very open to any further adjustments. |
terrymanu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please fix GitHub action first
|
Closed because of no response anymore. |
Fixes #36978
Changes proposed in this pull request
This PR fixes the incorrect handling of PostgreSQL custom types (UDT, enum) and VARBIT/BIT VARYING in ShardingSphere-Proxy, especially under the extended query protocol (Parse → Bind → Describe → Execute).
The main issue was that all PostgreSQL Types.OTHER were incorrectly mapped to JSON, causing protocol inconsistencies and incorrect parameter/column parsing.
Added explicit support for PostgreSQL VARBIT and VARBIT_ARRAY with a dedicated value parser.
Introduced UDT_GENERIC to represent PostgreSQL user-defined types (enum, domain, composite).
Added detection logic based on both JDBC type and actual columnTypeName.
Removed the incorrect fallback mapping: Types.OTHER → JSON.
Parse phase now propagates both parameterTypes and parameterTypeNames into PostgreSQLServerPreparedStatement.
Ensures downstream Bind/Describe phases can resolve correct PostgreSQL types instead of JSON.
Insert statements now resolve column types (dataType, typeName) from ShardingSphere metadata.
Automatically assigns accurate PostgreSQL column types to each parameter (including UDT + VARBIT).
Saves resolved typeName into the prepared statement for Bind phase use.
Construct proper PGobject when binding UDT or JSONB parameters using the resolved typeName.
Prevent JSON fallback for VARBIT and UDT parameters.
Ensures PostgreSQL receives correct parameter type during Bind.
Added typeName to ColumnMetaData and ShardingSphereColumn.
PostgreSQLDataTypeOption updated to:
Include VARBIT / BIT VARYING in extra types.
Load all PostgreSQL UDTs dynamically via pg_type (enum, composite, domain).
Verified on PostgreSQL 17.5 with real application workloads:
INSERT: VARBIT and custom UDT work correctly.
SELECT: raw VARBIT bitstring + UDT enum returned correctly.
UPDATE: bitwise OR on VARBIT executed successfully.
No JSON fallback or protocol errors.
This fully resolves issue #36978.
Before committing this PR, I'm sure that I have checked the following options:
Checklist
./mvnw clean install -B -T1C -Dmaven.javadoc.skip -Dmaven.jacoco.skip -e(https://shardingsphere.apache.org/community/en/involved/contribute/contributor/)