public inbox for [email protected]help / color / mirror / Atom feed
Re: [PATCH v1] Allow COPY "text" to output a header and add header matching mode to COPY FROM 15+ messages / 9 participants [nested] [flat]
* Re: [PATCH v1] Allow COPY "text" to output a header and add header matching mode to COPY FROM @ 2020-03-01 23:45 Rémi Lapeyre <[email protected]> 0 siblings, 2 replies; 15+ messages in thread From: Rémi Lapeyre @ 2020-03-01 23:45 UTC (permalink / raw) To: pgsql-hackers I created an entry for this patch in the new CommiFest but it seems that it is not finding it. Is there anything that I need to do? ^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: [PATCH v1] Allow COPY "text" to output a header and add header matching mode to COPY FROM @ 2020-03-02 05:39 Surafel Temesgen <[email protected]> parent: Rémi Lapeyre <[email protected]> 1 sibling, 0 replies; 15+ messages in thread From: Surafel Temesgen @ 2020-03-02 05:39 UTC (permalink / raw) To: Rémi Lapeyre <[email protected]>; +Cc: pgsql-hackers On Mon, Mar 2, 2020 at 2:45 AM Rémi Lapeyre <[email protected]> wrote: > > I created an entry for this patch in the new CommiFest but it seems that > it is not finding it. Is there anything that I need to do? > > Is is added on next open commit fest which is https://commitfest.postgresql.org/28/ now regards Surafel ^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: [PATCH v1] Allow COPY "text" to output a header and add header matching mode to COPY FROM @ 2020-07-01 09:04 Daniel Gustafsson <[email protected]> parent: Rémi Lapeyre <[email protected]> 1 sibling, 2 replies; 15+ messages in thread From: Daniel Gustafsson @ 2020-07-01 09:04 UTC (permalink / raw) To: Rémi Lapeyre <[email protected]>; +Cc: pgsql-hackers > On 2 Mar 2020, at 00:45, Rémi Lapeyre <[email protected]> wrote: > I created an entry for this patch in the new CommiFest but it seems that it is not finding it. Is there anything that I need to do? This patch no longer applies cleanly on HEAD, due to changes in the regress tests. Please submit a rebased version, I've marked this entry as Waiting on Author for now. cheers ./daniel ^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: [PATCH v2] Allow COPY "text" to output a header and add header matching mode to COPY FROM @ 2020-07-08 13:21 Daniel Gustafsson <[email protected]> parent: Daniel Gustafsson <[email protected]> 1 sibling, 2 replies; 15+ messages in thread From: Daniel Gustafsson @ 2020-07-08 13:21 UTC (permalink / raw) To: Rémi Lapeyre <[email protected]>; +Cc: pgsql-hackers; Rémi Lapeyre <[email protected]> > On 8 Jul 2020, at 13:45, Rémi Lapeyre <[email protected]> wrote: > > Hi, here's a new version of the patch that should apply cleanly. I'll monitor > the status on http://cfbot.cputube.org/ Please reply to the old thread about this, as that's the one connected to the Commitfest entry and thats where all the discussion has happened. While additional threads can be attached to a CF entry, it's for when multiple discussions are relevant to a patch, a single discussion should not be broken into multiple threads. cheers ./daniel ^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: [PATCH v2] Allow COPY "text" to output a header and add header matching mode to COPY FROM @ 2020-07-08 13:51 Rémi Lapeyre <[email protected]> parent: Daniel Gustafsson <[email protected]> 1 sibling, 0 replies; 15+ messages in thread From: Rémi Lapeyre @ 2020-07-08 13:51 UTC (permalink / raw) To: Daniel Gustafsson <[email protected]>; +Cc: pgsql-hackers > > Please reply to the old thread about this, as that's the one connected to the > Commitfest entry and thats where all the discussion has happened. While > additional threads can be attached to a CF entry, it's for when multiple > discussions are relevant to a patch, a single discussion should not be broken > into multiple threads. Sorry about this, I thought setting the In-Reply-To like so: git send-email -v2 [email protected] [email protected] HEAD^ There is some nice informations about how to write a good commit but I could not find exactly how to send it so I probably did something wrong. > > cheers ./daniel ^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: [PATCH v2] Allow COPY "text" to output a header and add header matching mode to COPY FROM @ 2020-07-08 14:10 Justin Pryzby <[email protected]> parent: Daniel Gustafsson <[email protected]> 1 sibling, 0 replies; 15+ messages in thread From: Justin Pryzby @ 2020-07-08 14:10 UTC (permalink / raw) To: Daniel Gustafsson <[email protected]>; +Cc: Rémi Lapeyre <[email protected]>; pgsql-hackers; Rémi Lapeyre <[email protected]> On Wed, Jul 08, 2020 at 03:21:48PM +0200, Daniel Gustafsson wrote: > > On 8 Jul 2020, at 13:45, Rémi Lapeyre <[email protected]> wrote: > > > > Hi, here's a new version of the patch that should apply cleanly. I'll monitor > > the status on http://cfbot.cputube.org/ > > Please reply to the old thread about this, as that's the one connected to the Actually, it seems like the subject was changed, but it was correctly associated with the existing thread, as defined by: |In-Reply-To: <[email protected]> And ML/pglister and cfbot see it as such "In response to Re: [PATCH v1] Allow COPY "text" to output a header and add header matching mode to COPY FROM at 2020-07-01 09:04:21 from Daniel Gustafsson" https://commitfest.postgresql.org/28/2504/ -- Justin ^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: [PATCH v2] Allow COPY "text" to output a header and add header matching mode to COPY FROM @ 2020-07-10 11:34 Peter Eisentraut <[email protected]> parent: Daniel Gustafsson <[email protected]> 1 sibling, 1 reply; 15+ messages in thread From: Peter Eisentraut @ 2020-07-10 11:34 UTC (permalink / raw) To: Rémi Lapeyre <[email protected]>; pgsql-hackers; +Cc: Rémi Lapeyre <[email protected]> On 2020-07-08 13:45, Rémi Lapeyre wrote: > Hi, here's a new version of the patch that should apply cleanly. I'll monitor > the status on http://cfbot.cputube.org/ It's hard to find an explanation what this patch actually does. I don't want to have to go through threads dating back 4 months to determine what was discussed and what was actually implemented. Since you're already using git format-patch, just add something to the commit message. It appears that these are really two separate features, so perhaps they should be two patches. Also, the new header matching mode could probably use more than one line of documentation. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services ^ permalink raw reply [nested|flat] 15+ messages in thread
* Add header support to text format and matching feature @ 2020-07-17 14:54 Rémi Lapeyre <[email protected]> parent: Peter Eisentraut <[email protected]> 0 siblings, 3 replies; 15+ messages in thread From: Rémi Lapeyre @ 2020-07-17 14:54 UTC (permalink / raw) To: pgsql-hackers; [email protected] > It's hard to find an explanation what this patch actually does. I don't > want to have to go through threads dating back 4 months to determine > what was discussed and what was actually implemented. Since you're > already using git format-patch, just add something to the commit message. > > > It appears that these are really two separate features, so perhaps they > should be two patches. Thanks for the feedback, I've split cleanly the two patches, simplified the tests and tried to explain the changes in the commit message. > Also, the new header matching mode could probably use more than one line > of documentation. I've improved the documentation, let's me know if it's better. It seems like cfbot is not happy with the way I'm sending my patches. The wiki has some good advices on how to write a patch but I couldn't find anything on how to send it. I've used git send-email -v3 --compose --to=... --in-reply-to=... HEAD^^ here but I'm not sure if it's correct. I will see if it works and will try to fix it if it's not but since it runs once a day it may take some time. ^ permalink raw reply [nested|flat] 15+ messages in thread
* [PATCH v3 1/2] Add header support to "COPY TO" text format @ 2020-07-17 14:54 Rémi Lapeyre <[email protected]> parent: Rémi Lapeyre <[email protected]> 2 siblings, 0 replies; 15+ messages in thread From: Rémi Lapeyre @ 2020-07-17 14:54 UTC (permalink / raw) To: pgsql-hackers; [email protected]; +Cc: Rémi Lapeyre <[email protected]> CSV format supports the HEADER option to output a header in the output, it is convenient when other programs need to consume the output. This patch adds the same option to the default text format. Discussion: https://www.postgresql.org/message-id/flat/CAF1-J-0PtCWMeLtswwGV2M70U26n4g33gpe1rcKQqe6wVQDrFA@mail.... --- contrib/file_fdw/input/file_fdw.source | 1 - contrib/file_fdw/output/file_fdw.source | 4 +--- doc/src/sgml/ref/copy.sgml | 3 ++- src/backend/commands/copy.c | 11 +++++++---- src/test/regress/input/copy.source | 12 ++++++++++++ src/test/regress/output/copy.source | 8 ++++++++ 6 files changed, 30 insertions(+), 9 deletions(-) Attachments: [text/x-patch] v3-0001-Add-header-support-to-COPY-TO-text-format.patch (5.5K, ../../[email protected]/2-v3-0001-Add-header-support-to-COPY-TO-text-format.patch) download | inline diff: diff --git a/contrib/file_fdw/input/file_fdw.source b/contrib/file_fdw/input/file_fdw.source index 45b728eeb3..83edb71077 100644 --- a/contrib/file_fdw/input/file_fdw.source +++ b/contrib/file_fdw/input/file_fdw.source @@ -37,7 +37,6 @@ CREATE USER MAPPING FOR regress_no_priv_user SERVER file_server; -- validator tests CREATE FOREIGN TABLE tbl () SERVER file_server OPTIONS (format 'xml'); -- ERROR -CREATE FOREIGN TABLE tbl () SERVER file_server OPTIONS (format 'text', header 'true'); -- ERROR CREATE FOREIGN TABLE tbl () SERVER file_server OPTIONS (format 'text', quote ':'); -- ERROR CREATE FOREIGN TABLE tbl () SERVER file_server OPTIONS (format 'text', escape ':'); -- ERROR CREATE FOREIGN TABLE tbl () SERVER file_server OPTIONS (format 'binary', header 'true'); -- ERROR diff --git a/contrib/file_fdw/output/file_fdw.source b/contrib/file_fdw/output/file_fdw.source index 52b4d5f1df..547b81fd16 100644 --- a/contrib/file_fdw/output/file_fdw.source +++ b/contrib/file_fdw/output/file_fdw.source @@ -33,14 +33,12 @@ CREATE USER MAPPING FOR regress_no_priv_user SERVER file_server; -- validator tests CREATE FOREIGN TABLE tbl () SERVER file_server OPTIONS (format 'xml'); -- ERROR ERROR: COPY format "xml" not recognized -CREATE FOREIGN TABLE tbl () SERVER file_server OPTIONS (format 'text', header 'true'); -- ERROR -ERROR: COPY HEADER available only in CSV mode CREATE FOREIGN TABLE tbl () SERVER file_server OPTIONS (format 'text', quote ':'); -- ERROR ERROR: COPY quote available only in CSV mode CREATE FOREIGN TABLE tbl () SERVER file_server OPTIONS (format 'text', escape ':'); -- ERROR ERROR: COPY escape available only in CSV mode CREATE FOREIGN TABLE tbl () SERVER file_server OPTIONS (format 'binary', header 'true'); -- ERROR -ERROR: COPY HEADER available only in CSV mode +ERROR: COPY HEADER available only in CSV and text mode CREATE FOREIGN TABLE tbl () SERVER file_server OPTIONS (format 'binary', quote ':'); -- ERROR ERROR: COPY quote available only in CSV mode CREATE FOREIGN TABLE tbl () SERVER file_server OPTIONS (format 'binary', escape ':'); -- ERROR diff --git a/doc/src/sgml/ref/copy.sgml b/doc/src/sgml/ref/copy.sgml index 18189abc6c..c628a69c57 100644 --- a/doc/src/sgml/ref/copy.sgml +++ b/doc/src/sgml/ref/copy.sgml @@ -269,7 +269,8 @@ COPY { <replaceable class="parameter">table_name</replaceable> [ ( <replaceable 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, and on input, the first line is ignored. - This option is allowed only when using <literal>CSV</literal> format. + This option is allowed only when using <literal>CSV</literal> or + <literal>text</literal> format. </para> </listitem> </varlistentry> diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c index 44da71c4cb..a21508a974 100644 --- a/src/backend/commands/copy.c +++ b/src/backend/commands/copy.c @@ -136,7 +136,7 @@ typedef struct CopyStateData bool binary; /* binary format? */ bool freeze; /* freeze rows on loading? */ bool csv_mode; /* Comma Separated Value format? */ - bool header_line; /* CSV header line? */ + bool header_line; /* CSV or text header line? */ char *null_print; /* NULL marker string (server encoding!) */ int null_print_len; /* length of same */ char *null_print_client; /* same converted to file encoding */ @@ -1363,10 +1363,10 @@ ProcessCopyOptions(ParseState *pstate, errmsg("COPY delimiter cannot be \"%s\"", cstate->delim))); /* Check header */ - if (!cstate->csv_mode && cstate->header_line) + if (cstate->binary && cstate->header_line) ereport(ERROR, (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), - errmsg("COPY HEADER available only in CSV mode"))); + errmsg("COPY HEADER available only in CSV and text mode"))); /* Check quote */ if (!cstate->csv_mode && cstate->quote != NULL) @@ -2099,8 +2099,11 @@ CopyTo(CopyState cstate) colname = NameStr(TupleDescAttr(tupDesc, attnum - 1)->attname); - CopyAttributeOutCSV(cstate, colname, false, + if (cstate->csv_mode) + CopyAttributeOutCSV(cstate, colname, false, list_length(cstate->attnumlist) == 1); + else + CopyAttributeOutText(cstate, colname); } CopySendEndOfRow(cstate); diff --git a/src/test/regress/input/copy.source b/src/test/regress/input/copy.source index a1d529ad36..2368649111 100644 --- a/src/test/regress/input/copy.source +++ b/src/test/regress/input/copy.source @@ -134,6 +134,18 @@ this is just a line full of junk that would error out if parsed copy copytest3 to stdout csv header; +create temp table copytest4 ( + c1 int, + "col with tabulation: " text); + +copy copytest4 from stdin (header); +this is just a line full of junk that would error out if parsed +1 a +2 b +\. + +copy copytest4 to stdout (header); + -- test copy from with a partitioned table create table parted_copytest ( a int, diff --git a/src/test/regress/output/copy.source b/src/test/regress/output/copy.source index 938d3551da..c1f7f99747 100644 --- a/src/test/regress/output/copy.source +++ b/src/test/regress/output/copy.source @@ -95,6 +95,14 @@ copy copytest3 to stdout csv header; c1,"col with , comma","col with "" quote" 1,a,1 2,b,2 +create temp table copytest4 ( + c1 int, + "col with tabulation: " text); +copy copytest4 from stdin (header); +copy copytest4 to stdout (header); +c1 col with tabulation: \t +1 a +2 b -- test copy from with a partitioned table create table parted_copytest ( a int, ^ permalink raw reply [nested|flat] 15+ messages in thread
* [PATCH v3 2/2] Add header matching mode to "COPY FROM" @ 2020-07-17 14:54 Rémi Lapeyre <[email protected]> parent: Rémi Lapeyre <[email protected]> 2 siblings, 0 replies; 15+ messages in thread From: Rémi Lapeyre @ 2020-07-17 14:54 UTC (permalink / raw) To: pgsql-hackers; [email protected]; +Cc: Rémi Lapeyre <[email protected]> COPY FROM supports the HEADER option to silently discard the header from a CSV or text file. It is possible to load by mistake a file that matches the expected format, for example if two text columns have been swapped, resulting in garbage in the database. This option adds the possibility to actually check the header to make sure it matches what is expected and exit immediatly if it does not. Discussion: https://www.postgresql.org/message-id/flat/CAF1-J-0PtCWMeLtswwGV2M70U26n4g33gpe1rcKQqe6wVQDrFA@mail.... --- contrib/file_fdw/input/file_fdw.source | 6 ++ contrib/file_fdw/output/file_fdw.source | 9 ++- doc/src/sgml/ref/copy.sgml | 8 ++- src/backend/commands/copy.c | 84 +++++++++++++++++++++++-- src/test/regress/input/copy.source | 25 ++++++++ src/test/regress/output/copy.source | 17 +++++ 6 files changed, 140 insertions(+), 9 deletions(-) Attachments: [text/x-patch] v3-0002-Add-header-matching-mode-to-COPY-FROM.patch (9.9K, ../../[email protected]/2-v3-0002-Add-header-matching-mode-to-COPY-FROM.patch) download | inline diff: diff --git a/contrib/file_fdw/input/file_fdw.source b/contrib/file_fdw/input/file_fdw.source index 83edb71077..7a3983c785 100644 --- a/contrib/file_fdw/input/file_fdw.source +++ b/contrib/file_fdw/input/file_fdw.source @@ -79,6 +79,12 @@ CREATE FOREIGN TABLE agg_bad ( OPTIONS (format 'csv', filename '@abs_srcdir@/data/agg.bad', header 'true', delimiter ';', quote '@', escape '"', null ''); ALTER FOREIGN TABLE agg_bad ADD CHECK (a >= 0); +-- test header matching +CREATE FOREIGN TABLE header_match ("1" int, foo text) SERVER file_server +OPTIONS (format 'csv', filename '@abs_srcdir@/data/list1.csv', delimiter ',', header 'match'); +CREATE FOREIGN TABLE header_dont_match (a int, foo text) SERVER file_server +OPTIONS (format 'csv', filename '@abs_srcdir@/data/list1.csv', delimiter ',', header 'match'); -- ERROR + -- per-column options tests CREATE FOREIGN TABLE text_csv ( word1 text OPTIONS (force_not_null 'true'), diff --git a/contrib/file_fdw/output/file_fdw.source b/contrib/file_fdw/output/file_fdw.source index 547b81fd16..d76a3dc6f8 100644 --- a/contrib/file_fdw/output/file_fdw.source +++ b/contrib/file_fdw/output/file_fdw.source @@ -93,6 +93,11 @@ CREATE FOREIGN TABLE agg_bad ( ) SERVER file_server OPTIONS (format 'csv', filename '@abs_srcdir@/data/agg.bad', header 'true', delimiter ';', quote '@', escape '"', null ''); ALTER FOREIGN TABLE agg_bad ADD CHECK (a >= 0); +-- test header matching +CREATE FOREIGN TABLE header_match ("1" int, foo text) SERVER file_server +OPTIONS (format 'csv', filename '/Users/remi/src/postgresql/contrib/file_fdw/data/list1.csv', delimiter ',', header 'match'); +CREATE FOREIGN TABLE header_dont_match (a int, foo text) SERVER file_server +OPTIONS (format 'csv', filename '/Users/remi/src/postgresql/contrib/file_fdw/data/list1.csv', delimiter ',', header 'match'); -- ERROR -- per-column options tests CREATE FOREIGN TABLE text_csv ( word1 text OPTIONS (force_not_null 'true'), @@ -439,12 +444,14 @@ SET ROLE regress_file_fdw_superuser; -- cleanup RESET ROLE; DROP EXTENSION file_fdw CASCADE; -NOTICE: drop cascades to 7 other objects +NOTICE: drop cascades to 9 other objects DETAIL: drop cascades to server file_server drop cascades to user mapping for regress_file_fdw_superuser on server file_server drop cascades to user mapping for regress_no_priv_user on server file_server drop cascades to foreign table agg_text drop cascades to foreign table agg_csv drop cascades to foreign table agg_bad +drop cascades to foreign table header_match +drop cascades to foreign table header_dont_match drop cascades to foreign table text_csv DROP ROLE regress_file_fdw_superuser, regress_file_fdw_user, regress_no_priv_user; diff --git a/doc/src/sgml/ref/copy.sgml b/doc/src/sgml/ref/copy.sgml index c628a69c57..c35914511f 100644 --- a/doc/src/sgml/ref/copy.sgml +++ b/doc/src/sgml/ref/copy.sgml @@ -36,7 +36,7 @@ COPY { <replaceable class="parameter">table_name</replaceable> [ ( <replaceable FREEZE [ <replaceable class="parameter">boolean</replaceable> ] DELIMITER '<replaceable class="parameter">delimiter_character</replaceable>' NULL '<replaceable class="parameter">null_string</replaceable>' - HEADER [ <replaceable class="parameter">boolean</replaceable> ] + HEADER { <literal>match</literal> | <literal>true</literal> | <literal>false</literal> } QUOTE '<replaceable class="parameter">quote_character</replaceable>' ESCAPE '<replaceable class="parameter">escape_character</replaceable>' FORCE_QUOTE { ( <replaceable class="parameter">column_name</replaceable> [, ...] ) | * } @@ -268,7 +268,11 @@ COPY { <replaceable class="parameter">table_name</replaceable> [ ( <replaceable <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, and on input, the first line is ignored. + names from the table. On input, the first line is discarded when set + to <literal>true</literal> or required to match the column names if set + to <literal>match</literal>. If the number of columns in the header is + not correct, their order differs from the one expected, or the name or + case do not match, the copy will be aborted with an error. This option is allowed only when using <literal>CSV</literal> or <literal>text</literal> format. </para> diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c index a21508a974..cde6582f1a 100644 --- a/src/backend/commands/copy.c +++ b/src/backend/commands/copy.c @@ -95,6 +95,16 @@ typedef enum CopyInsertMethod CIM_MULTI_CONDITIONAL /* use table_multi_insert only if valid */ } CopyInsertMethod; +/* + * Represents whether the header must be absent, present or present and match. + */ +typedef enum CopyHeader +{ + COPY_HEADER_ABSENT, + COPY_HEADER_PRESENT, + COPY_HEADER_MATCH +} CopyHeader; + /* * This struct contains all the state variables used throughout a COPY * operation. For simplicity, we use the same struct for all variants of COPY, @@ -136,7 +146,7 @@ typedef struct CopyStateData bool binary; /* binary format? */ bool freeze; /* freeze rows on loading? */ bool csv_mode; /* Comma Separated Value format? */ - bool header_line; /* CSV or text header line? */ + CopyHeader header_line; /* CSV or text header line? */ char *null_print; /* NULL marker string (server encoding!) */ int null_print_len; /* length of same */ char *null_print_client; /* same converted to file encoding */ @@ -1182,7 +1192,28 @@ ProcessCopyOptions(ParseState *pstate, (errcode(ERRCODE_SYNTAX_ERROR), errmsg("conflicting or redundant options"), parser_errposition(pstate, defel->location))); - cstate->header_line = defGetBoolean(defel); + + PG_TRY(); + { + if (defGetBoolean(defel)) + cstate->header_line = COPY_HEADER_PRESENT; + else + cstate->header_line = COPY_HEADER_ABSENT; + } + PG_CATCH(); + { + if (!cstate->is_copy_from) + PG_RE_THROW(); + + char *sval = defGetString(defel); + if (pg_strcasecmp(sval, "match") == 0) + cstate->header_line = COPY_HEADER_MATCH; + else + ereport(ERROR, + (errcode(ERRCODE_SYNTAX_ERROR), + errmsg("header requires a boolean or \"match\""))); + } + PG_END_TRY(); } else if (strcmp(defel->defname, "quote") == 0) { @@ -2101,7 +2132,7 @@ CopyTo(CopyState cstate) if (cstate->csv_mode) CopyAttributeOutCSV(cstate, colname, false, - list_length(cstate->attnumlist) == 1); + list_length(cstate->attnumlist) == 1); else CopyAttributeOutText(cstate, colname); } @@ -3599,12 +3630,53 @@ NextCopyFromRawFields(CopyState cstate, char ***fields, int *nfields) /* only available for text or csv input */ Assert(!cstate->binary); - /* on input just throw the header line away */ + /* on input check that the header line is correct if needed */ if (cstate->cur_lineno == 0 && cstate->header_line) { + ListCell *cur; + TupleDesc tupDesc; + + tupDesc = RelationGetDescr(cstate->rel); + cstate->cur_lineno++; - if (CopyReadLine(cstate)) - return false; /* done */ + done = CopyReadLine(cstate); + + if (cstate->header_line == COPY_HEADER_MATCH) + { + if (cstate->csv_mode) + fldct = CopyReadAttributesCSV(cstate); + else + fldct = CopyReadAttributesText(cstate); + + if (fldct < list_length(cstate->attnumlist)) + ereport(ERROR, + (errcode(ERRCODE_BAD_COPY_FILE_FORMAT), + errmsg("missing header"))); + else if (fldct > list_length(cstate->attnumlist)) + ereport(ERROR, + (errcode(ERRCODE_BAD_COPY_FILE_FORMAT), + errmsg("extra data after last expected header"))); + + foreach(cur, cstate->attnumlist) + { + int attnum = lfirst_int(cur); + char *colName = cstate->raw_fields[attnum - 1]; + Form_pg_attribute attr = TupleDescAttr(tupDesc, attnum - 1); + + if (colName == NULL) + colName = cstate->null_print; + + if (namestrcmp(&attr->attname, colName) != 0) { + ereport(ERROR, + (errcode(ERRCODE_BAD_COPY_FILE_FORMAT), + errmsg("wrong header for column \"%s\": got \"%s\"", + NameStr(attr->attname), colName))); + } + } + } + + if (done) + return false; } cstate->cur_lineno++; diff --git a/src/test/regress/input/copy.source b/src/test/regress/input/copy.source index 2368649111..4d21c7d524 100644 --- a/src/test/regress/input/copy.source +++ b/src/test/regress/input/copy.source @@ -213,3 +213,28 @@ select * from parted_copytest where b = 1; select * from parted_copytest where b = 2; drop table parted_copytest; + +-- Test header matching feature +create table header_copytest ( + a int, + b int, + c text +); +copy header_copytest from stdin with (header wrong_choice); +copy header_copytest from stdin with (header match); +a b c +1 2 foo +\. +copy header_copytest from stdin with (header match); +a b +1 2 +\. +copy header_copytest from stdin with (header match); +a b c d +1 2 foo bar +\. +copy header_copytest from stdin with (header match, format csv); +a,b,c +1,2,foo +\. +drop table header_copytest; diff --git a/src/test/regress/output/copy.source b/src/test/regress/output/copy.source index c1f7f99747..b792181fe3 100644 --- a/src/test/regress/output/copy.source +++ b/src/test/regress/output/copy.source @@ -173,3 +173,20 @@ select * from parted_copytest where b = 2; (1 row) drop table parted_copytest; +-- Test header matching feature +create table header_copytest ( + a int, + b int, + c text +); +copy header_copytest from stdin with (header wrong_choice); +ERROR: header requires a boolean or "match" +copy header_copytest from stdin with (header match); +copy header_copytest from stdin with (header match); +ERROR: missing header +CONTEXT: COPY header_copytest, line 1: "a b" +copy header_copytest from stdin with (header match); +ERROR: extra data after last expected header +CONTEXT: COPY header_copytest, line 1: "a b c d" +copy header_copytest from stdin with (header match, format csv); +drop table header_copytest; ^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: Add header support to text format and matching feature @ 2020-07-17 15:27 Magnus Hagander <[email protected]> parent: Rémi Lapeyre <[email protected]> 2 siblings, 1 reply; 15+ messages in thread From: Magnus Hagander @ 2020-07-17 15:27 UTC (permalink / raw) To: Rémi Lapeyre <[email protected]>; +Cc: pgsql-hackers; Peter Eisentraut <[email protected]> On Fri, Jul 17, 2020 at 5:11 PM Rémi Lapeyre <[email protected]> wrote: > > > > It's hard to find an explanation what this patch actually does. I don't > > want to have to go through threads dating back 4 months to determine > > what was discussed and what was actually implemented. Since you're > > already using git format-patch, just add something to the commit message. > > > > > > It appears that these are really two separate features, so perhaps they > > should be two patches. > > Thanks for the feedback, I've split cleanly the two patches, simplified the > tests and tried to explain the changes in the commit message. > > > Also, the new header matching mode could probably use more than one line > > of documentation. > > I've improved the documentation, let's me know if it's better. > > It seems like cfbot is not happy with the way I'm sending my patches. The > wiki > has some good advices on how to write a patch but I couldn't find anything > on > how to send it. I've used > git send-email -v3 --compose --to=... --in-reply-to=... HEAD^^ > > here but I'm not sure if it's correct. I will see if it works and will try > to fix > it if it's not but since it runs once a day it may take some time. > If you have two patches that depend on each other, you should send them as two attachment to the same email. You now sent them as two separate emails, and cfbot will then pick up the latest one of them which is only patch 0002 (at least I'm fairly sure that's how it works). I don't know how to do that with git-send-email, but you can certainly do it easy with git-format-patch and just attach them using your regular MUA. (and while the cfbot and the archives have no problems dealing with the change in subject, it does break threading in some other MUAs, so I would recommend not doing that and sticking to the existing subject of the thread) -- Magnus Hagander Me: https://www.hagander.net/ <http://www.hagander.net/; Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/; ^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: Add header support to text format and matching feature @ 2020-07-17 16:48 Rémi Lapeyre <[email protected]> parent: Magnus Hagander <[email protected]> 0 siblings, 1 reply; 15+ messages in thread From: Rémi Lapeyre @ 2020-07-17 16:48 UTC (permalink / raw) To: pgsql-hackers > > I don't know how to do that with git-send-email, but you can certainly do it easy with git-format-patch and just attach them using your regular MUA. > > (and while the cfbot and the archives have no problems dealing with the change in subject, it does break threading in some other MUAs, so I would recommend not doing that and sticking to the existing subject of the thread) > Thanks, here are both patches attached so cfbot can read them. Attachments: [application/octet-stream] v3-0001-Add-header-support-to-COPY-TO-text-format.patch (7.0K, ../../[email protected]/2-v3-0001-Add-header-support-to-COPY-TO-text-format.patch) download | inline diff: From 99fa3b6d623105c5eea6fe14b2dc7287663fe8fb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9mi=20Lapeyre?= <[email protected]> Date: Fri, 17 Jul 2020 01:50:06 +0200 Subject: [PATCH v3 1/2] Add header support to "COPY TO" text format MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="------------2.27.0" This is a multi-part message in MIME format. --------------2.27.0 Content-Type: text/plain; charset=UTF-8; format=fixed Content-Transfer-Encoding: 8bit CSV format supports the HEADER option to output a header in the output, it is convenient when other programs need to consume the output. This patch adds the same option to the default text format. Discussion: https://www.postgresql.org/message-id/flat/CAF1-J-0PtCWMeLtswwGV2M70U26n4g33gpe1rcKQqe6wVQDrFA@mail.gmail.com --- contrib/file_fdw/input/file_fdw.source | 1 - contrib/file_fdw/output/file_fdw.source | 4 +--- doc/src/sgml/ref/copy.sgml | 3 ++- src/backend/commands/copy.c | 11 +++++++---- src/test/regress/input/copy.source | 12 ++++++++++++ src/test/regress/output/copy.source | 8 ++++++++ 6 files changed, 30 insertions(+), 9 deletions(-) --------------2.27.0 Content-Type: text/x-patch; name="v3-0001-Add-header-support-to-COPY-TO-text-format.patch" Content-Transfer-Encoding: 8bit Content-Disposition: attachment; filename="v3-0001-Add-header-support-to-COPY-TO-text-format.patch" diff --git a/contrib/file_fdw/input/file_fdw.source b/contrib/file_fdw/input/file_fdw.source index 45b728eeb3..83edb71077 100644 --- a/contrib/file_fdw/input/file_fdw.source +++ b/contrib/file_fdw/input/file_fdw.source @@ -37,7 +37,6 @@ CREATE USER MAPPING FOR regress_no_priv_user SERVER file_server; -- validator tests CREATE FOREIGN TABLE tbl () SERVER file_server OPTIONS (format 'xml'); -- ERROR -CREATE FOREIGN TABLE tbl () SERVER file_server OPTIONS (format 'text', header 'true'); -- ERROR CREATE FOREIGN TABLE tbl () SERVER file_server OPTIONS (format 'text', quote ':'); -- ERROR CREATE FOREIGN TABLE tbl () SERVER file_server OPTIONS (format 'text', escape ':'); -- ERROR CREATE FOREIGN TABLE tbl () SERVER file_server OPTIONS (format 'binary', header 'true'); -- ERROR diff --git a/contrib/file_fdw/output/file_fdw.source b/contrib/file_fdw/output/file_fdw.source index 52b4d5f1df..547b81fd16 100644 --- a/contrib/file_fdw/output/file_fdw.source +++ b/contrib/file_fdw/output/file_fdw.source @@ -33,14 +33,12 @@ CREATE USER MAPPING FOR regress_no_priv_user SERVER file_server; -- validator tests CREATE FOREIGN TABLE tbl () SERVER file_server OPTIONS (format 'xml'); -- ERROR ERROR: COPY format "xml" not recognized -CREATE FOREIGN TABLE tbl () SERVER file_server OPTIONS (format 'text', header 'true'); -- ERROR -ERROR: COPY HEADER available only in CSV mode CREATE FOREIGN TABLE tbl () SERVER file_server OPTIONS (format 'text', quote ':'); -- ERROR ERROR: COPY quote available only in CSV mode CREATE FOREIGN TABLE tbl () SERVER file_server OPTIONS (format 'text', escape ':'); -- ERROR ERROR: COPY escape available only in CSV mode CREATE FOREIGN TABLE tbl () SERVER file_server OPTIONS (format 'binary', header 'true'); -- ERROR -ERROR: COPY HEADER available only in CSV mode +ERROR: COPY HEADER available only in CSV and text mode CREATE FOREIGN TABLE tbl () SERVER file_server OPTIONS (format 'binary', quote ':'); -- ERROR ERROR: COPY quote available only in CSV mode CREATE FOREIGN TABLE tbl () SERVER file_server OPTIONS (format 'binary', escape ':'); -- ERROR diff --git a/doc/src/sgml/ref/copy.sgml b/doc/src/sgml/ref/copy.sgml index 18189abc6c..c628a69c57 100644 --- a/doc/src/sgml/ref/copy.sgml +++ b/doc/src/sgml/ref/copy.sgml @@ -269,7 +269,8 @@ COPY { <replaceable class="parameter">table_name</replaceable> [ ( <replaceable 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, and on input, the first line is ignored. - This option is allowed only when using <literal>CSV</literal> format. + This option is allowed only when using <literal>CSV</literal> or + <literal>text</literal> format. </para> </listitem> </varlistentry> diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c index 44da71c4cb..a21508a974 100644 --- a/src/backend/commands/copy.c +++ b/src/backend/commands/copy.c @@ -136,7 +136,7 @@ typedef struct CopyStateData bool binary; /* binary format? */ bool freeze; /* freeze rows on loading? */ bool csv_mode; /* Comma Separated Value format? */ - bool header_line; /* CSV header line? */ + bool header_line; /* CSV or text header line? */ char *null_print; /* NULL marker string (server encoding!) */ int null_print_len; /* length of same */ char *null_print_client; /* same converted to file encoding */ @@ -1363,10 +1363,10 @@ ProcessCopyOptions(ParseState *pstate, errmsg("COPY delimiter cannot be \"%s\"", cstate->delim))); /* Check header */ - if (!cstate->csv_mode && cstate->header_line) + if (cstate->binary && cstate->header_line) ereport(ERROR, (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), - errmsg("COPY HEADER available only in CSV mode"))); + errmsg("COPY HEADER available only in CSV and text mode"))); /* Check quote */ if (!cstate->csv_mode && cstate->quote != NULL) @@ -2099,8 +2099,11 @@ CopyTo(CopyState cstate) colname = NameStr(TupleDescAttr(tupDesc, attnum - 1)->attname); - CopyAttributeOutCSV(cstate, colname, false, + if (cstate->csv_mode) + CopyAttributeOutCSV(cstate, colname, false, list_length(cstate->attnumlist) == 1); + else + CopyAttributeOutText(cstate, colname); } CopySendEndOfRow(cstate); diff --git a/src/test/regress/input/copy.source b/src/test/regress/input/copy.source index a1d529ad36..2368649111 100644 --- a/src/test/regress/input/copy.source +++ b/src/test/regress/input/copy.source @@ -134,6 +134,18 @@ this is just a line full of junk that would error out if parsed copy copytest3 to stdout csv header; +create temp table copytest4 ( + c1 int, + "col with tabulation: " text); + +copy copytest4 from stdin (header); +this is just a line full of junk that would error out if parsed +1 a +2 b +\. + +copy copytest4 to stdout (header); + -- test copy from with a partitioned table create table parted_copytest ( a int, diff --git a/src/test/regress/output/copy.source b/src/test/regress/output/copy.source index 938d3551da..c1f7f99747 100644 --- a/src/test/regress/output/copy.source +++ b/src/test/regress/output/copy.source @@ -95,6 +95,14 @@ copy copytest3 to stdout csv header; c1,"col with , comma","col with "" quote" 1,a,1 2,b,2 +create temp table copytest4 ( + c1 int, + "col with tabulation: " text); +copy copytest4 from stdin (header); +copy copytest4 to stdout (header); +c1 col with tabulation: \t +1 a +2 b -- test copy from with a partitioned table create table parted_copytest ( a int, --------------2.27.0-- [application/octet-stream] v3-0002-Add-header-matching-mode-to-COPY-FROM.patch (11.5K, ../../[email protected]/3-v3-0002-Add-header-matching-mode-to-COPY-FROM.patch) download | inline diff: From 3e803c01593b6283a9deb7e4ddedca3d28650259 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9mi=20Lapeyre?= <[email protected]> Date: Fri, 17 Jul 2020 02:04:55 +0200 Subject: [PATCH v3 2/2] Add header matching mode to "COPY FROM" MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="------------2.27.0" This is a multi-part message in MIME format. --------------2.27.0 Content-Type: text/plain; charset=UTF-8; format=fixed Content-Transfer-Encoding: 8bit COPY FROM supports the HEADER option to silently discard the header from a CSV or text file. It is possible to load by mistake a file that matches the expected format, for example if two text columns have been swapped, resulting in garbage in the database. This option adds the possibility to actually check the header to make sure it matches what is expected and exit immediatly if it does not. Discussion: https://www.postgresql.org/message-id/flat/CAF1-J-0PtCWMeLtswwGV2M70U26n4g33gpe1rcKQqe6wVQDrFA@mail.gmail.com --- contrib/file_fdw/input/file_fdw.source | 6 ++ contrib/file_fdw/output/file_fdw.source | 9 ++- doc/src/sgml/ref/copy.sgml | 8 ++- src/backend/commands/copy.c | 84 +++++++++++++++++++++++-- src/test/regress/input/copy.source | 25 ++++++++ src/test/regress/output/copy.source | 17 +++++ 6 files changed, 140 insertions(+), 9 deletions(-) --------------2.27.0 Content-Type: text/x-patch; name="v3-0002-Add-header-matching-mode-to-COPY-FROM.patch" Content-Transfer-Encoding: 8bit Content-Disposition: attachment; filename="v3-0002-Add-header-matching-mode-to-COPY-FROM.patch" diff --git a/contrib/file_fdw/input/file_fdw.source b/contrib/file_fdw/input/file_fdw.source index 83edb71077..7a3983c785 100644 --- a/contrib/file_fdw/input/file_fdw.source +++ b/contrib/file_fdw/input/file_fdw.source @@ -79,6 +79,12 @@ CREATE FOREIGN TABLE agg_bad ( OPTIONS (format 'csv', filename '@abs_srcdir@/data/agg.bad', header 'true', delimiter ';', quote '@', escape '"', null ''); ALTER FOREIGN TABLE agg_bad ADD CHECK (a >= 0); +-- test header matching +CREATE FOREIGN TABLE header_match ("1" int, foo text) SERVER file_server +OPTIONS (format 'csv', filename '@abs_srcdir@/data/list1.csv', delimiter ',', header 'match'); +CREATE FOREIGN TABLE header_dont_match (a int, foo text) SERVER file_server +OPTIONS (format 'csv', filename '@abs_srcdir@/data/list1.csv', delimiter ',', header 'match'); -- ERROR + -- per-column options tests CREATE FOREIGN TABLE text_csv ( word1 text OPTIONS (force_not_null 'true'), diff --git a/contrib/file_fdw/output/file_fdw.source b/contrib/file_fdw/output/file_fdw.source index 547b81fd16..d76a3dc6f8 100644 --- a/contrib/file_fdw/output/file_fdw.source +++ b/contrib/file_fdw/output/file_fdw.source @@ -93,6 +93,11 @@ CREATE FOREIGN TABLE agg_bad ( ) SERVER file_server OPTIONS (format 'csv', filename '@abs_srcdir@/data/agg.bad', header 'true', delimiter ';', quote '@', escape '"', null ''); ALTER FOREIGN TABLE agg_bad ADD CHECK (a >= 0); +-- test header matching +CREATE FOREIGN TABLE header_match ("1" int, foo text) SERVER file_server +OPTIONS (format 'csv', filename '/Users/remi/src/postgresql/contrib/file_fdw/data/list1.csv', delimiter ',', header 'match'); +CREATE FOREIGN TABLE header_dont_match (a int, foo text) SERVER file_server +OPTIONS (format 'csv', filename '/Users/remi/src/postgresql/contrib/file_fdw/data/list1.csv', delimiter ',', header 'match'); -- ERROR -- per-column options tests CREATE FOREIGN TABLE text_csv ( word1 text OPTIONS (force_not_null 'true'), @@ -439,12 +444,14 @@ SET ROLE regress_file_fdw_superuser; -- cleanup RESET ROLE; DROP EXTENSION file_fdw CASCADE; -NOTICE: drop cascades to 7 other objects +NOTICE: drop cascades to 9 other objects DETAIL: drop cascades to server file_server drop cascades to user mapping for regress_file_fdw_superuser on server file_server drop cascades to user mapping for regress_no_priv_user on server file_server drop cascades to foreign table agg_text drop cascades to foreign table agg_csv drop cascades to foreign table agg_bad +drop cascades to foreign table header_match +drop cascades to foreign table header_dont_match drop cascades to foreign table text_csv DROP ROLE regress_file_fdw_superuser, regress_file_fdw_user, regress_no_priv_user; diff --git a/doc/src/sgml/ref/copy.sgml b/doc/src/sgml/ref/copy.sgml index c628a69c57..c35914511f 100644 --- a/doc/src/sgml/ref/copy.sgml +++ b/doc/src/sgml/ref/copy.sgml @@ -36,7 +36,7 @@ COPY { <replaceable class="parameter">table_name</replaceable> [ ( <replaceable FREEZE [ <replaceable class="parameter">boolean</replaceable> ] DELIMITER '<replaceable class="parameter">delimiter_character</replaceable>' NULL '<replaceable class="parameter">null_string</replaceable>' - HEADER [ <replaceable class="parameter">boolean</replaceable> ] + HEADER { <literal>match</literal> | <literal>true</literal> | <literal>false</literal> } QUOTE '<replaceable class="parameter">quote_character</replaceable>' ESCAPE '<replaceable class="parameter">escape_character</replaceable>' FORCE_QUOTE { ( <replaceable class="parameter">column_name</replaceable> [, ...] ) | * } @@ -268,7 +268,11 @@ COPY { <replaceable class="parameter">table_name</replaceable> [ ( <replaceable <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, and on input, the first line is ignored. + names from the table. On input, the first line is discarded when set + to <literal>true</literal> or required to match the column names if set + to <literal>match</literal>. If the number of columns in the header is + not correct, their order differs from the one expected, or the name or + case do not match, the copy will be aborted with an error. This option is allowed only when using <literal>CSV</literal> or <literal>text</literal> format. </para> diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c index a21508a974..cde6582f1a 100644 --- a/src/backend/commands/copy.c +++ b/src/backend/commands/copy.c @@ -95,6 +95,16 @@ typedef enum CopyInsertMethod CIM_MULTI_CONDITIONAL /* use table_multi_insert only if valid */ } CopyInsertMethod; +/* + * Represents whether the header must be absent, present or present and match. + */ +typedef enum CopyHeader +{ + COPY_HEADER_ABSENT, + COPY_HEADER_PRESENT, + COPY_HEADER_MATCH +} CopyHeader; + /* * This struct contains all the state variables used throughout a COPY * operation. For simplicity, we use the same struct for all variants of COPY, @@ -136,7 +146,7 @@ typedef struct CopyStateData bool binary; /* binary format? */ bool freeze; /* freeze rows on loading? */ bool csv_mode; /* Comma Separated Value format? */ - bool header_line; /* CSV or text header line? */ + CopyHeader header_line; /* CSV or text header line? */ char *null_print; /* NULL marker string (server encoding!) */ int null_print_len; /* length of same */ char *null_print_client; /* same converted to file encoding */ @@ -1182,7 +1192,28 @@ ProcessCopyOptions(ParseState *pstate, (errcode(ERRCODE_SYNTAX_ERROR), errmsg("conflicting or redundant options"), parser_errposition(pstate, defel->location))); - cstate->header_line = defGetBoolean(defel); + + PG_TRY(); + { + if (defGetBoolean(defel)) + cstate->header_line = COPY_HEADER_PRESENT; + else + cstate->header_line = COPY_HEADER_ABSENT; + } + PG_CATCH(); + { + if (!cstate->is_copy_from) + PG_RE_THROW(); + + char *sval = defGetString(defel); + if (pg_strcasecmp(sval, "match") == 0) + cstate->header_line = COPY_HEADER_MATCH; + else + ereport(ERROR, + (errcode(ERRCODE_SYNTAX_ERROR), + errmsg("header requires a boolean or \"match\""))); + } + PG_END_TRY(); } else if (strcmp(defel->defname, "quote") == 0) { @@ -2101,7 +2132,7 @@ CopyTo(CopyState cstate) if (cstate->csv_mode) CopyAttributeOutCSV(cstate, colname, false, - list_length(cstate->attnumlist) == 1); + list_length(cstate->attnumlist) == 1); else CopyAttributeOutText(cstate, colname); } @@ -3599,12 +3630,53 @@ NextCopyFromRawFields(CopyState cstate, char ***fields, int *nfields) /* only available for text or csv input */ Assert(!cstate->binary); - /* on input just throw the header line away */ + /* on input check that the header line is correct if needed */ if (cstate->cur_lineno == 0 && cstate->header_line) { + ListCell *cur; + TupleDesc tupDesc; + + tupDesc = RelationGetDescr(cstate->rel); + cstate->cur_lineno++; - if (CopyReadLine(cstate)) - return false; /* done */ + done = CopyReadLine(cstate); + + if (cstate->header_line == COPY_HEADER_MATCH) + { + if (cstate->csv_mode) + fldct = CopyReadAttributesCSV(cstate); + else + fldct = CopyReadAttributesText(cstate); + + if (fldct < list_length(cstate->attnumlist)) + ereport(ERROR, + (errcode(ERRCODE_BAD_COPY_FILE_FORMAT), + errmsg("missing header"))); + else if (fldct > list_length(cstate->attnumlist)) + ereport(ERROR, + (errcode(ERRCODE_BAD_COPY_FILE_FORMAT), + errmsg("extra data after last expected header"))); + + foreach(cur, cstate->attnumlist) + { + int attnum = lfirst_int(cur); + char *colName = cstate->raw_fields[attnum - 1]; + Form_pg_attribute attr = TupleDescAttr(tupDesc, attnum - 1); + + if (colName == NULL) + colName = cstate->null_print; + + if (namestrcmp(&attr->attname, colName) != 0) { + ereport(ERROR, + (errcode(ERRCODE_BAD_COPY_FILE_FORMAT), + errmsg("wrong header for column \"%s\": got \"%s\"", + NameStr(attr->attname), colName))); + } + } + } + + if (done) + return false; } cstate->cur_lineno++; diff --git a/src/test/regress/input/copy.source b/src/test/regress/input/copy.source index 2368649111..4d21c7d524 100644 --- a/src/test/regress/input/copy.source +++ b/src/test/regress/input/copy.source @@ -213,3 +213,28 @@ select * from parted_copytest where b = 1; select * from parted_copytest where b = 2; drop table parted_copytest; + +-- Test header matching feature +create table header_copytest ( + a int, + b int, + c text +); +copy header_copytest from stdin with (header wrong_choice); +copy header_copytest from stdin with (header match); +a b c +1 2 foo +\. +copy header_copytest from stdin with (header match); +a b +1 2 +\. +copy header_copytest from stdin with (header match); +a b c d +1 2 foo bar +\. +copy header_copytest from stdin with (header match, format csv); +a,b,c +1,2,foo +\. +drop table header_copytest; diff --git a/src/test/regress/output/copy.source b/src/test/regress/output/copy.source index c1f7f99747..b792181fe3 100644 --- a/src/test/regress/output/copy.source +++ b/src/test/regress/output/copy.source @@ -173,3 +173,20 @@ select * from parted_copytest where b = 2; (1 row) drop table parted_copytest; +-- Test header matching feature +create table header_copytest ( + a int, + b int, + c text +); +copy header_copytest from stdin with (header wrong_choice); +ERROR: header requires a boolean or "match" +copy header_copytest from stdin with (header match); +copy header_copytest from stdin with (header match); +ERROR: missing header +CONTEXT: COPY header_copytest, line 1: "a b" +copy header_copytest from stdin with (header match); +ERROR: extra data after last expected header +CONTEXT: COPY header_copytest, line 1: "a b c d" +copy header_copytest from stdin with (header match, format csv); +drop table header_copytest; --------------2.27.0-- ^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: Add header support to text format and matching feature @ 2020-07-18 11:23 vignesh C <[email protected]> parent: Rémi Lapeyre <[email protected]> 0 siblings, 1 reply; 15+ messages in thread From: vignesh C @ 2020-07-18 11:23 UTC (permalink / raw) To: Rémi Lapeyre <[email protected]>; +Cc: pgsql-hackers On Fri, Jul 17, 2020 at 10:18 PM Rémi Lapeyre <[email protected]> wrote: > > > > > I don't know how to do that with git-send-email, but you can certainly do it easy with git-format-patch and just attach them using your regular MUA. > > > > (and while the cfbot and the archives have no problems dealing with the change in subject, it does break threading in some other MUAs, so I would recommend not doing that and sticking to the existing subject of the thread) > > > > Thanks, here are both patches attached so cfbot can read them. Few comments: Few tests are failing because of hardcoded path: +-- test header matching +CREATE FOREIGN TABLE header_match ("1" int, foo text) SERVER file_server +OPTIONS (format 'csv', filename '/Users/remi/src/postgresql/contrib/file_fdw/data/list1.csv', delimiter ',', header 'match'); +CREATE FOREIGN TABLE header_dont_match (a int, foo text) SERVER file_server +OPTIONS (format 'csv', filename '/Users/remi/src/postgresql/contrib/file_fdw/data/list1.csv', delimiter ',', header 'match'); -- ERROR Generally path is not specified like this. file_fdw test of make check-world is failing because of this. There is one warning present in the changes: copy.c: In function ‘ProcessCopyOptions’: copy.c:1208:5: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement] char *sval = defGetString(defel); There is space before tab in indent in the below code, check for git diff --check. + if (fldct < list_length(cstate->attnumlist)) + ereport(ERROR, + (errcode(ERRCODE_BAD_COPY_FILE_FORMAT), + errmsg("missing header"))); Regards, Vignesh EnterpriseDB: http://www.enterprisedb.com ^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: Add header support to text format and matching feature @ 2020-07-18 15:25 Rémi Lapeyre <[email protected]> parent: vignesh C <[email protected]> 0 siblings, 1 reply; 15+ messages in thread From: Rémi Lapeyre @ 2020-07-18 15:25 UTC (permalink / raw) To: vignesh C <[email protected]>; +Cc: pgsql-hackers Thanks for the feedback, > There is one warning present in the changes: > copy.c: In function ‘ProcessCopyOptions’: > copy.c:1208:5: warning: ISO C90 forbids mixed declarations and code > [-Wdeclaration-after-statement] > char *sval = defGetString(defel); > Weirdly this is not caught by clang, even with -Wdeclaration-after-statement. Here’s a new version that fix all the issues. Rémi Attachments: [application/octet-stream] v4-0001-Add-header-support-to-COPY-TO-text-format.patch (7.0K, ../../[email protected]/2-v4-0001-Add-header-support-to-COPY-TO-text-format.patch) download | inline diff: From 99fa3b6d623105c5eea6fe14b2dc7287663fe8fb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9mi=20Lapeyre?= <[email protected]> Date: Fri, 17 Jul 2020 01:50:06 +0200 Subject: [PATCH v4 1/2] Add header support to "COPY TO" text format MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="------------2.27.0" This is a multi-part message in MIME format. --------------2.27.0 Content-Type: text/plain; charset=UTF-8; format=fixed Content-Transfer-Encoding: 8bit CSV format supports the HEADER option to output a header in the output, it is convenient when other programs need to consume the output. This patch adds the same option to the default text format. Discussion: https://www.postgresql.org/message-id/flat/CAF1-J-0PtCWMeLtswwGV2M70U26n4g33gpe1rcKQqe6wVQDrFA@mail.gmail.com --- contrib/file_fdw/input/file_fdw.source | 1 - contrib/file_fdw/output/file_fdw.source | 4 +--- doc/src/sgml/ref/copy.sgml | 3 ++- src/backend/commands/copy.c | 11 +++++++---- src/test/regress/input/copy.source | 12 ++++++++++++ src/test/regress/output/copy.source | 8 ++++++++ 6 files changed, 30 insertions(+), 9 deletions(-) --------------2.27.0 Content-Type: text/x-patch; name="v4-0001-Add-header-support-to-COPY-TO-text-format.patch" Content-Transfer-Encoding: 8bit Content-Disposition: attachment; filename="v4-0001-Add-header-support-to-COPY-TO-text-format.patch" diff --git a/contrib/file_fdw/input/file_fdw.source b/contrib/file_fdw/input/file_fdw.source index 45b728eeb3..83edb71077 100644 --- a/contrib/file_fdw/input/file_fdw.source +++ b/contrib/file_fdw/input/file_fdw.source @@ -37,7 +37,6 @@ CREATE USER MAPPING FOR regress_no_priv_user SERVER file_server; -- validator tests CREATE FOREIGN TABLE tbl () SERVER file_server OPTIONS (format 'xml'); -- ERROR -CREATE FOREIGN TABLE tbl () SERVER file_server OPTIONS (format 'text', header 'true'); -- ERROR CREATE FOREIGN TABLE tbl () SERVER file_server OPTIONS (format 'text', quote ':'); -- ERROR CREATE FOREIGN TABLE tbl () SERVER file_server OPTIONS (format 'text', escape ':'); -- ERROR CREATE FOREIGN TABLE tbl () SERVER file_server OPTIONS (format 'binary', header 'true'); -- ERROR diff --git a/contrib/file_fdw/output/file_fdw.source b/contrib/file_fdw/output/file_fdw.source index 52b4d5f1df..547b81fd16 100644 --- a/contrib/file_fdw/output/file_fdw.source +++ b/contrib/file_fdw/output/file_fdw.source @@ -33,14 +33,12 @@ CREATE USER MAPPING FOR regress_no_priv_user SERVER file_server; -- validator tests CREATE FOREIGN TABLE tbl () SERVER file_server OPTIONS (format 'xml'); -- ERROR ERROR: COPY format "xml" not recognized -CREATE FOREIGN TABLE tbl () SERVER file_server OPTIONS (format 'text', header 'true'); -- ERROR -ERROR: COPY HEADER available only in CSV mode CREATE FOREIGN TABLE tbl () SERVER file_server OPTIONS (format 'text', quote ':'); -- ERROR ERROR: COPY quote available only in CSV mode CREATE FOREIGN TABLE tbl () SERVER file_server OPTIONS (format 'text', escape ':'); -- ERROR ERROR: COPY escape available only in CSV mode CREATE FOREIGN TABLE tbl () SERVER file_server OPTIONS (format 'binary', header 'true'); -- ERROR -ERROR: COPY HEADER available only in CSV mode +ERROR: COPY HEADER available only in CSV and text mode CREATE FOREIGN TABLE tbl () SERVER file_server OPTIONS (format 'binary', quote ':'); -- ERROR ERROR: COPY quote available only in CSV mode CREATE FOREIGN TABLE tbl () SERVER file_server OPTIONS (format 'binary', escape ':'); -- ERROR diff --git a/doc/src/sgml/ref/copy.sgml b/doc/src/sgml/ref/copy.sgml index 18189abc6c..c628a69c57 100644 --- a/doc/src/sgml/ref/copy.sgml +++ b/doc/src/sgml/ref/copy.sgml @@ -269,7 +269,8 @@ COPY { <replaceable class="parameter">table_name</replaceable> [ ( <replaceable 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, and on input, the first line is ignored. - This option is allowed only when using <literal>CSV</literal> format. + This option is allowed only when using <literal>CSV</literal> or + <literal>text</literal> format. </para> </listitem> </varlistentry> diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c index 44da71c4cb..a21508a974 100644 --- a/src/backend/commands/copy.c +++ b/src/backend/commands/copy.c @@ -136,7 +136,7 @@ typedef struct CopyStateData bool binary; /* binary format? */ bool freeze; /* freeze rows on loading? */ bool csv_mode; /* Comma Separated Value format? */ - bool header_line; /* CSV header line? */ + bool header_line; /* CSV or text header line? */ char *null_print; /* NULL marker string (server encoding!) */ int null_print_len; /* length of same */ char *null_print_client; /* same converted to file encoding */ @@ -1363,10 +1363,10 @@ ProcessCopyOptions(ParseState *pstate, errmsg("COPY delimiter cannot be \"%s\"", cstate->delim))); /* Check header */ - if (!cstate->csv_mode && cstate->header_line) + if (cstate->binary && cstate->header_line) ereport(ERROR, (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), - errmsg("COPY HEADER available only in CSV mode"))); + errmsg("COPY HEADER available only in CSV and text mode"))); /* Check quote */ if (!cstate->csv_mode && cstate->quote != NULL) @@ -2099,8 +2099,11 @@ CopyTo(CopyState cstate) colname = NameStr(TupleDescAttr(tupDesc, attnum - 1)->attname); - CopyAttributeOutCSV(cstate, colname, false, + if (cstate->csv_mode) + CopyAttributeOutCSV(cstate, colname, false, list_length(cstate->attnumlist) == 1); + else + CopyAttributeOutText(cstate, colname); } CopySendEndOfRow(cstate); diff --git a/src/test/regress/input/copy.source b/src/test/regress/input/copy.source index a1d529ad36..2368649111 100644 --- a/src/test/regress/input/copy.source +++ b/src/test/regress/input/copy.source @@ -134,6 +134,18 @@ this is just a line full of junk that would error out if parsed copy copytest3 to stdout csv header; +create temp table copytest4 ( + c1 int, + "col with tabulation: " text); + +copy copytest4 from stdin (header); +this is just a line full of junk that would error out if parsed +1 a +2 b +\. + +copy copytest4 to stdout (header); + -- test copy from with a partitioned table create table parted_copytest ( a int, diff --git a/src/test/regress/output/copy.source b/src/test/regress/output/copy.source index 938d3551da..c1f7f99747 100644 --- a/src/test/regress/output/copy.source +++ b/src/test/regress/output/copy.source @@ -95,6 +95,14 @@ copy copytest3 to stdout csv header; c1,"col with , comma","col with "" quote" 1,a,1 2,b,2 +create temp table copytest4 ( + c1 int, + "col with tabulation: " text); +copy copytest4 from stdin (header); +copy copytest4 to stdout (header); +c1 col with tabulation: \t +1 a +2 b -- test copy from with a partitioned table create table parted_copytest ( a int, --------------2.27.0-- [application/octet-stream] v4-0002-Add-header-matching-mode-to-COPY-FROM.patch (11.4K, ../../[email protected]/3-v4-0002-Add-header-matching-mode-to-COPY-FROM.patch) download | inline diff: From 9f931f133e4d92906c15ff5dc257384c673d9159 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9mi=20Lapeyre?= <[email protected]> Date: Fri, 17 Jul 2020 02:04:55 +0200 Subject: [PATCH v4 2/2] Add header matching mode to "COPY FROM" MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="------------2.27.0" This is a multi-part message in MIME format. --------------2.27.0 Content-Type: text/plain; charset=UTF-8; format=fixed Content-Transfer-Encoding: 8bit COPY FROM supports the HEADER option to silently discard the header from a CSV or text file. It is possible to load by mistake a file that matches the expected format, for example if two text columns have been swapped, resulting in garbage in the database. This option adds the possibility to actually check the header to make sure it matches what is expected and exit immediatly if it does not. Discussion: https://www.postgresql.org/message-id/flat/CAF1-J-0PtCWMeLtswwGV2M70U26n4g33gpe1rcKQqe6wVQDrFA@mail.gmail.com --- contrib/file_fdw/input/file_fdw.source | 6 ++ contrib/file_fdw/output/file_fdw.source | 9 ++- doc/src/sgml/ref/copy.sgml | 8 ++- src/backend/commands/copy.c | 85 +++++++++++++++++++++++-- src/test/regress/input/copy.source | 25 ++++++++ src/test/regress/output/copy.source | 17 +++++ 6 files changed, 141 insertions(+), 9 deletions(-) --------------2.27.0 Content-Type: text/x-patch; name="v4-0002-Add-header-matching-mode-to-COPY-FROM.patch" Content-Transfer-Encoding: 8bit Content-Disposition: attachment; filename="v4-0002-Add-header-matching-mode-to-COPY-FROM.patch" diff --git a/contrib/file_fdw/input/file_fdw.source b/contrib/file_fdw/input/file_fdw.source index 83edb71077..7a3983c785 100644 --- a/contrib/file_fdw/input/file_fdw.source +++ b/contrib/file_fdw/input/file_fdw.source @@ -79,6 +79,12 @@ CREATE FOREIGN TABLE agg_bad ( OPTIONS (format 'csv', filename '@abs_srcdir@/data/agg.bad', header 'true', delimiter ';', quote '@', escape '"', null ''); ALTER FOREIGN TABLE agg_bad ADD CHECK (a >= 0); +-- test header matching +CREATE FOREIGN TABLE header_match ("1" int, foo text) SERVER file_server +OPTIONS (format 'csv', filename '@abs_srcdir@/data/list1.csv', delimiter ',', header 'match'); +CREATE FOREIGN TABLE header_dont_match (a int, foo text) SERVER file_server +OPTIONS (format 'csv', filename '@abs_srcdir@/data/list1.csv', delimiter ',', header 'match'); -- ERROR + -- per-column options tests CREATE FOREIGN TABLE text_csv ( word1 text OPTIONS (force_not_null 'true'), diff --git a/contrib/file_fdw/output/file_fdw.source b/contrib/file_fdw/output/file_fdw.source index 547b81fd16..ebe826b9f4 100644 --- a/contrib/file_fdw/output/file_fdw.source +++ b/contrib/file_fdw/output/file_fdw.source @@ -93,6 +93,11 @@ CREATE FOREIGN TABLE agg_bad ( ) SERVER file_server OPTIONS (format 'csv', filename '@abs_srcdir@/data/agg.bad', header 'true', delimiter ';', quote '@', escape '"', null ''); ALTER FOREIGN TABLE agg_bad ADD CHECK (a >= 0); +-- test header matching +CREATE FOREIGN TABLE header_match ("1" int, foo text) SERVER file_server +OPTIONS (format 'csv', filename '@abs_srcdir@/data/list1.csv', delimiter ',', header 'match'); +CREATE FOREIGN TABLE header_dont_match (a int, foo text) SERVER file_server +OPTIONS (format 'csv', filename '@abs_srcdir@/data/list1.csv', delimiter ',', header 'match'); -- ERROR -- per-column options tests CREATE FOREIGN TABLE text_csv ( word1 text OPTIONS (force_not_null 'true'), @@ -439,12 +444,14 @@ SET ROLE regress_file_fdw_superuser; -- cleanup RESET ROLE; DROP EXTENSION file_fdw CASCADE; -NOTICE: drop cascades to 7 other objects +NOTICE: drop cascades to 9 other objects DETAIL: drop cascades to server file_server drop cascades to user mapping for regress_file_fdw_superuser on server file_server drop cascades to user mapping for regress_no_priv_user on server file_server drop cascades to foreign table agg_text drop cascades to foreign table agg_csv drop cascades to foreign table agg_bad +drop cascades to foreign table header_match +drop cascades to foreign table header_dont_match drop cascades to foreign table text_csv DROP ROLE regress_file_fdw_superuser, regress_file_fdw_user, regress_no_priv_user; diff --git a/doc/src/sgml/ref/copy.sgml b/doc/src/sgml/ref/copy.sgml index c628a69c57..c35914511f 100644 --- a/doc/src/sgml/ref/copy.sgml +++ b/doc/src/sgml/ref/copy.sgml @@ -36,7 +36,7 @@ COPY { <replaceable class="parameter">table_name</replaceable> [ ( <replaceable FREEZE [ <replaceable class="parameter">boolean</replaceable> ] DELIMITER '<replaceable class="parameter">delimiter_character</replaceable>' NULL '<replaceable class="parameter">null_string</replaceable>' - HEADER [ <replaceable class="parameter">boolean</replaceable> ] + HEADER { <literal>match</literal> | <literal>true</literal> | <literal>false</literal> } QUOTE '<replaceable class="parameter">quote_character</replaceable>' ESCAPE '<replaceable class="parameter">escape_character</replaceable>' FORCE_QUOTE { ( <replaceable class="parameter">column_name</replaceable> [, ...] ) | * } @@ -268,7 +268,11 @@ COPY { <replaceable class="parameter">table_name</replaceable> [ ( <replaceable <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, and on input, the first line is ignored. + names from the table. On input, the first line is discarded when set + to <literal>true</literal> or required to match the column names if set + to <literal>match</literal>. If the number of columns in the header is + not correct, their order differs from the one expected, or the name or + case do not match, the copy will be aborted with an error. This option is allowed only when using <literal>CSV</literal> or <literal>text</literal> format. </para> diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c index a21508a974..d58fa0cdd8 100644 --- a/src/backend/commands/copy.c +++ b/src/backend/commands/copy.c @@ -95,6 +95,16 @@ typedef enum CopyInsertMethod CIM_MULTI_CONDITIONAL /* use table_multi_insert only if valid */ } CopyInsertMethod; +/* + * Represents whether the header must be absent, present or present and match. + */ +typedef enum CopyHeader +{ + COPY_HEADER_ABSENT, + COPY_HEADER_PRESENT, + COPY_HEADER_MATCH +} CopyHeader; + /* * This struct contains all the state variables used throughout a COPY * operation. For simplicity, we use the same struct for all variants of COPY, @@ -136,7 +146,7 @@ typedef struct CopyStateData bool binary; /* binary format? */ bool freeze; /* freeze rows on loading? */ bool csv_mode; /* Comma Separated Value format? */ - bool header_line; /* CSV or text header line? */ + CopyHeader header_line; /* CSV or text header line? */ char *null_print; /* NULL marker string (server encoding!) */ int null_print_len; /* length of same */ char *null_print_client; /* same converted to file encoding */ @@ -1182,7 +1192,29 @@ ProcessCopyOptions(ParseState *pstate, (errcode(ERRCODE_SYNTAX_ERROR), errmsg("conflicting or redundant options"), parser_errposition(pstate, defel->location))); - cstate->header_line = defGetBoolean(defel); + + PG_TRY(); + { + if (defGetBoolean(defel)) + cstate->header_line = COPY_HEADER_PRESENT; + else + cstate->header_line = COPY_HEADER_ABSENT; + } + PG_CATCH(); + { + char *sval = defGetString(defel); + + if (!cstate->is_copy_from) + PG_RE_THROW(); + + if (pg_strcasecmp(sval, "match") == 0) + cstate->header_line = COPY_HEADER_MATCH; + else + ereport(ERROR, + (errcode(ERRCODE_SYNTAX_ERROR), + errmsg("header requires a boolean or \"match\""))); + } + PG_END_TRY(); } else if (strcmp(defel->defname, "quote") == 0) { @@ -2101,7 +2133,7 @@ CopyTo(CopyState cstate) if (cstate->csv_mode) CopyAttributeOutCSV(cstate, colname, false, - list_length(cstate->attnumlist) == 1); + list_length(cstate->attnumlist) == 1); else CopyAttributeOutText(cstate, colname); } @@ -3599,12 +3631,53 @@ NextCopyFromRawFields(CopyState cstate, char ***fields, int *nfields) /* only available for text or csv input */ Assert(!cstate->binary); - /* on input just throw the header line away */ + /* on input check that the header line is correct if needed */ if (cstate->cur_lineno == 0 && cstate->header_line) { + ListCell *cur; + TupleDesc tupDesc; + + tupDesc = RelationGetDescr(cstate->rel); + cstate->cur_lineno++; - if (CopyReadLine(cstate)) - return false; /* done */ + done = CopyReadLine(cstate); + + if (cstate->header_line == COPY_HEADER_MATCH) + { + if (cstate->csv_mode) + fldct = CopyReadAttributesCSV(cstate); + else + fldct = CopyReadAttributesText(cstate); + + if (fldct < list_length(cstate->attnumlist)) + ereport(ERROR, + (errcode(ERRCODE_BAD_COPY_FILE_FORMAT), + errmsg("missing header"))); + else if (fldct > list_length(cstate->attnumlist)) + ereport(ERROR, + (errcode(ERRCODE_BAD_COPY_FILE_FORMAT), + errmsg("extra data after last expected header"))); + + foreach(cur, cstate->attnumlist) + { + int attnum = lfirst_int(cur); + char *colName = cstate->raw_fields[attnum - 1]; + Form_pg_attribute attr = TupleDescAttr(tupDesc, attnum - 1); + + if (colName == NULL) + colName = cstate->null_print; + + if (namestrcmp(&attr->attname, colName) != 0) { + ereport(ERROR, + (errcode(ERRCODE_BAD_COPY_FILE_FORMAT), + errmsg("wrong header for column \"%s\": got \"%s\"", + NameStr(attr->attname), colName))); + } + } + } + + if (done) + return false; } cstate->cur_lineno++; diff --git a/src/test/regress/input/copy.source b/src/test/regress/input/copy.source index 2368649111..4d21c7d524 100644 --- a/src/test/regress/input/copy.source +++ b/src/test/regress/input/copy.source @@ -213,3 +213,28 @@ select * from parted_copytest where b = 1; select * from parted_copytest where b = 2; drop table parted_copytest; + +-- Test header matching feature +create table header_copytest ( + a int, + b int, + c text +); +copy header_copytest from stdin with (header wrong_choice); +copy header_copytest from stdin with (header match); +a b c +1 2 foo +\. +copy header_copytest from stdin with (header match); +a b +1 2 +\. +copy header_copytest from stdin with (header match); +a b c d +1 2 foo bar +\. +copy header_copytest from stdin with (header match, format csv); +a,b,c +1,2,foo +\. +drop table header_copytest; diff --git a/src/test/regress/output/copy.source b/src/test/regress/output/copy.source index c1f7f99747..b792181fe3 100644 --- a/src/test/regress/output/copy.source +++ b/src/test/regress/output/copy.source @@ -173,3 +173,20 @@ select * from parted_copytest where b = 2; (1 row) drop table parted_copytest; +-- Test header matching feature +create table header_copytest ( + a int, + b int, + c text +); +copy header_copytest from stdin with (header wrong_choice); +ERROR: header requires a boolean or "match" +copy header_copytest from stdin with (header match); +copy header_copytest from stdin with (header match); +ERROR: missing header +CONTEXT: COPY header_copytest, line 1: "a b" +copy header_copytest from stdin with (header match); +ERROR: extra data after last expected header +CONTEXT: COPY header_copytest, line 1: "a b c d" +copy header_copytest from stdin with (header match, format csv); +drop table header_copytest; --------------2.27.0-- ^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: Add header support to text format and matching feature @ 2020-07-25 13:34 Daniel Verite <[email protected]> parent: Rémi Lapeyre <[email protected]> 0 siblings, 0 replies; 15+ messages in thread From: Daniel Verite @ 2020-07-25 13:34 UTC (permalink / raw) To: =?ISO-8859-15?Q? R=E9mi?= Lapeyre <[email protected]>; +Cc: vignesh C <[email protected]>; pgsql-hackers Rémi Lapeyre wrote: > Here’s a new version that fix all the issues. Here's a review of v4. The patch improves COPY in two ways: - COPY TO and COPY FROM now accept "HEADER ON" for the TEXT format (previously it was only for CSV) - COPY FROM also accepts "HEADER match" to tell that there's a header and that its contents must match the columns of the destination table. This works for both the CSV and TEXT formats. The syntax for the columns is the same as for the data and the match is case-sensitive. The first improvement when submitted alone (in 2018 by Simon Muller) has been judged not useful enough or even hazardous without any "match" feature. It was returned with feedback in 2018-10 and resubmitted by Rémi in 2020-02 with the match feature. The patches apply cleanly, "make check" and "make check-world" pass. In my tests it works fine except for one crash that I can reproduce on a fresh build and default configuration with: $ cat >file.txt i 1 $ psql postgres=# create table x(i int); CREATE TABLE postgres=# \copy x(i) from file.txt with (header match) COPY 1 postgres=# \copy x(i) from file.txt with (header match) COPY 1 postgres=# \copy x(i) from file.txt with (header match) COPY 1 postgres=# \copy x(i) from file.txt with (header match) COPY 1 postgres=# \copy x(i) from file.txt with (header match) COPY 1 postgres=# \copy x(i) from file.txt with (header match) PANIC: ERRORDATA_STACK_SIZE exceeded server closed the connection unexpectedly This probably means the server terminated abnormally before or while processing the request. The connection to the server was lost. Attempting reset: Failed. I suspect the reason is the way that PG_TRY/PG_CATCH is used, see below. Code comments: +/* + * Represents whether the header must be absent, present or present and match. + */ +typedef enum CopyHeader +{ + COPY_HEADER_ABSENT, + COPY_HEADER_PRESENT, + COPY_HEADER_MATCH +} CopyHeader; + /* * This struct contains all the state variables used throughout a COPY * operation. For simplicity, we use the same struct for all variants of COPY, @@ -136,7 +146,7 @@ typedef struct CopyStateData bool binary; /* binary format? */ bool freeze; /* freeze rows on loading? */ bool csv_mode; /* Comma Separated Value format? */ - bool header_line; /* CSV or text header line? */ + CopyHeader header_line; /* CSV or text header line? */ After the redefinition into this enum type, there are still a bunch of references to header_line that treat it like a boolean: 1190: if (cstate->header_line) 1398: if (cstate->binary && cstate->header_line) 2119: if (cstate->header_line) 3635: if (cstate->cur_lineno == 0 && cstate->header_line) It works fine since COPY_HEADER_ABSENT is 0 as the first value of the enum, but maybe it's not good style to count on that. + PG_TRY(); + { + if (defGetBoolean(defel)) + cstate->header_line = COPY_HEADER_PRESENT; + else + cstate->header_line = COPY_HEADER_ABSENT; + } + PG_CATCH(); + { + char *sval = defGetString(defel); + + if (!cstate->is_copy_from) + PG_RE_THROW(); + + if (pg_strcasecmp(sval, "match") == 0) + cstate->header_line = COPY_HEADER_MATCH; + else + ereport(ERROR, + (errcode(ERRCODE_SYNTAX_ERROR), + errmsg("header requires a boolean or \"match\""))); + } + PG_END_TRY(); It seems wrong to use a PG_CATCH block for this. I understand that it's because defGetBoolean() calls ereport() on non-booleans, but then it should be split into an error-throwing function and a non-error-throwing lexical analysis of the boolean, the above code calling the latter. Besides the comments in elog.h above PG_TRY say that "the error recovery code can either do PG_RE_THROW to propagate the error outwards, or do a (sub)transaction abort. Failure to do so may leave the system in an inconsistent state for further processing." Maybe this is what happens with the repeated uses of "match" eventually failing with ERRORDATA_STACK_SIZE exceeded. - HEADER [ <replaceable class="parameter">boolean</replaceable> ] + HEADER { <literal>match</literal> | <literal>true</literal> | <literal>false</literal> } This should be enclosed in square brackets because HEADER with no argument is still accepted. + names from the table. On input, the first line is discarded when set + to <literal>true</literal> or required to match the column names if set The elision of "header" as the subject might be misinterpreted as if it's the first line that is true. I'd suggest "when <literal>header>/literal> is set to ..." to avoid any confusion. Best regards, -- Daniel Vérité PostgreSQL-powered mailer: https://www.manitou-mail.org Twitter: @DanielVerite ^ permalink raw reply [nested|flat] 15+ messages in thread
end of thread, other threads:[~2020-07-25 13:34 UTC | newest] Thread overview: 15+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2020-03-01 23:45 Re: [PATCH v1] Allow COPY "text" to output a header and add header matching mode to COPY FROM Rémi Lapeyre <[email protected]> 2020-03-02 05:39 ` Surafel Temesgen <[email protected]> 2020-07-01 09:04 ` Daniel Gustafsson <[email protected]> 2020-07-08 13:21 ` Daniel Gustafsson <[email protected]> 2020-07-08 13:51 ` Rémi Lapeyre <[email protected]> 2020-07-08 14:10 ` Justin Pryzby <[email protected]> 2020-07-10 11:34 ` Peter Eisentraut <[email protected]> 2020-07-17 14:54 ` Add header support to text format and matching feature Rémi Lapeyre <[email protected]> 2020-07-17 14:54 ` [PATCH v3 1/2] Add header support to "COPY TO" text format Rémi Lapeyre <[email protected]> 2020-07-17 14:54 ` [PATCH v3 2/2] Add header matching mode to "COPY FROM" Rémi Lapeyre <[email protected]> 2020-07-17 15:27 ` Re: Add header support to text format and matching feature Magnus Hagander <[email protected]> 2020-07-17 16:48 ` Re: Add header support to text format and matching feature Rémi Lapeyre <[email protected]> 2020-07-18 11:23 ` Re: Add header support to text format and matching feature vignesh C <[email protected]> 2020-07-18 15:25 ` Re: Add header support to text format and matching feature Rémi Lapeyre <[email protected]> 2020-07-25 13:34 ` Re: Add header support to text format and matching feature Daniel Verite <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox