Thread
Commits
Same data as JSON:
GET /api/v1/messages/:b64id/commits
the thread's linked commits as JSON, with link sources.
API reference →
-
Support multi-line headers in COPY FROM command.
- bc2f348e87c0 19 (unreleased) landed
-
Extend COPY FROM with HEADER <integer> to skip multiple lines
Shinya Kato <shinya11.kato@gmail.com> — 2025-06-09T07:10:34Z
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? [1] https://dev.mysql.com/doc/refman/8.4/en/load-data.html#load-data-field-line-handling [2] https://learn.microsoft.com/en-us/sql/t-sql/statements/bulk-insert-transact-sql?view=sql-server-ver17#firstrow--first_row [3] https://docs.oracle.com/en/database/oracle/oracle-database/23/sutil/oracle-sql-loader-commands.html#SUTIL-GUID-84244C46-6AFD-412D-9240-BEB0B5C2718B -- Best regards, Shinya Kato NTT OSS Center
-
Re: Extend COPY FROM with HEADER <integer> to skip multiple lines
Fujii Masao <masao.fujii@oss.nttdata.com> — 2025-06-09T08:27:19Z
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] https://postgr.es/m/CALAY4q8nGSXp0P5uf56vn-mD7reWqZP5k6PS1CGUm26X4FsYJA@mail.gmail.com -- Fujii Masao NTT DATA Japan Corporation
-
Re: Extend COPY FROM with HEADER <integer> to skip multiple lines
Shinya Kato <shinya11.kato@gmail.com> — 2025-06-09T08:51:37Z
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] https://postgr.es/m/CALAY4q8nGSXp0P5uf56vn-mD7reWqZP5k6PS1CGUm26X4FsYJA@mail.gmail.com Oh, I missed it. I will check it soon. -- Best regards, Shinya Kato NTT OSS Center
-
Re: Extend COPY FROM with HEADER <integer> to skip multiple lines
Andrew Dunstan <andrew@dunslane.net> — 2025-06-09T13:28:20Z
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] > https://postgr.es/m/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
-
Re: Extend COPY FROM with HEADER <integer> to skip multiple lines
Shinya Kato <shinya11.kato@gmail.com> — 2025-06-10T00:43:10Z
> > 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] https://postgr.es/m/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
-
Re: Extend COPY FROM with HEADER <integer> to skip multiple lines
Fujii Masao <masao.fujii@oss.nttdata.com> — 2025-06-10T05:34:32Z
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] https://postgr.es/m/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
-
Re: Extend COPY FROM with HEADER <integer> to skip multiple lines
Shinya Kato <shinya11.kato@gmail.com> — 2025-06-10T05:46:11Z
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] https://postgr.es/m/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
-
Re: Extend COPY FROM with HEADER <integer> to skip multiple lines
Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> — 2025-06-10T10:05:30Z
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
-
Re: Extend COPY FROM with HEADER <integer> to skip multiple lines
Shinya Kato <shinya11.kato@gmail.com> — 2025-06-11T03:29:26Z
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
-
Re: Extend COPY FROM with HEADER <integer> to skip multiple lines
Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> — 2025-06-11T10:21:56Z
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
-
Re: Extend COPY FROM with HEADER <integer> to skip multiple lines
Shinya Kato <shinya11.kato@gmail.com> — 2025-06-26T05:35:34Z
> > 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
-
Re: Extend COPY FROM with HEADER <integer> to skip multiple lines
Fujii Masao <masao.fujii@oss.nttdata.com> — 2025-06-26T07:36:18Z
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 -
Re: Extend COPY FROM with HEADER <integer> to skip multiple lines
Shinya Kato <shinya11.kato@gmail.com> — 2025-06-26T10:12:03Z
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 -
Re: Extend COPY FROM with HEADER <integer> to skip multiple lines
Fujii Masao <masao.fujii@oss.nttdata.com> — 2025-06-26T15:03:43Z
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 -
Re: Extend COPY FROM with HEADER <integer> to skip multiple lines
Shinya Kato <shinya11.kato@gmail.com> — 2025-06-27T02:55:18Z
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 -
Re: Extend COPY FROM with HEADER <integer> to skip multiple lines
Fujii Masao <masao.fujii@oss.nttdata.com> — 2025-06-27T03:22:17Z
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
-
Re: Extend COPY FROM with HEADER <integer> to skip multiple lines
Yugo Nagata <nagata@sraoss.co.jp> — 2025-06-27T04:09:42Z
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> -
Re: Extend COPY FROM with HEADER <integer> to skip multiple lines
Fujii Masao <masao.fujii@oss.nttdata.com> — 2025-06-27T15:33:45Z
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 -
Re: Extend COPY FROM with HEADER <integer> to skip multiple lines
Yugo Nagata <nagata@sraoss.co.jp> — 2025-06-30T07:24:28Z
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> -
Re: Extend COPY FROM with HEADER <integer> to skip multiple lines
Shinya Kato <shinya11.kato@gmail.com> — 2025-07-02T05:39:43Z
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 -
Re: Extend COPY FROM with HEADER <integer> to skip multiple lines
Fujii Masao <masao.fujii@oss.nttdata.com> — 2025-07-02T07:48:15Z
On 2025/07/02 14:39, Shinya Kato wrote: > 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. Thanks for updating the patch! > >> 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> How about making the wording a bit clearer? For example: If set to MATCH, the first line is discarded, and it must contain column names that exactly match the table's columns, in both number and order; otherwise, an error is raised. Also, the phrase "if this option is set to..." is repeated three times in the current text. For the second and third instances, we could simplify it to just "if set to...". Regards, -- Fujii Masao NTT DATA Japan Corporation -
Re: Extend COPY FROM with HEADER <integer> to skip multiple lines
Shinya Kato <shinya11.kato@gmail.com> — 2025-07-03T02:08:22Z
On Wed, Jul 2, 2025 at 4:48 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote: > >> 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> > > How about making the wording a bit clearer? For example: > > If set to MATCH, the first line is discarded, and it must contain column names that > exactly match the table's columns, in both number and order; otherwise, an error is raised. Thank you for the review. I fixed it. > > Also, the phrase "if this option is set to..." is repeated three times in the current text. > For the second and third instances, we could simplify it to just "if set to...". Agreed. However, for the sake of symmetry between "On output" and "On input" and to maintain clarity between the paragraphs, I have omitted "this option is" from the "On input" paragraph only. -- Best regards, Shinya Kato NTT OSS Center
-
Re: Extend COPY FROM with HEADER <integer> to skip multiple lines
Fujii Masao <masao.fujii@oss.nttdata.com> — 2025-07-03T06:32:45Z
On 2025/07/03 11:08, Shinya Kato wrote: > On Wed, Jul 2, 2025 at 4:48 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote: > >>>> 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> >> >> How about making the wording a bit clearer? For example: >> >> If set to MATCH, the first line is discarded, and it must contain column names that >> exactly match the table's columns, in both number and order; otherwise, an error is raised. > > Thank you for the review. I fixed it. Thanks for updating the patch! I've pushed the patch. >> Also, the phrase "if this option is set to..." is repeated three times in the current text. >> For the second and third instances, we could simplify it to just "if set to...". > > Agreed. However, for the sake of symmetry between "On output" and "On > input" and to maintain clarity between the paragraphs, I have omitted > "this option is" from the "On input" paragraph only. Yes, I agree that's better. Regards, -- Fujii Masao NTT DATA Japan Corporation
-
Re: Extend COPY FROM with HEADER <integer> to skip multiple lines
Shinya Kato <shinya11.kato@gmail.com> — 2025-07-03T06:40:07Z
On Thu, Jul 3, 2025 at 3:32 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote: > > > > On 2025/07/03 11:08, Shinya Kato wrote: > > On Wed, Jul 2, 2025 at 4:48 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote: > > > >>>> 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> > >> > >> How about making the wording a bit clearer? For example: > >> > >> If set to MATCH, the first line is discarded, and it must contain column names that > >> exactly match the table's columns, in both number and order; otherwise, an error is raised. > > > > Thank you for the review. I fixed it. > > Thanks for updating the patch! I've pushed the patch. > > > >> Also, the phrase "if this option is set to..." is repeated three times in the current text. > >> For the second and third instances, we could simplify it to just "if set to...". > > > > Agreed. However, for the sake of symmetry between "On output" and "On > > input" and to maintain clarity between the paragraphs, I have omitted > > "this option is" from the "On input" paragraph only. > > Yes, I agree that's better. > > Regards, > > -- > Fujii Masao > NTT DATA Japan Corporation > Thank you for pushing! -- Best regards, Shinya Kato NTT OSS Center