Обсуждение: file_fdw: Support multi-line HEADER option.
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
Вложения
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 yourcomments 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 yourcomments 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], 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
Вложения
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
integergreater than or equal to zero” has covered the error of “ival<0”. It would be better to generate less different
errormessages.
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].
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