Re: file_fdw: Support multi-line HEADER option.

On Thu, Jan 15, 2026 at 8:45 PM Shinya Kato <shinya11(dot)kato(at)gmail(dot)com> wrote:
>
> On Mon, Jan 12, 2026 at 1:01 PM Chao Li <li(dot)evan(dot)chao(at)gmail(dot)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.

Thanks for updating the patch!

+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.

- 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.

+ 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)));

...
-------------------------------------

Regards,

--
Fujii Masao