talmacschen-arch commented on issue #1726:
URL: https://github.com/apache/cloudberry/issues/1726#issuecomment-4440008267
@my-ship-it — quick progress update on your three suggestions; all three are
now in PR #1727.
**1. Strict numeric parsing in the shared helper** — ✅ already in the
original fix commit `f5db4e1`. `parse_fdw_encoding_option()` uses `strtol()`
with `endptr != value && *endptr == '\0' && errno == 0 && n >= 0 &&
PG_VALID_ENCODING(n)`. `atoi()` is intentionally avoided, and the function
comment calls out exactly why (it's the bug being fixed). See
`src/backend/access/external/external.c:50-76`.
**2. Single source of truth** — ✅ also in `f5db4e1`. Both call sites
genuinely go through the helper:
- validator → `gpcontrib/gp_exttable_fdw/option.c:144`
- read path → `src/backend/access/external/external.c:321` (from
`GetExtFromForeignTableOptions`)
So the validator and the read path cannot drift apart later.
**3. Test coverage** — the matrix you listed (`'6'` / `'UTF8'` / `'utf-8'` /
`'GBK'` / `'bogus'`) was already covered in
`gpcontrib/gp_exttable_fdw/{input,output}/gp_exttable_fdw.source` from the
original commit (plus an `ALTER FOREIGN TABLE … OPTIONS (SET encoding 'UTF8')`
round-trip on a row that was created with `encoding '0'`). I just pushed
`4a35397` to lock down the strictness of the numeric path specifically with one
more case: **`encoding '6abc'`** — i.e. the mixed numeric+letters input your
suggestion (1) is designed to reject (under the old `atoi()` it would have
silently become `6` = UTF8, which is exactly the family of
silent-mistranslation bug we're trying to make impossible).
One small disclosure: I don't have a Cloudberry dev cluster on my current
workstation, so I haven't been able to run `make installcheck` locally for the
new `'6abc'` case. The expected error text is byte-for-byte derived from the
existing `'bogus'` case (same `ereport` site in `parse_fdw_encoding_option()`,
same coordinator-side validator path, no segment suffix), so it should match —
but CI will be the authoritative check.
Would appreciate another pass on #1727 when you have a moment. Thanks again
for the careful review.
--
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]
