On Mon, Jan 12, 2026 at 1:01 PM Chao Li <li.evan.chao@gmail.com> wrote:
> Thanks for the patch. Here are a few review comments:
Thank you for the review!
> 1
> ```
> - * Allow 0, 1, "true", "false", "on", "off", a non-negative integer, or
> - * "match".
> + * Allow 0, 1, "true", "false", "on", "off", a non-negative integer (also
> + * as a string, to support file_fdw options), or "match".
> */
> ```
>
> From this comment, I cannot get how “0” and “1” will behave, and I cannot find a test case to show me that.
>
> With this patch, “2” acts the same as 2, so “1” acts the same as 1. Will “1” be a line count or a boolean true?
The header option ends up as an integer line count in
defGetCopyHeaderOption whether the value is quoted or not, so we don't
need to distinguish between them. But as you said, it is ambiguous, so
I updated the comment and added a test case.
> 2
> ```
> + /* Check if the header is a valid integer */
> + ival = pg_strtoint32_safe(header, (Node *)&escontext);
> + if (escontext.error_occurred)
> + ereport(ERROR,
> + (errcode(ERRCODE_SYNTAX_ERROR),
> + errmsg("%s requires a Boolean value, a non-negative integer, "
> + "or the string \"match\"",
> + def->defname)));
> ```
>
> I am thinking if INVALID_PARAMETER_VALUE would be better fit here for the error code.
This code path only fires for an unrecognized keyword. Per [0], that
falls under ERRCODE_SYNTAX_ERROR rather than INVALID_PARAMETER_VALUE,
which is for syntactically valid values that are out of range. So I
think the current error code is the right one to keep.
[0] https://www.postgresql.org/message-id/20250630162428.0efb43fb9cdaf2e203706011%40sraoss.co.jp
--
Best regards,
Shinya Kato
NTT OSS Center
