talmacschen-arch opened a new pull request, #1727:
URL: https://github.com/apache/cloudberry/pull/1727
<!-- ====================================================================
PR description draft for issue #1726 fix
==================================================================== -->
Fixes #1726
### What does this PR do?
Make the `gp_exttable_fdw` FDW accept both **symbolic encoding names**
(`UTF8`, `utf-8`, `GBK`, …) and numeric encoding IDs in the `encoding` OPTIONS
entry, mirroring what the legacy `CREATE EXTERNAL TABLE … ENCODING …` path has
always accepted, and what the rest of PostgreSQL accepts everywhere else
(`client_encoding`, `pg_dump`, `pg_conversion`, `CREATE DATABASE`, …).
Before this PR, both the FDW catalog reader
(`src/backend/access/external/external.c:280`) and the option validator
(`gpcontrib/gp_exttable_fdw/option.c:139,142`) parsed the value with `atoi()`.
`atoi("UTF8")` silently returns `0` (`SQL_ASCII`) and `PG_VALID_ENCODING(0)` is
true, so:
| OPTIONS spelling | Before | After |
| --------------------- | ------------------- | --------------------------- |
| `encoding '6'` | UTF8 ✅ | UTF8 ✅ (unchanged) |
| `encoding 'UTF8'` | SQL_ASCII (silent ❌) | UTF8 ✅ |
| `encoding 'utf-8'` | SQL_ASCII (silent ❌) | UTF8 ✅ |
| `encoding 'GBK'` | SQL_ASCII (silent ❌) | GBK ✅ |
| `encoding 'bogus'` | SQL_ASCII (silent ❌) | ERROR — unrecognized ✅ |
Affected scope is `gp_exttable_fdw` (used by `gp_exttable_server`). The
standalone `pxf_fdw` is unaffected — its validator routes `encoding` through
`ProcessCopyOptions`, which is already name-aware.
#### Implementation
A small shared helper `parse_fdw_encoding_option(const char *)` is added in
`src/backend/access/external/external.c` (declared in
`src/include/access/external.h`):
1. Try `pg_char_to_encoding(value)` — the same translation the legacy
`CREATE EXTERNAL TABLE` path uses.
2. Otherwise try a strict numeric form (`strtol` + end-of-string +
`PG_VALID_ENCODING` checks). `atoi` is intentionally avoided: `atoi("UTF8")`
silently mistranslates and that is the bug being fixed.
3. Otherwise `ereport(ERROR)`.
Both the FDW validator (`gp_exttable_permission_check`) and the read path
(`GetExtFromForeignTableOptions`) call this helper. No on-disk format change:
values are stored verbatim in `pg_foreign_table.ftoptions` exactly as the user
wrote them, and resolved at read time. This keeps `pg_dump` output
round-trip-safe and avoids silently rewriting catalog rows during upgrade.
The bypass on the runtime PXF error message ("`Define the external table
with ENCODING UTF8...`") lives in the separate `apache/cloudberry-pxf-server`
repository and will be addressed in a follow-up PR there.
### Type of Change
- [x] Bug fix (non-breaking change)
- [ ] New feature (non-breaking change)
- [x] **Behavior change on upgrade** — see "User-facing changes" below
- [ ] Documentation update
### Test Plan
Added cases to `gpcontrib/gp_exttable_fdw/input/gp_exttable_fdw.source`:
- `encoding '6'` → resolves to UTF8 (control)
- `encoding 'UTF8'` / `encoding 'utf-8'` → resolves to UTF8 (the fix)
- `encoding 'GBK'` → resolves to GBK
- `encoding 'bogus'` → ereports
- `ALTER FOREIGN TABLE … OPTIONS (SET encoding 'UTF8')` on a table that
was originally created with `encoding '0'` → reads as UTF8 after the ALTER
The pre-existing `encoding '-1'` ereport test was kept; only the error
message text was updated to the new `"\"-1\" is not a valid encoding name or
code"` form.
- [x] Unit tests added/updated (`gpcontrib/gp_exttable_fdw/`)
- [ ] Integration tests added/updated
- [ ] Passed `make installcheck` *(to be run by reviewer / CI)*
- [ ] Passed `make -C src/test installcheck-cbdb-parallel` *(to be run)*
### Impact
**User-facing changes (release-note worthy ⚠️):**
Foreign tables that were created **before** this fix with a non-numeric
`encoding` option (e.g. `encoding 'UTF8'`) had the literal name silently stored
in `pg_foreign_table.ftoptions`. With the old `atoi()` reader they took effect
as `SQL_ASCII`. After this fix, the same on-disk row is interpreted as the
named encoding (e.g. `UTF8`) — i.e. the encoding actually requested by the user.
For most affected users this is the desired correction (data was already
being silently mishandled). Operators who want to identify which existing
tables are about to flip semantics can run:
```sql
SELECT n.nspname, c.relname,
(SELECT split_part(opt, '=', 2)
FROM unnest(ftoptions) AS opt
WHERE opt LIKE 'encoding=%') AS stored_encoding
FROM pg_foreign_table ft
JOIN pg_class c ON c.oid = ft.ftrelid
JOIN pg_namespace n ON n.oid = c.relnamespace
WHERE ft.ftserver IN (SELECT oid FROM pg_foreign_server
WHERE srvname IN ('gp_exttable_server'))
AND EXISTS (
SELECT 1 FROM unnest(ftoptions) AS opt
WHERE opt LIKE 'encoding=%'
AND substring(opt FROM 'encoding=(.*)') !~ '^[0-9]+$'
);
```
If a table appears that should *not* flip, the operator can pin it by
re-issuing
`ALTER FOREIGN TABLE … OPTIONS (SET encoding '<numeric>')` before upgrade.
**Performance:** none. Two extra string compares + one syscache-free name
lookup on each foreign-table read; same for validation.
**Dependencies:** none. `pg_char_to_encoding()` is already declared in
`src/include/mb/pg_wchar.h:567` and used by the legacy path.
### Checklist
- [x] Followed [contribution
guide](https://cloudberry.apache.org/contribute/code)
- [ ] Added/updated documentation (release-note line; doc PR not required)
- [x] Reviewed code for security implications (no new external input
surfaces; helper ereports on invalid input)
- [ ] Requested review from [cloudberry
committers](https://github.com/orgs/apache/teams/cloudberry-committers)
### Additional Context
The design discussion in the issue (`#1726`) considered a write-time
normalization variant via `ProcessUtility_hook`, where the option value would
be rewritten to its numeric form *before* persisting into `ftoptions`. That
approach was abandoned because the hook is only installed by the extension's
`_PG_init`, which does not run until the first call into the extension's shared
library — and on the very first `CREATE FOREIGN TABLE` of a fresh backend, the
hook check has already been passed by the time the validator triggers `dlopen`.
Adopting it would require listing `gp_exttable_fdw` in
`session_preload_libraries`, which changes deployment expectations. The chosen
read-side resolution works without any preload requirement.
--
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]
