You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@shardingsphere.apache.org by "iamhucong (via GitHub)" <gi...@apache.org> on 2026/08/11 08:01:17 UTC
[PR] Use payload charset for PostgreSQL binary strings [shardingsphere]
iamhucong opened a new pull request, #39409:
URL: https://github.com/apache/shardingsphere/pull/39409
Changes proposed in this pull request:
- Decode PostgreSQL `VARCHAR`, `CHAR`, and unspecified binary bind values with the packet payload charset.
- Strengthen both read tests with non-ASCII values encoded using a non-default multibyte charset.
The PostgreSQL frontend stores its negotiated charset in `PostgreSQLPacketPayload`. The affected readers used `new String(byte[])`, which instead depends on the JVM default charset. When those charsets differ, non-ASCII binary bind values are corrupted before reaching statement execution. The write and column-length paths already use the payload charset.
Verification:
- `./mvnw -pl database/protocol/dialect/postgresql -DskipITs test` (356 tests)
- Focused tests: 8 tests
- Target-class JaCoCo CLASS/LINE/BRANCH coverage: 100%
- `./mvnw checkstyle:check -Pcheck -T1C`
- `./mvnw spotless:apply -Pcheck -T1C`
Documentation and release notes are not changed because this restores the existing payload-charset contract without changing configuration or public APIs.
--
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: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Use payload charset for PostgreSQL binary strings [shardingsphere]
Posted by "FlyingZC (via GitHub)" <gi...@apache.org>.
FlyingZC merged PR #39409:
URL: https://github.com/apache/shardingsphere/pull/39409
--
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: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Use payload charset for PostgreSQL binary strings [shardingsphere]
Posted by "iamhucong (via GitHub)" <gi...@apache.org>.
iamhucong commented on PR #39409:
URL: https://github.com/apache/shardingsphere/pull/39409#issuecomment-5252696779
Manual binary-bind verification was completed against head `0fae0cd105767035c54c6b797bc84902df6afbb2`.
**Environment**
- ShardingSphere-Proxy built from the current source
- Proxy JVM default charset forced to `ISO-8859-1`
- PostgreSQL client encoding set to UTF-8
- PostgreSQL 12 backend
- Raw PostgreSQL `Parse` / `Bind` messages with the parameter format explicitly set to binary
**Current implementation**
- `VARCHAR` (OID 1043), value `中文`: passed and round-tripped unchanged
- `UNSPECIFIED` (OID 0), value `中文`: passed and round-tripped unchanged
- `VARCHAR` (OID 1043), value `a`: passed
- `UNSPECIFIED` (OID 0), value `2020-08-23 15:57:03+08`: passed
**Controlled pre-fix comparison**
The two decoder classes from `apache/master` before this PR were compiled into a temporary classpath override and run with the same Proxy, backend, JVM charset, and wire-protocol client:
- Both multibyte cases produced mojibake because the UTF-8 payload bytes were decoded with the ISO-8859-1 JVM default charset.
- The original ASCII and timestamp cases still passed.
This before/after comparison confirms that the harness exercises both changed binary decoders, distinguishes the fix from the previous behavior, and preserves the original regression scenarios. All temporary processes and containers were removed afterward, and the working tree remained clean.
--
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: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Use payload charset for PostgreSQL binary strings [shardingsphere]
Posted by "iamhucong (via GitHub)" <gi...@apache.org>.
iamhucong commented on PR #39409:
URL: https://github.com/apache/shardingsphere/pull/39409#issuecomment-5250913285
### Code Correctness and Risk Review
**Review Result: Mergeable**
Overall risk is low. The change is limited to PostgreSQL binary bind string decoding and aligns the read path with the existing payload-charset contract.
Evidence:
- `PostgreSQLComBindPacket` delegates binary parameters to the corresponding `PostgreSQLBinaryProtocolValue.read()` implementation.
- The affected readers previously used the JVM default charset, which can differ from the packet payload charset.
- The updated behavior is consistent with the text bind, write, and column-length paths, which already use the payload charset.
- ASCII behavior is unchanged; the fix only corrects non-ASCII values when the charsets differ.
- No public API, SPI, configuration, state, or data structure changes are involved.
- Focused tests (8), module tests (356), Spotless, and Checkstyle passed. Both target classes have 100% CLASS/LINE/BRANCH coverage.
Reviewed head: `011735755abcdf15bd080b69fc8f0b85e94aeb89`.
Residual risk: an end-to-end Proxy test with a custom JVM `file.encoding` was not run. The unit tests directly exercise the mismatched-charset decoding boundary, so this residual risk is low. This is a code-scope review; CI was 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: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org