On Mon, Jan 19, 2026 at 8:22 PM Fujii Masao <[email protected]> wrote: > +CREATE FOREIGN TABLE tbl () SERVER file_server OPTIONS (header > '2.5'); -- ERROR > +ERROR: header requires a Boolean value, a non-negative integer, or > the string "match" > > This isn't caused by the patch, but I think the error message itself should > be improved in a separate patch before adding multi-line header support > to file_fdw. Per the Error Message Style Guide, "non-negative" should > be avoided because it's ambiguous about whether zero is accepted.
I didn't know such a style guide, thanks. I've fixed it in the v4-0001 patch.
> - syntax requires a value to be present in all cases. To activate
> - <command>COPY</command> options typically written without a value,
> you can pass
> - the value TRUE, since all such options are Booleans.
> + syntax requires a value to be present in all cases. Use the appropriate
> + explicit value (for example <literal>TRUE</literal>,
> <literal>match</literal>,
> + or a non-negative integer line count for <literal>HEADER</literal>) to
> enable
> + the desired behavior.
>
> Using "non-negative" here has the same ambiguity.
>
> I'm not sure if this change is an improvement. Isn't the original wording
> almost OK? Of course, "all such options are Booleans" is not true and
> probably should be something like "all such options accpet Boolean values",
> though.
I've fixed it as suggested.
> + if (IsA(def->arg, Integer))
> + return defCheckCopyHeaderInteger(def, intVal(def->arg), is_from);
> + else
> + return defCheckCopyHeaderString(def, defGetString(def), is_from);
>
> Personally, I think it would be simpler and easier to understand to keep
> the logic in a single function defGetCopyHeaderOption(), rather than
> splitting it across three functions. Thought?
>
> For example,
> -------------------------------------
> switch (nodeTag(def->arg))
> {
> case T_Integer:
> ival = intVal(def->arg);
> break;
> default:
> {
> char *sval = defGetString(def);
>
> ...
>
> else
> {
> ErrorSaveContext escontext = {T_ErrorSaveContext};
>
> /* Check if the header is a valid integer */
> ival = pg_strtoint32_safe(sval, (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)));
> }
> }
> break;
> }
>
> if (ival < 0)
> ereport(ERROR,
> (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> errmsg("a negative integer value cannot be "
> "specified for %s", def->defname)));
>
> ...
> -------------------------------------
Thank you! I was struggling with how to write it cleanly, but the code
you suggested is clear and easy to understand.
--
Best regards,
Shinya Kato
NTT OSS Center
v4-0001-Replace-non-negative-with-greater-than-or-equal-t.patch
Description: Binary data
v4-0002-file_fdw-Support-multi-line-HEADER-option.patch
Description: Binary data
