my-ship-it commented on issue #1726:
URL: https://github.com/apache/cloudberry/issues/1726#issuecomment-4427834027
Thanks for the unusually thorough write-up — the root cause, the reproducer,
the causal chain (`atoi("UTF8") → 0 → PG_SQL_ASCII`), and the contrast with the
legacy `CREATE EXTERNAL TABLE` path all line up perfectly with what I see in
the current `main`. Both bug sites reproduce as described:
- `src/backend/access/external/external.c:280` (read path)
- `gpcontrib/gp_exttable_fdw/option.c:139` (validator)
And the "Define the external table with ENCODING UTF8" downstream symptom on
the PXF binary path makes total sense once you trace it back here — that error
message is going to feel pretty surreal to anyone whose DDL literally says
`ENCODING UTF8`. 🙃
I also really appreciate the **Update (2026-05-09)** section explaining why
the `ProcessUtility_hook` approach was abandoned. The load-order reasoning is
correct: `_PG_init` only fires on the first `dlopen`, which is triggered by the
validator inside `transformGenericOptions`, and by then `ProcessUtility`'s hook
check for the current statement has already gone past. Requiring
`session_preload_libraries` (the `pg_hint_plan` route) would fix it, but that's
a real change to the deployment story for `gp_exttable_fdw`, so I agree the
read-side approach is the better trade-off here.
The decision to **not** rewrite on-disk values and to surface the behavior
change in release notes + a detection query is the right call — existing
misconfigured rows getting "fixed" on upgrade is what the vast majority of
operators actually want, and the detection query gives anyone who needs to pin
the old broken semantics a clean escape hatch.
A couple of small suggestions for the PR — feel free to take or leave:
1. **Strict numeric parsing in the shared helper.** Since `atoi` is
precisely what got us here, it'd be nice if `parse_fdw_encoding_option()` used
`strtol` with an `*endptr == '\0'` (and non-empty input) check, so something
like `encoding '6abc'` is rejected rather than silently accepted as `6`.
2. **Single source of truth.** It'd be great if both `external.c:280` and
`option.c:139-142` genuinely call through the same helper, so the validator and
the read path can't drift apart later. (Sounds like that's already the plan —
just calling it out.)
3. **Test coverage.** A small regression test covering the matrix from your
table (`'6'`, `'UTF8'`, `'utf-8'`, `'GBK'`, `'bogus'`) would lock the behavior
in nicely.
Looking forward to the PR — thanks again for the careful analysis and for
picking the safer of the two implementation shapes. 🙏
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
