file_fdw: Support multi-line HEADER option.

On Fri, Jan 9, 2026 at 5:15 PM Shinya Kato <shinya11.kato@gmail.com> wrote:

Hi hackers,
(CC: Fujii-san, committer of bc2f348e8)

Commit bc2f348e8[0] introduced multi-line HEADER support for COPY.
However, file_fdw does not yet support it, so I have implemented it in
the attached patch.

Since foreign table options in CREATE/ALTER FOREIGN TABLE are
specified as single-quoted strings, I updated defGetCopyHeaderOption()
to handle string values as well.

Thoughts?

+1

Could you add this patch to the next Commitfest? It would be better to
commit it in v19 so that multi-line header support is delivered for
both COPY and file_fdw at the same time.

Regards,

--
Fujii Masao

Hi Shinya Kato

I tested the patch and have no functional questions. I have a small question: Is it necessary to add "(also as a string, to support file_fdw options)" to the final `ereport` error message in `defCheckCopyHeaderString`? Like in one of your comments below.

Thank you.

songjinzhou
tsinghualucky912@foxmail.com

On Fri, Jan 9, 2026 at 7:22 PM Fujii Masao <masao.fujii@gmail.com> wrote:

Thoughts?

+1

Thank you for the review!

Could you add this patch to the next Commitfest? It would be better to
commit it in v19 so that multi-line header support is delivered for
both COPY and file_fdw at the same time.

Yeah, this feature is targeted for v19. I've added it to the next Commitfest.
https://commitfest.postgresql.org/patch/6383/

--
Best regards,
Shinya Kato
NTT OSS Center

On Fri, 09 Jan 2026 at 21:57, Shinya Kato <shinya11.kato@gmail.com> wrote:

On Fri, Jan 9, 2026 at 7:22 PM Fujii Masao <masao.fujii@gmail.com> wrote:

Thoughts?

+1

Thank you for the review!

Could you add this patch to the next Commitfest? It would be better to
commit it in v19 so that multi-line header support is delivered for
both COPY and file_fdw at the same time.

Yeah, this feature is targeted for v19. I've added it to the next Commitfest.
https://commitfest.postgresql.org/patch/6383/

A minor nitpick:

+  or a numeric line count for <literal>HEADER</literal>) to enable the desired
+  behavior.

s/numeric/non-negative integer/

More precise: excludes negative values & clarifies it's an integer.

--
Regards,
Japin Li
ChengDu WenWu Information Technology Co., Ltd.

On Jan 9, 2026, at 16:14, Shinya Kato <shinya11.kato@gmail.com> wrote:

Hi hackers,
(CC: Fujii-san, committer of bc2f348e8)

Commit bc2f348e8[0] introduced multi-line HEADER support for COPY.
However, file_fdw does not yet support it, so I have implemented it in
the attached patch.

Since foreign table options in CREATE/ALTER FOREIGN TABLE are
specified as single-quoted strings, I updated defGetCopyHeaderOption()
to handle string values as well.

Thoughts?

[0] https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=bc2f348e87c02de63647dbe290d64ff088880dbe

--
Best regards,
Shinya Kato
NTT OSS Center
<v1-0001-file_fdw-Support-multi-line-HEADER-option.patch>

Hi Shinya,

Thanks for the patch. Here are a few review comments:

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?

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.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

On Fri, Jan 9, 2026 at 9:51 PM songjinzhou <tsinghualucky912@foxmail.com> wrote:

I tested the patch and have no functional questions. I have a small question: Is it necessary to add "(also as a string, to support file_fdw options)" to the final `ereport` error message in `defCheckCopyHeaderString`? Like in one of your comments below.

Thank you for reviewing my patch and the comment!

I don’t think we should add “(also as a string, to support file_fdw
options)” to the ereport message because PostgreSQL error messages
typically describe the semantic type rather than the input format.

For instance, REJECT_LIMIT accepts a string in file_fdw but the error
message simply asks for a numeric value.

--
Best regards,
Shinya Kato
NTT OSS Center

On Sat, Jan 10, 2026 at 1:14 AM Japin Li <japinli@hotmail.com> wrote:

A minor nitpick:

+  or a numeric line count for <literal>HEADER</literal>) to enable the desired
+  behavior.

s/numeric/non-negative integer/

More precise: excludes negative values & clarifies it's an integer.

Thank you for the review! Agreed, and the revised patch is attached.

--
Best regards,
Shinya Kato
NTT OSS Center

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]/messages/by-id/20250630162428.0efb43fb9cdaf2e203706011@sraoss.co.jp, 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]: /messages/by-id/20250630162428.0efb43fb9cdaf2e203706011@sraoss.co.jp

--
Best regards,
Shinya Kato
NTT OSS Center

On Thu, Jan 15, 2026 at 8:45 PM Shinya Kato <shinya11.kato@gmail.com> wrote:

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.

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

On Mon, Jan 19, 2026 at 8:22 PM Fujii Masao <masao.fujii@gmail.com> 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

On Jan 15, 2026, at 19:44, Shinya Kato <shinya11.kato@gmail.com> wrote:

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.

I am sorry maybe I didn’t express myself clear. But in v4, this problem is clearer:

1 - 0001
```
 	/*
-	 * Allow 0, 1, "true", "false", "on", "off", a non-negative integer, or
-	 * "match".
+	 * Allow 0, 1, "true", "false", "on", "off", an integer greater than or
+	 * equal to zero, or "match".
 	 */
```

Here, “0, 1” is a duplicate of “an integer greater than or equal to zero”, so the commend can be simplified as:

```
Allow “true”, “false”, “on”, “off”, an integer greater than or equal to zero, or ...
```

And one more comment for 0002:

2 - 0002

Looking at the two error branches:

```
+				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, an integer greater than or "
+										"equal to zero, or the string \"match\"",
+										def->defname)));
+				}
```
and
```
+	if (ival < 0)
+		ereport(ERROR,
+				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+				 errmsg("a negative integer value cannot be "
+						"specified for %s", def->defname)));
```

For the "ival<0" case, I think we can use the same error message as the first one, because the error message “an integer greater than or equal to zero” has covered the error of “ival<0”. It would be better to generate less different error messages.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

On Tue, Jan 20, 2026 at 1:17 PM Shinya Kato <shinya11.kato@gmail.com> wrote:

Thank you! I was struggling with how to write it cleanly, but the code
you suggested is clear and easy to understand.

Thanks for updating the patch!

I've applied the cosmetic changes and attached updated versions of the patches.

Regarding 0001 patch:

-                        errmsg("%s requires a Boolean value, a
non-negative integer, "
-                                       "or the string \"match\"",
+                        errmsg("%s requires a Boolean value, an
integer greater than or "
+                                       "equal to zero, or the string
\"match\"",

I added the word "value" after "integer" in the error message.

Regarding 0002 patch:

         * Allow 0, 1, "true", "false", "on", "off", an integer greater than or
-        * equal to zero, or "match".
+        * equal to zero (also as a string, to support file_fdw options), or
+        * "match".

I updated the comment to more clearly list the values handled by
defGetCopyHeaderOption(). With the latest patch, the comment is:

Allow an integer value greater than or equal to zero (integers
specified as strings are also accepted, mainly for file_fdw foreign
table options), or "true", "false", "on", "off", or "match".

I'm thinking to commit the attached patches.

Regards,

--
Fujii Masao

On Tue, Jan 20, 2026 at 2:54 PM Chao Li <li.evan.chao@gmail.com> wrote:

I am sorry maybe I didn’t express myself clear. But in v4, this problem is clearer:

1 - 0001
```
/*
-        * Allow 0, 1, "true", "false", "on", "off", a non-negative integer, or
-        * "match".
+        * Allow 0, 1, "true", "false", "on", "off", an integer greater than or
+        * equal to zero, or "match".
*/
```

Here, “0, 1” is a duplicate of “an integer greater than or equal to zero”, so the commend can be simplified as:

Yeah, the latest patch I attached upthread updates this comment and
removes "0, 1" from it.

Regards,

--
Fujii Masao

On 2026-Jan-20, Fujii Masao wrote:

I'm thinking to commit the attached patches.

In 0001 I would recommend to move the string "match" to outside the
message. Otherwise a translator might be tempted to translate it, and
then it would make no sense. (I have never seen this happen in
Postgres, but I have seen this kind of translation mistake in other
software, and it is infuriating.)

--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"Los minutos y los segundos son mercadería de la ciudad, donde un infeliz se
afana por no perder ni siquiera un segundo y no advierto que obrando de ese
modo pierde una vida." ("La vuelta de Don Camilo", G. Guareschi)

On Wed, Jan 21, 2026 at 12:07 AM Álvaro Herrera <alvherre@kurilemu.de> wrote:

On 2026-Jan-20, Fujii Masao wrote:

I'm thinking to commit the attached patches.

In 0001 I would recommend to move the string "match" to outside the
message. Otherwise a translator might be tempted to translate it, and
then it would make no sense. (I have never seen this happen in
Postgres, but I have seen this kind of translation mistake in other
software, and it is infuriating.)

Thanks for the review!

Agreed. I've updated the patches as suggested.
The revised versions are attached.

Regards,

--
Fujii Masao

From: Fujii Masao <masao.fujii@gmail.com>
Sent: Wednesday, January 21, 2026 15:38
To: Álvaro Herrera <alvherre@kurilemu.de>
Cc: Shinya Kato <shinya11.kato@gmail.com>; Chao Li <li.evan.chao@gmail.com>; PostgreSQL-development <pgsql-hackers@postgresql.org>
Subject: Re: file_fdw: Support multi-line HEADER option.

On Wed, Jan 21, 2026 at 12:07 AM Álvaro Herrera <alvherre@kurilemu.de> wrote:

On 2026-Jan-20, Fujii Masao wrote:

I'm thinking to commit the attached patches.

In 0001 I would recommend to move the string "match" to outside the
message.  Otherwise a translator might be tempted to translate it, and
then it would make no sense.  (I have never seen this happen in
Postgres, but I have seen this kind of translation mistake in other
software, and it is infuriating.)

Thanks for the review!

Agreed. I've updated the patches as suggested.
The revised versions are attached.

Regards,

--
Fujii Masao

________________________________________

Hi Fujii-san,

I've noticed an inconsistent naming convention in the patch:
we use a capitalized Boolean for the boolean type, but a lowercase integer when referring to the value type.

+ errmsg("%s requires a Boolean value, an integer "
+		"value greater than or equal to zero

Is there any reason why we follow this mixed style?

Another nitpick in v6-0001 patch:
tow whitespaces are between "special" and "value"
+ second %s is the special value "match" for that option */

Thanks,
Steven

On Wed, Jan 21, 2026 at 5:18 PM Steven Niu <niushiji@gmail.com> wrote:

I've noticed an inconsistent naming convention in the patch:
we use a capitalized Boolean for the boolean type, but a lowercase integer when referring to the value type.

+ errmsg("%s requires a Boolean value, an integer "
+               "value greater than or equal to zero

Is there any reason why we follow this mixed style?

I don't have a strong opinion on this. "Boolean value" has been used in command
error messages from before, and I couldn't find any messages that use the term
"Integer". So I committed the patch with the original wording. If this turns out
to be an issue, we can revisit it later.

Another nitpick in v6-0001 patch:
tow whitespaces are between "special" and "value"
+ second %s is the special value "match" for that option */

Thanks for catching that! I've fixed it.

I've pushed the patches. Thanks!

Regards,

--
Fujii Masao

Fujii Masao <masao.fujii@gmail.com> writes:

On Wed, Jan 21, 2026 at 5:18 PM Steven Niu <niushiji@gmail.com> wrote:

I've noticed an inconsistent naming convention in the patch:
we use a capitalized Boolean for the boolean type, but a lowercase integer when referring to the value type.
Is there any reason why we follow this mixed style?

I don't have a strong opinion on this. "Boolean value" has been used in command
error messages from before, and I couldn't find any messages that use the term
"Integer". So I committed the patch with the original wording.

For context, the reason that "Boolean" is often capitalized is
that the term refers to an identifiable person, George Boole [1]https://en.wikipedia.org/wiki/George_Boole.
You don't see "Newtonian" or "Einsteinian" without caps either.
So this message is not wrong in isolation. But I don't think that
we have a consistent project style about whether to capitalize
"Boolean" or not. It's not a correct spelling of the SQL type name,
for sure, but it's commonplace when being used as an adjective.
So does staying consistent with the lower-case type name justify
spelling "Boolean algebra" without caps? I dunno.

If anyone wants to investigate this some more and recommend that
we adopt a project style rule (and create a patch to fix the
stragglers), have at it.

regards, tom lane

[1]: https://en.wikipedia.org/wiki/George_Boole