Extend COPY FROM with HEADER <integer> to skip multiple lines

On 2025/06/09 16:10, Shinya Kato wrote:

Hi hackers,

I'd like to propose a new feature for the COPY FROM command to allow
skipping multiple header lines when loading data. This enhancement
would enable files with multi-line headers to be loaded without any
preprocessing, which would significantly improve usability.

In real-world scenarios, it's common for data files to contain
multiple header lines, such as file descriptions or column
explanations. Currently, the COPY command cannot load these files
directly, which requires users to preprocess them with tools like sed
or tail.

Although you can use "COPY t FROM PROGRAM 'tail -n +3 /path/to/file'",
some environments do not have the tail command available.
Additionally, this approach requires superuser privileges or
membership in the pg_execute_server_program role.

This feature also has precedent in other major RDBMS:
- MySQL: LOAD DATA ... IGNORE N LINES [1]
- SQL Server: BULK INSERT … WITH (FIRST ROW=N) [2]
- Oracle SQL*Loader: sqlldr … SKIP=N [3]

I have not yet created a patch, but I am willing to implement an
extension for the HEADER option. I would like to discuss the
specification first.

The specification I have in mind is as follows:
- Command: COPY FROM
- Formats: text and csv
- Option syntax: HEADER [ boolean | integer | MATCH] (Extend the
HEADER option to accept an integer value in addition to the existing
boolean and MATCH keywords.)
- Behavior: Let N be the specified integer.
- If N < 0, raise an error.
- If N = 0 or 1, same behavior when boolean is specified.
- If N > 1, skip the first N rows.

Thoughts?

I generally like the idea.

However, a similar proposal was made earlier [1]/messages/by-id/CALAY4q8nGSXp0P5uf56vn-mD7reWqZP5k6PS1CGUm26X4FsYJA@mail.gmail.com, and seemingly
some hackers weren't in favor of it. It's probably worth reading
that thread to understand the previous concerns.

Regards,

[1]: /messages/by-id/CALAY4q8nGSXp0P5uf56vn-mD7reWqZP5k6PS1CGUm26X4FsYJA@mail.gmail.com

--
Fujii Masao
NTT DATA Japan Corporation

2025年6月9日(月) 17:27 Fujii Masao <masao.fujii@oss.nttdata.com>:

I generally like the idea.

Thanks.

However, a similar proposal was made earlier [1], and seemingly
some hackers weren't in favor of it. It's probably worth reading
that thread to understand the previous concerns.

[1] /messages/by-id/CALAY4q8nGSXp0P5uf56vn-mD7reWqZP5k6PS1CGUm26X4FsYJA@mail.gmail.com

Oh, I missed it. I will check it soon.

--
Best regards,
Shinya Kato
NTT OSS Center

On 2025-06-09 Mo 4:27 AM, Fujii Masao wrote:

On 2025/06/09 16:10, Shinya Kato wrote:

Hi hackers,

I'd like to propose a new feature for the COPY FROM command to allow
skipping multiple header lines when loading data. This enhancement
would enable files with multi-line headers to be loaded without any
preprocessing, which would significantly improve usability.

In real-world scenarios, it's common for data files to contain
multiple header lines, such as file descriptions or column
explanations. Currently, the COPY command cannot load these files
directly, which requires users to preprocess them with tools like sed
or tail.

Although you can use "COPY t FROM PROGRAM 'tail -n +3 /path/to/file'",
some environments do not have the tail command available.
Additionally, this approach requires superuser privileges or
membership in the pg_execute_server_program role.

This feature also has precedent in other major RDBMS:
- MySQL: LOAD DATA ... IGNORE N LINES [1]
- SQL Server: BULK INSERT … WITH (FIRST ROW=N) [2]
- Oracle SQL*Loader: sqlldr … SKIP=N [3]

I have not yet created a patch, but I am willing to implement an
extension for the HEADER option. I would like to discuss the
specification first.

The specification I have in mind is as follows:
- Command: COPY FROM
- Formats: text and csv
- Option syntax: HEADER [ boolean | integer | MATCH] (Extend the
HEADER option to accept an integer value in addition to the existing
boolean and MATCH keywords.)
- Behavior: Let N be the specified integer.
   - If N < 0, raise an error.
   - If N = 0 or 1, same behavior when boolean is specified.
   - If N > 1, skip the first N rows.

Thoughts?

I generally like the idea.

However, a similar proposal was made earlier [1], and seemingly
some hackers weren't in favor of it. It's probably worth reading
that thread to understand the previous concerns.

Regards,

[1]
/messages/by-id/CALAY4q8nGSXp0P5uf56vn-mD7reWqZP5k6PS1CGUm26X4FsYJA@mail.gmail.com

I think the earlier proposal went rather further than this one, which I
suspect can be implemented fairly cheaply.

I don't have terribly strong feelings about it, but matching a feature
implemented elsewhere has some attraction if it can be done easily.

OTOH I'm a bit curious to know what software produces multi-line CSV
headers.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

However, a similar proposal was made earlier [1], and seemingly
some hackers weren't in favor of it. It's probably worth reading
that thread to understand the previous concerns.

[1] /messages/by-id/CALAY4q8nGSXp0P5uf56vn-mD7reWqZP5k6PS1CGUm26X4FsYJA@mail.gmail.com

Oh, I missed it. I will check it soon.

I read it.

There are clear differences from the earlier proposal. My sole
motivation is to skip multiple headers, and I don't believe a feature
for skipping footers is necessary. To be clear, I think it's best to
simply extend the current HEADER option.

Regarding the concern about adding ETL-like functionality, this
feature is already implemented in other RDBMSs, which is why I believe
it is also necessary for PostgreSQL.

Honestly, I haven't implemented it yet, so I'm not sure about the
performance. However, I don't expect it to be significantly different
from the current HEADER option that skips a single line.

I think the earlier proposal went rather further than this one, which I
suspect can be implemented fairly cheaply.

That's probably it, exactly.

I don't have terribly strong feelings about it, but matching a feature
implemented elsewhere has some attraction if it can be done easily.

OTOH I'm a bit curious to know what software produces multi-line CSV
headers.

Both Pandas and R can create CSV files with multi-line headers
(although I don't personally think this is desirable). Furthermore,
various systems sometimes generate reports as CSV files that
unexpectedly contain multiple header lines.

--
Best regards,
Shinya Kato
NTT OSS Center

On 2025/06/10 9:43, Shinya Kato wrote:

However, a similar proposal was made earlier [1], and seemingly
some hackers weren't in favor of it. It's probably worth reading
that thread to understand the previous concerns.

[1] /messages/by-id/CALAY4q8nGSXp0P5uf56vn-mD7reWqZP5k6PS1CGUm26X4FsYJA@mail.gmail.com

Oh, I missed it. I will check it soon.

I read it.

There are clear differences from the earlier proposal. My sole
motivation is to skip multiple headers, and I don't believe a feature
for skipping footers is necessary. To be clear, I think it's best to
simply extend the current HEADER option.

Sounds ok to me.

Regarding the concern about adding ETL-like functionality, this
feature is already implemented in other RDBMSs, which is why I believe
it is also necessary for PostgreSQL.

Honestly, I haven't implemented it yet, so I'm not sure about the
performance. However, I don't expect it to be significantly different
from the current HEADER option that skips a single line.

So it seems better for you to implement the patch at first and then
check the performance overhead etc if necessary.

Regards,

--
Fujii Masao
NTT DATA Japan Corporation

On Tue, Jun 10, 2025 at 2:34 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:

There are clear differences from the earlier proposal. My sole
motivation is to skip multiple headers, and I don't believe a feature
for skipping footers is necessary. To be clear, I think it's best to
simply extend the current HEADER option.

Sounds ok to me.

Thank you.

Regarding the concern about adding ETL-like functionality, this
feature is already implemented in other RDBMSs, which is why I believe
it is also necessary for PostgreSQL.

Honestly, I haven't implemented it yet, so I'm not sure about the
performance. However, I don't expect it to be significantly different
from the current HEADER option that skips a single line.

So it seems better for you to implement the patch at first and then
check the performance overhead etc if necessary.

Thank you for your advice. I will create a patch.

--
Best regards,
Shinya Kato
NTT OSS Center

On Tue, Jun 10, 2025 at 2:34 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:

On 2025/06/10 9:43, Shinya Kato wrote:

However, a similar proposal was made earlier [1], and seemingly
some hackers weren't in favor of it. It's probably worth reading
that thread to understand the previous concerns.

[1] /messages/by-id/CALAY4q8nGSXp0P5uf56vn-mD7reWqZP5k6PS1CGUm26X4FsYJA@mail.gmail.com

Oh, I missed it. I will check it soon.

I read it.

There are clear differences from the earlier proposal. My sole
motivation is to skip multiple headers, and I don't believe a feature
for skipping footers is necessary. To be clear, I think it's best to
simply extend the current HEADER option.

Sounds ok to me.

Regarding the concern about adding ETL-like functionality, this
feature is already implemented in other RDBMSs, which is why I believe
it is also necessary for PostgreSQL.

Honestly, I haven't implemented it yet, so I'm not sure about the
performance. However, I don't expect it to be significantly different
from the current HEADER option that skips a single line.

So it seems better for you to implement the patch at first and then
check the performance overhead etc if necessary.

Regards,

--
Fujii Masao
NTT DATA Japan Corporation

--
Best regards,
Shinya Kato
NTT OSS Center

Andrew Dunstan <andrew@dunslane.net> writes:

OTOH I'm a bit curious to know what software produces multi-line CSV
headers.

AWS CloudFront access logs are stored in S3 as TSV files (one per hour
per CF node) with a two-line header comment where the first line is the
version and the second lists the fields (but not in a form useful for
HEADER MATCH).

Although not useful for the above format, and not intended to derail or
bloat the proposal in this thread, would it be useful to have a mode
that combines skip and match? I.e. skip N lines, then check the fields
in the one after that against the target columns.

- ilmari

On Tue, Jun 10, 2025 at 7:05 PM Dagfinn Ilmari Mannsåker <ilmari@ilmari.org>
wrote:

Andrew Dunstan <andrew@dunslane.net> writes:

OTOH I'm a bit curious to know what software produces multi-line CSV
headers.

AWS CloudFront access logs are stored in S3 as TSV files (one per hour
per CF node) with a two-line header comment where the first line is the
version and the second lists the fields (but not in a form useful for
HEADER MATCH).

Thank you for providing that example.

Although not useful for the above format, and not intended to derail or
bloat the proposal in this thread, would it be useful to have a mode
that combines skip and match? I.e. skip N lines, then check the fields
in the one after that against the target columns.

I think it would be useful, but the target columns are not always at the
bottom of the header. For example, the target columns could be on the first
line, with explanations or sub-columns on the lines that follow.

Considering this, the patch would become too complicated, so I'd like to
keep this out of scope. What do you think?

--
Best regards,
Shinya Kato
NTT OSS Center

Shinya Kato <shinya11.kato@gmail.com> writes:

On Tue, Jun 10, 2025 at 7:05 PM Dagfinn Ilmari Mannsåker <ilmari@ilmari.org>
wrote:

Although not useful for the above format, and not intended to derail or
bloat the proposal in this thread, would it be useful to have a mode
that combines skip and match? I.e. skip N lines, then check the fields
in the one after that against the target columns.

I think it would be useful, but the target columns are not always at the
bottom of the header. For example, the target columns could be on the first
line, with explanations or sub-columns on the lines that follow.

Considering this, the patch would become too complicated, so I'd like to
keep this out of scope. What do you think?

Yeah, that's a valid point, and as I said I don't want to bloat or
derail this proposal, so keeping it simple is fine.

- ilmari

So it seems better for you to implement the patch at first and then
check the performance overhead etc if necessary.

Thank you for your advice. I will create a patch.

I created a patch.

As you can see from the patch, I believe the performance impact is
negligible. The only changes were to modify how the HEADER option is stored
and to add a loop that skips the specified number of header lines when
parsing the options.

The design is such that if a HEADER value larger than the number of lines
in the file is specified, the command will complete with zero rows loaded
and will not return an error. This is the same behavior as specifying
HEADER true for a CSV file that has zero rows.

And I will add this patch for the next CF.

Thoughts?

--
Best regards,
Shinya Kato
NTT OSS Center

On 2025/06/26 14:35, Shinya Kato wrote:

So it seems better for you to implement the patch at first and then
check the performance overhead etc if necessary.

Thank you for your advice. I will create a patch.

I created a patch.

Thanks for the patch!

As you can see from the patch, I believe the performance impact is negligible. The only changes were to modify how the HEADER option is stored and to add a loop that skips the specified number of header lines when parsing the options.

The design is such that if a HEADER value larger than the number of lines in the file is specified, the command will complete with zero rows loaded and will not return an error. This is the same behavior as specifying HEADER true for a CSV file that has zero rows.

Sounds good to me.

And I will add this patch for the next CF.

Thoughts?

When I compiled with the patch applied, I got the following warning:

copyfromparse.c:834:20: error: ‘done’ may be used uninitialized [-Werror=maybe-uninitialized]
834 | if (done)
| ^

+      lines are discarded.  An integer value greater than 1 is only valid for
+      <command>COPY FROM</command> commands.

This might be slightly misleading since 0 is also a valid value for COPY FROM.

+		for (int i = 0; i < cstate->opts.header.skip_lines; i++)
+		{
+			cstate->cur_lineno++;
+			done = CopyReadLine(cstate, is_csv);
+		}

If "done" is true, shouldn't we break out of the loop immediately?
Otherwise, with a large HEADER value, we could end up wasting a lot of
cycles unnecessarily.

+defGetCopyHeaderOption(DefElem *def, bool is_from)
  {
+	CopyHeaderOption option;

To avoid repeating the same code like "option.match = false" in every case,
how about initializing the struct like "option = {false, 1}"?

+							(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+							 errmsg("HEADER must be non-negative integer")));

This message could be confusing since HEADER can also accept a Boolean or "match".
Maybe it's better to use the same message as "%s requires a Boolean value, integer value,
or \"match\"",? "integer value"? If so, it's also better to use "non-negative integer"
instead of just "integer".

Regards,

--
Fujii Masao
NTT DATA Japan Corporation

On Thu, Jun 26, 2025 at 4:36 PM Fujii Masao <masao.fujii@oss.nttdata.com>
wrote:

On 2025/06/26 14:35, Shinya Kato wrote:

So it seems better for you to implement the patch at first and then
check the performance overhead etc if necessary.

Thank you for your advice. I will create a patch.

I created a patch.

Thanks for the patch!

Thank you for your review.

When I compiled with the patch applied, I got the following warning:

copyfromparse.c:834:20: error: ‘done’ may be used uninitialized
[-Werror=maybe-uninitialized]
834 | if (done)
| ^

Oh, sorry. I missed it.
I fixed it to initialize done = false.

+      lines are discarded.  An integer value greater than 1 is only valid
for
+      <command>COPY FROM</command> commands.

This might be slightly misleading since 0 is also a valid value for COPY
FROM.

 That's certainly true. I fixed it below:
+      lines are discarded.  An integer value greater than 1 is not allowed
for
+      <command>COPY TO</command> commands.

+               for (int i = 0; i < cstate->opts.header.skip_lines; i++)
+               {
+                       cstate->cur_lineno++;
+                       done = CopyReadLine(cstate, is_csv);
+               }

If "done" is true, shouldn't we break out of the loop immediately?
Otherwise, with a large HEADER value, we could end up wasting a lot of
cycles unnecessarily.

Exactly, fixed.

+defGetCopyHeaderOption(DefElem *def, bool is_from)
{
+       CopyHeaderOption option;

To avoid repeating the same code like "option.match = false" in every case,
how about initializing the struct like "option = {false, 1}"?

Exactly, fixed.

+
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+                                                        errmsg("HEADER
must be non-negative integer")));

This message could be confusing since HEADER can also accept a Boolean or
"match".
Maybe it's better to use the same message as "%s requires a Boolean value,
integer value,
or \"match\"",? "integer value"? If so, it's also better to use
"non-negative integer"
instead of just "integer".

Yes, I fixed it to use the same message which replaced "integer" to
"non-negative integer".

Original error message "%s requires a Boolean value, integer value, or
\"match\"" should also be fixed to "non-negative integer"?

--
Best regards,
Shinya Kato
NTT OSS Center

On 2025/06/26 19:12, Shinya Kato wrote:

On Thu, Jun 26, 2025 at 4:36 PM Fujii Masao <masao.fujii@oss.nttdata.com <mailto:masao.fujii@oss.nttdata.com>> wrote:

On 2025/06/26 14:35, Shinya Kato wrote:

  > > So it seems better for you to implement the patch at first and then
  > > check the performance overhead etc if necessary.
  >
  > Thank you for your advice. I will create a patch.

I created a patch.

Thanks for the patch!

Thank you for your review.

When I compiled with the patch applied, I got the following warning:

copyfromparse.c:834:20: error: ‘done’ may be used uninitialized [-Werror=maybe-uninitialized]
   834 |                 if (done)
       |                    ^

Oh, sorry. I missed it.
I fixed it to initialize done = false.

+      lines are discarded.  An integer value greater than 1 is only valid for
+      <command>COPY FROM</command> commands.

This might be slightly misleading since 0 is also a valid value for COPY FROM.

 That's certainly true. I fixed it below:
+      lines are discarded.  An integer value greater than 1 is not allowed for
+      <command>COPY TO</command> commands.

Thanks for fixing that! However, it's odd that the description for COPY TO appears
under the section starting with "On input." Shouldn't it be under the "On output"
section instead?

Also, I think the entire HEADER option section could use a clearer structure.
How about revising it like this?

---------------------
      <listitem>
       <para>
-      Specifies that the file contains a header line with the names of each
-      column in the file.  On output, the first line contains the column
-      names from the table.  On input, the first line is discarded when this
-      option is set to <literal>true</literal> (or equivalent Boolean value).
-      If this option is set to <literal>MATCH</literal>, the number and names
-      of the columns in the header line must match the actual column names of
-      the table, in order;  otherwise an error is raised.
+      On output, if this option is set to <literal>true</literal>
+      (or an equivalent Boolean value), the first line of the output will
+      contain the column names from the table.
+      Integer values <literal>0</literal> and <literal>1</literal> are
+      accepted as Boolean values, but other integers are not allowed for
+      <command>COPY TO</command> commands.
+     </para>
+     <para>
+      On input, if this option is set to <literal>true</literal>
+      (or an equivalent Boolean value), the first line of the input is
+      discarded.  If it is set to a non-negative integer, that number of
+      lines are discarded.  If the option is set to <literal>MATCH</literal>,
+      the number and names of the columns in the header line must exactly
+      match those of the table, in order;  otherwise an error is raised.
+      The <literal>MATCH</literal> value is only valid for
+      <command>COPY FROM</command> commands.
+     </para>
+     <para>
        This option is not allowed when using <literal>binary</literal> format.
-      The <literal>MATCH</literal> option is only valid for <command>COPY
-      FROM</command> commands.
       </para>
      </listitem>
---------------------

Also, similar to the existing "boolean" type explanation in the COPY docs,
we should add a corresponding entry for integer, now that it's accepted by
the HEADER option. The VACUUM docs has a good example of how to phrase this.

Original error message "%s requires a Boolean value, integer value, or \"match\"" should also be fixed to "non-negative integer"?

Yes, I think that the message

"%s requires a Boolean value, a non-negative integer, or the string \"match\""

is clearer and more accurate.

-typedef enum CopyHeaderChoice
+typedef struct CopyHeaderOption
  {
-	COPY_HEADER_FALSE = 0,
-	COPY_HEADER_TRUE,
-	COPY_HEADER_MATCH,
-} CopyHeaderChoice;
+	bool	match;		/* header line must match actual names? */
+	int		skip_lines;	/* number of lines to skip before data */
+} CopyHeaderOption;
<snip>
-	CopyHeaderChoice header_line;	/* header line? */
+	CopyHeaderOption header;	/* header line? */

Wouldn't it be simpler to just use an integer instead of introducing
CopyHeaderOption? We could use 0 for false, 1 for true, n > 1 for skipping lines,
and -1 for MATCH in that integer.

This would simplify the logic in defGetCopyHeaderOption().
I've attached a patch that shows this idea in action. Thought?

Regards,

--
Fujii Masao
NTT DATA Japan Corporation

On Fri, Jun 27, 2025 at 12:03 AM Fujii Masao <masao.fujii@oss.nttdata.com>
wrote:

On 2025/06/26 19:12, Shinya Kato wrote:

On Thu, Jun 26, 2025 at 4:36 PM Fujii Masao <masao.fujii@oss.nttdata.com

<mailto:masao.fujii@oss.nttdata.com>> wrote:

On 2025/06/26 14:35, Shinya Kato wrote:

So it seems better for you to implement the patch at first

and then

check the performance overhead etc if necessary.

Thank you for your advice. I will create a patch.

I created a patch.

Thanks for the patch!

Thank you for your review.

When I compiled with the patch applied, I got the following warning:

copyfromparse.c:834:20: error: ‘done’ may be used uninitialized

[-Werror=maybe-uninitialized]

834 | if (done)
| ^

Oh, sorry. I missed it.
I fixed it to initialize done = false.

+ lines are discarded. An integer value greater than 1 is only

valid for

+ <command>COPY FROM</command> commands.

This might be slightly misleading since 0 is also a valid value for

COPY FROM.

That's certainly true. I fixed it below:
+ lines are discarded. An integer value greater than 1 is not

allowed for

+ <command>COPY TO</command> commands.

Thanks for fixing that! However, it's odd that the description for COPY TO
appears
under the section starting with "On input." Shouldn't it be under the "On
output"
section instead?

Also, I think the entire HEADER option section could use a clearer
structure.
How about revising it like this?

---------------------
<listitem>
<para>
-      Specifies that the file contains a header line with the names of
each
-      column in the file.  On output, the first line contains the column
-      names from the table.  On input, the first line is discarded when
this
-      option is set to <literal>true</literal> (or equivalent Boolean
value).
-      If this option is set to <literal>MATCH</literal>, the number and
names
-      of the columns in the header line must match the actual column
names of
-      the table, in order;  otherwise an error is raised.
+      On output, if this option is set to <literal>true</literal>
+      (or an equivalent Boolean value), the first line of the output will
+      contain the column names from the table.
+      Integer values <literal>0</literal> and <literal>1</literal> are
+      accepted as Boolean values, but other integers are not allowed for
+      <command>COPY TO</command> commands.
+     </para>
+     <para>
+      On input, if this option is set to <literal>true</literal>
+      (or an equivalent Boolean value), the first line of the input is
+      discarded.  If it is set to a non-negative integer, that number of
+      lines are discarded.  If the option is set to
<literal>MATCH</literal>,
+      the number and names of the columns in the header line must exactly
+      match those of the table, in order;  otherwise an error is raised.
+      The <literal>MATCH</literal> value is only valid for
+      <command>COPY FROM</command> commands.
+     </para>
+     <para>
This option is not allowed when using <literal>binary</literal>
format.
-      The <literal>MATCH</literal> option is only valid for <command>COPY
-      FROM</command> commands.
</para>
</listitem>
---------------------

Yes, your documentation is clearer than mine.

Also, similar to the existing "boolean" type explanation in the COPY docs,
we should add a corresponding entry for integer, now that it's accepted by
the HEADER option. The VACUUM docs has a good example of how to phrase
this.

Exactly.

Original error message "%s requires a Boolean value, integer value, or

\"match\"" should also be fixed to "non-negative integer"?

Yes, I think that the message

"%s requires a Boolean value, a non-negative integer, or the string
\"match\""

is clearer and more accurate.

Thank you, you're right.

-typedef enum CopyHeaderChoice
+typedef struct CopyHeaderOption
{
-       COPY_HEADER_FALSE = 0,
-       COPY_HEADER_TRUE,
-       COPY_HEADER_MATCH,
-} CopyHeaderChoice;
+       bool    match;          /* header line must match actual names? */
+       int             skip_lines;     /* number of lines to skip before
data */
+} CopyHeaderOption;
<snip>
-       CopyHeaderChoice header_line;   /* header line? */
+       CopyHeaderOption header;        /* header line? */

Wouldn't it be simpler to just use an integer instead of introducing
CopyHeaderOption? We could use 0 for false, 1 for true, n > 1 for skipping
lines,
and -1 for MATCH in that integer.

This would simplify the logic in defGetCopyHeaderOption().
I've attached a patch that shows this idea in action. Thought?

I thought about the approach, and had a minor concern about the following
inconsistency between the option and its internal implementation:
- When the option is set to "MATCH", header_line becomes -1.
- When the option is set to -1, it's an error.

However, your patch is clear, and it seems we don't need to worry about
this concern.
Your patch looks good to me.

--
Best regards,
Shinya Kato
NTT OSS Center

On 2025/06/27 11:55, Shinya Kato wrote:

I thought about the approach, and had a minor concern about the following inconsistency between the option and its internal implementation:
- When the option is set to "MATCH", header_line becomes -1.
- When the option is set to -1, it's an error.

I think this inconsistency is acceptable, since users won't encounter it directly.
Also the value -1 isn't used explicitly, i.e., it's always referenced through a macro
for "match", which keeps the code readable and understandable.

However, your patch is clear, and it seems we don't need to worry about this concern.
Your patch looks good to me.

Thanks for reviewing the patch! I've marked it as ready for committer.

Regards,

--
Fujii Masao
NTT DATA Japan Corporation

On Fri, 27 Jun 2025 12:22:17 +0900
Fujii Masao <masao.fujii@oss.nttdata.com> wrote:

However, your patch is clear, and it seems we don't need to worry about this concern.
Your patch looks good to me.

Thanks for reviewing the patch! I've marked it as ready for committer.

I have a few minor comment on the patch.

+				if (ival < 0)
+					ereport(ERROR,
+							(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+							 errmsg("%s requires a Boolean value, a non-negative "
+									"integer, or the string \"match\"",
+									def->defname)));

 	ereport(ERROR,
 			(errcode(ERRCODE_SYNTAX_ERROR),
-			 errmsg("%s requires a Boolean value or \"match\"",
+			 errmsg("%s requires a Boolean value, a non-negative integer, "
+					"or the string \"match\"",
 					def->defname)));

These two pieces of code raise the same error, but with different error codes:
one returns ERRCODE_INVALID_PARAMETER_VALUE, while the other returns ERRCODE_SYNTAX_ERROR.

I believe the former is more appropriate, although the existing code uses the
latter.　In any case, the error codes should be consistent.

Regarding the documentation, how about explicitly stating that when MATCH is specified, only
the first line is skipped? While this may seem obvious, it’s worth clarifying, as the semantics
of the HEADER option have become a bit more complex with this change.

Regards,
Yugo Nagata

--
Yugo Nagata <nagata@sraoss.co.jp>

On 2025/06/27 13:09, Yugo Nagata wrote:

On Fri, 27 Jun 2025 12:22:17 +0900
Fujii Masao <masao.fujii@oss.nttdata.com> wrote:

However, your patch is clear, and it seems we don't need to worry about this concern.
Your patch looks good to me.

Thanks for reviewing the patch! I've marked it as ready for committer.

I have a few minor comment on the patch.

+				if (ival < 0)
+					ereport(ERROR,
+							(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+							 errmsg("%s requires a Boolean value, a non-negative "
+									"integer, or the string \"match\"",
+									def->defname)));

ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
-			 errmsg("%s requires a Boolean value or \"match\"",
+			 errmsg("%s requires a Boolean value, a non-negative integer, "
+					"or the string \"match\"",
def->defname)));

These two pieces of code raise the same error, but with different error codes:
one returns ERRCODE_INVALID_PARAMETER_VALUE, while the other returns ERRCODE_SYNTAX_ERROR.

I believe the former is more appropriate, although the existing code uses the
latter.　In any case, the error codes should be consistent.

I'm not sure there's an actual rule like "the error code must match
if the error message is the same." But if using the same message with
a different error code is confusing, I'm fine with changing
the earlier message. For example:

  							(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-							 errmsg("%s requires a Boolean value, a non-negative "
-									"integer, or the string \"match\"",
+							 errmsg("a negative integer value cannot be "
+									"specified for %s",
  									def->defname)));

Regards,

--
Fujii Masao
NTT DATA Japan Corporation

On Sat, 28 Jun 2025 00:33:45 +0900
Fujii Masao <masao.fujii@oss.nttdata.com> wrote:

On 2025/06/27 13:09, Yugo Nagata wrote:

On Fri, 27 Jun 2025 12:22:17 +0900
Fujii Masao <masao.fujii@oss.nttdata.com> wrote:

However, your patch is clear, and it seems we don't need to worry about this concern.
Your patch looks good to me.

Thanks for reviewing the patch! I've marked it as ready for committer.

I have a few minor comment on the patch.

+				if (ival < 0)
+					ereport(ERROR,
+							(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+							 errmsg("%s requires a Boolean value, a non-negative "
+									"integer, or the string \"match\"",
+									def->defname)));

ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
-			 errmsg("%s requires a Boolean value or \"match\"",
+			 errmsg("%s requires a Boolean value, a non-negative integer, "
+					"or the string \"match\"",
def->defname)));

These two pieces of code raise the same error, but with different error codes:
one returns ERRCODE_INVALID_PARAMETER_VALUE, while the other returns ERRCODE_SYNTAX_ERROR.

I believe the former is more appropriate, although the existing code uses the
latter.　In any case, the error codes should be consistent.

I'm not sure there's an actual rule like "the error code must match
if the error message is the same." But if using the same message with
a different error code is confusing, I'm fine with changing
the earlier message. For example:

(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-							 errmsg("%s requires a Boolean value, a non-negative "
-									"integer, or the string \"match\"",
+							 errmsg("a negative integer value cannot be "
+									"specified for %s",
def->defname)));

Fair point. There might not be any explicit rule.
However, I feel it somewhat confusing.

After looking into the code, it seems that ERRCODE_SYNTAX_ERROR tends to be used
for invalid keywords or parameter types, while ERRCODE_INVALID_PARAMETER_VALUE
is typically used for values that are out of the acceptable range.

So, the proposed change seems reasonable to me.

Regards,
Yugo Nagata

--
Yugo Nagata <nagata@sraoss.co.jp>

On Mon, Jun 30, 2025 at 4:24 PM Yugo Nagata <nagata@sraoss.co.jp> wrote:

I have a few minor comment on the patch.

+                           if (ival < 0)
+                                   ereport(ERROR,
+                                                   (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+                                                    errmsg("%s requires a Boolean value, a non-negative "
+                                                                   "integer, or the string \"match\"",
+                                                                   def->defname)));

ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
-                    errmsg("%s requires a Boolean value or \"match\"",
+                    errmsg("%s requires a Boolean value, a non-negative integer, "
+                                   "or the string \"match\"",
def->defname)));

These two pieces of code raise the same error, but with different error codes:
one returns ERRCODE_INVALID_PARAMETER_VALUE, while the other returns ERRCODE_SYNTAX_ERROR.

I believe the former is more appropriate, although the existing code uses the
latter.　In any case, the error codes should be consistent.

I'm not sure there's an actual rule like "the error code must match
if the error message is the same." But if using the same message with
a different error code is confusing, I'm fine with changing
the earlier message. For example:

(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-                                                      errmsg("%s requires a Boolean value, a non-negative "
-                                                                     "integer, or the string \"match\"",
+                                                      errmsg("a negative integer value cannot be "
+                                                                     "specified for %s",
def->defname)));

Fair point. There might not be any explicit rule.
However, I feel it somewhat confusing.

After looking into the code, it seems that ERRCODE_SYNTAX_ERROR tends to be used
for invalid keywords or parameter types, while ERRCODE_INVALID_PARAMETER_VALUE
is typically used for values that are out of the acceptable range.

So, the proposed change seems reasonable to me.

Thank you for the review. The change looks good to me, too. A new
patch is attached.

Regarding the documentation, how about explicitly stating that when MATCH is specified, only
the first line is skipped? While this may seem obvious, it’s worth clarifying, as the semantics
of the HEADER option have become a bit more complex with this change.

Agreed. I have updated the documentation as follows:

+      lines are discarded.  If the option is set to <literal>MATCH</literal>,
+      the number and names of the columns in the header line must exactly
+      match those of the table and, in order, after which the header line is
+      discarded;  otherwise an error is raised.  The <literal>MATCH</literal>

--
Best regards,
Shinya Kato
NTT OSS Center