From 47573825b3c39d5f0ce6037cb1c17b6344c8dc0a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9mi=20Lapeyre?= Date: Tue, 13 Oct 2020 14:45:56 +0200 Subject: [PATCH v10 2/2] Add header matching mode to "COPY FROM" MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="------------2.31.1" This is a multi-part message in MIME format. --------------2.31.1 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 | 12 +++-- src/backend/commands/copy.c | 65 +++++++++++++++++++++++-- src/backend/commands/copyfromparse.c | 52 ++++++++++++++++++-- src/backend/commands/copyto.c | 2 +- src/include/commands/copy.h | 12 ++++- src/test/regress/input/copy.source | 25 ++++++++++ src/test/regress/output/copy.source | 17 +++++++ 9 files changed, 186 insertions(+), 14 deletions(-) --------------2.31.1 Content-Type: text/x-patch; name="v10-0002-Add-header-matching-mode-to-COPY-FROM.patch" Content-Transfer-Encoding: 8bit Content-Disposition: attachment; filename="v10-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..cb15cbb418 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_doesnt_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..b40ce99a40 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_doesnt_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_doesnt_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 e6e95c9b4c..739a25a6ce 100644 --- a/doc/src/sgml/ref/copy.sgml +++ b/doc/src/sgml/ref/copy.sgml @@ -36,7 +36,7 @@ COPY { table_name [ ( boolean ] DELIMITER 'delimiter_character' NULL 'null_string' - HEADER [ boolean ] + HEADER [ match | true | false ] QUOTE 'quote_character' ESCAPE 'escape_character' FORCE_QUOTE { ( column_name [, ...] ) | * } @@ -276,9 +276,13 @@ COPY { table_name [ ( 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 CSV or - text format. + names from the table. On input, the first line is discarded when + header is set to true or required + to match the column names if set to match. If the + number of columns in the header is not correct, their order differs + from the one expected, or the name or case doesn't match, the copy will + be aborted with an error. This option is allowed only when using + CSV or text format. diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c index 1165f6b254..c13c89ddf6 100644 --- a/src/backend/commands/copy.c +++ b/src/backend/commands/copy.c @@ -315,7 +315,66 @@ DoCopy(ParseState *pstate, const CopyStmt *stmt, } /* - * Process the statement option list for COPY. +* Extract a CopyHeader value from a DefElem. +*/ +static CopyHeader +DefGetCopyHeader(DefElem *def) +{ + /* + * If no parameter given, assume "true" is meant. + */ + if (def->arg == NULL) + return COPY_HEADER_PRESENT; + + /* + * Allow 0, 1, "true", "false", "on", "off" or "match". + */ + switch (nodeTag(def->arg)) + { + case T_Integer: + switch (intVal(def->arg)) + { + case 0: + return COPY_HEADER_ABSENT; + case 1: + return COPY_HEADER_PRESENT; + default: + /* otherwise, error out below */ + break; + } + break; + default: + { + char *sval = defGetString(def); + + /* + * The set of strings accepted here should match up with the + * grammar's opt_boolean_or_string production. + */ + if (pg_strcasecmp(sval, "true") == 0) + return COPY_HEADER_PRESENT; + if (pg_strcasecmp(sval, "false") == 0) + return COPY_HEADER_ABSENT; + if (pg_strcasecmp(sval, "on") == 0) + return COPY_HEADER_PRESENT; + if (pg_strcasecmp(sval, "off") == 0) + return COPY_HEADER_ABSENT; + if (pg_strcasecmp(sval, "match") == 0) + return COPY_HEADER_MATCH; + + } + break; + } + + ereport(ERROR, + (errcode(ERRCODE_SYNTAX_ERROR), + errmsg("%s requires a boolean or \"match\"", + def->defname))); + return COPY_HEADER_ABSENT; /* keep compiler quiet */ +} + +/* +* Process the statement option list for COPY. * * Scan the options list (a list of DefElem) and transpose the information * into *opts_out, applying appropriate error checking. @@ -410,7 +469,7 @@ ProcessCopyOptions(ParseState *pstate, errmsg("conflicting or redundant options"), parser_errposition(pstate, defel->location))); header_specified = true; - opts_out->header_line = defGetBoolean(defel); + opts_out->header_line = DefGetCopyHeader(defel); } else if (strcmp(defel->defname, "quote") == 0) { @@ -591,7 +650,7 @@ ProcessCopyOptions(ParseState *pstate, errmsg("COPY delimiter cannot be \"%s\"", opts_out->delim))); /* Check header */ - if (opts_out->binary && opts_out->header_line) + if (opts_out->binary && opts_out->header_line != COPY_HEADER_ABSENT) ereport(ERROR, (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), errmsg("COPY HEADER available only in CSV and text mode"))); diff --git a/src/backend/commands/copyfromparse.c b/src/backend/commands/copyfromparse.c index 0813424768..d9ce0456f2 100644 --- a/src/backend/commands/copyfromparse.c +++ b/src/backend/commands/copyfromparse.c @@ -72,6 +72,7 @@ #include "miscadmin.h" #include "pgstat.h" #include "port/pg_bswap.h" +#include "utils/builtins.h" #include "utils/memutils.h" #include "utils/rel.h" @@ -739,12 +740,55 @@ NextCopyFromRawFields(CopyFromState cstate, char ***fields, int *nfields) /* only available for text or csv input */ Assert(!cstate->opts.binary); - /* on input just throw the header line away */ - if (cstate->cur_lineno == 0 && cstate->opts.header_line) + /* on input check that the header line is correct if needed */ + if (cstate->cur_lineno == 0 && cstate->opts.header_line != COPY_HEADER_ABSENT) { + ListCell *cur; + TupleDesc tupDesc; + + tupDesc = RelationGetDescr(cstate->rel); + cstate->cur_lineno++; - if (CopyReadLine(cstate)) - return false; /* done */ + done = CopyReadLine(cstate); + + if (cstate->opts.header_line == COPY_HEADER_MATCH) + { + if (cstate->opts.csv_mode) + fldct = CopyReadAttributesCSV(cstate); + else + fldct = CopyReadAttributesText(cstate); + + if (fldct < list_length(cstate->attnumlist)) + ereport(ERROR, + (errcode(ERRCODE_BAD_COPY_FILE_FORMAT), + errmsg("incomplete header, expected %d columns but got %d", + list_length(cstate->attnumlist), fldct))); + else if (fldct > list_length(cstate->attnumlist)) + ereport(ERROR, + (errcode(ERRCODE_BAD_COPY_FILE_FORMAT), + errmsg("extra data after last expected header, expected %d columns but got %d", + list_length(cstate->attnumlist), fldct))); + + 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->opts.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/backend/commands/copyto.c b/src/backend/commands/copyto.c index e9ae1c64a2..ed0191557c 100644 --- a/src/backend/commands/copyto.c +++ b/src/backend/commands/copyto.c @@ -854,7 +854,7 @@ DoCopyTo(CopyToState cstate) cstate->file_encoding); /* if a header has been requested send the line */ - if (cstate->opts.header_line) + if (cstate->opts.header_line != COPY_HEADER_ABSENT) { bool hdr_delim = false; diff --git a/src/include/commands/copy.h b/src/include/commands/copy.h index 095d6f0b7e..8075742664 100644 --- a/src/include/commands/copy.h +++ b/src/include/commands/copy.h @@ -19,6 +19,16 @@ #include "parser/parse_node.h" #include "tcop/dest.h" +/* + * Represents whether the header must match, be absent or be present. + */ +typedef enum CopyHeader +{ + COPY_HEADER_ABSENT, + COPY_HEADER_PRESENT, + COPY_HEADER_MATCH +} CopyHeader; + /* * A struct to hold COPY options, in a parsed form. All of these are related * to formatting, except for 'freeze', which doesn't really belong here, but @@ -32,7 +42,7 @@ typedef struct CopyFormatOptions 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 */ diff --git a/src/test/regress/input/copy.source b/src/test/regress/input/copy.source index 379e47a541..d9b4f64022 100644 --- a/src/test/regress/input/copy.source +++ b/src/test/regress/input/copy.source @@ -275,3 +275,28 @@ copy tab_progress_reporting from '@abs_srcdir@/data/emp.data' drop trigger check_after_tab_progress_reporting on tab_progress_reporting; drop function notice_after_tab_progress_reporting(); drop table tab_progress_reporting; + +-- 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 ce8309ef56..df2bea76a3 100644 --- a/src/test/regress/output/copy.source +++ b/src/test/regress/output/copy.source @@ -227,3 +227,20 @@ INFO: progress: {"type": "FILE", "command": "COPY FROM", "relname": "tab_progre drop trigger check_after_tab_progress_reporting on tab_progress_reporting; drop function notice_after_tab_progress_reporting(); drop table tab_progress_reporting; +-- 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: incomplete header, expected 3 columns but got 2 +CONTEXT: COPY header_copytest, line 1: "a b" +copy header_copytest from stdin with (header match); +ERROR: extra data after last expected header, expected 3 columns but got 4 +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.31.1--