Subject: Re: Add header support to text format and matching feature

From: Peter Eisentraut <[email-masked]>
Date: Fri, 28 Jan 2022 09:57:46 +0100

On 31.12.21 18:36, Rémi Lapeyre wrote:
> Here’s an updated version of the patch that takes into account the changes in d1029bb5a2. The actual code is the same as v10 which was already marked as ready for committer.

I have committed the 0001 patch.  I will work on the 0002 patch next.

I notice in the 0002 patch that there is no test case for the error 
"wrong header for column \"%s\": got \"%s\"", which I think is really 
the core functionality of this patch.  So please add that.

I wonder whether the header matching should be a separate option from 
the HEADER option.  The option parsing in this patch is quite 
complicated and could be simpler if there were two separate options.  It 
appears this has been mentioned in the thread but not fully discussed.

---

From: Rémi Lapeyre <[email-masked]>
Date: Sun, 30 Jan 2022 23:56:57 +0100

> On 28 Jan 2022, at 09:57, Peter Eisentraut <[email-masked]> wrote:
> 

Thanks!

> I notice in the 0002 patch that there is no test case for the error "wrong header for column \"%s\": got \"%s\"", which I think is really the core functionality of this patch.  So please add that.
> 

I added a test for it in this new version of the patch.

> I wonder whether the header matching should be a separate option from the HEADER option.  The option parsing in this patch is quite complicated and could be simpler if there were two separate options.  It appears this has been mentioned in the thread but not fully discussed.

I suppose a new option could be added but I’m not sure it would simplify things much with regard to the code and in my opinion it would be a bit weirder for users, right now it is just:

    copy my_table from stdin with (header match);

with an additional option it could be:

    copy my_table from stdin with (header true,  match);

with potentially “header true” being implicit when “match” is given:

    copy my_table from stdin with (match);

But I think we would still have to check for and return an error if the user inputs:

    copy my_table from stdin with (header off, match);

Rather than complicating things, the current implementation seemed to be the best but I will update the patch if you think I should change it.

Best regards,
Rémi

---

From: Peter Eisentraut <[email-masked]>
Date: Mon, 31 Jan 2022 07:54:01 +0100

> I added a test for it in this new version of the patch.

The file_fdw.sql tests contain this

+CREATE FOREIGN TABLE header_doesnt_match (a int, foo text) SERVER 
file_server
+OPTIONS (format 'csv', filename :'filename', delimiter ',', header 
'match');   -- ERROR

but no actual error is generated.  Please review the additions on the 
file_fdw tests to see that they make sense.

---

From: Peter Eisentraut <[email-masked]>
Date: Thu, 3 Feb 2022 07:37:44 +0100

> file_fdw tests to see that they make sense.

A few more comments on your latest patch:

- The DefGetCopyHeader() function seems very bulky and might not be 
necessary.  I think you can just check for the string "match" first and 
then use defGetBoolean() as before if it didn't match.

- If you define COPY_HEADER_ABSENT = 0 in the enum, then most of the 
existing truth checks don't need to be changed.

- In NextCopyFromRawFields(), it looks like you have code that replaces 
the null_print value if the supplied column name is null.  I don't think 
we should allow null column values.  Someone, this should be an error. 
(Do we use null_print on output and make the column name null if it 
matches?)

---

From: Greg Stark <[email-masked]>
Date: Fri, 25 Mar 2022 00:21:07 -0400

Great to see the first of the two patches committed.

It looks like the second patch got some feedback from Peter in
February and has been marked "Waiting on author" ever since.

Remi, will you have a chance to look at this this month?

Peter, are these comments blocking if Remi doesn't have a chance to
work on it should I move it to the next release or could it be fixed
up and committed?

---

From: "Daniel Verite" <[email-masked]>
Date: Fri, 25 Mar 2022 13:02:33 +0100

> then use defGetBoolean() as before if it didn't match.

The problem is that defGetBoolean() ends like this in the non-matching
case:

	ereport(ERROR,
		(errcode(ERRCODE_SYNTAX_ERROR),
		 errmsg("%s requires a Boolean value",
		def->defname)));

We don't want this error message when the string does not match
since it's really a tri-state option, not a boolean.

To avoid duplicating the code that recognizes true/false/on/off/0/1,
probably defGetBoolean()'s guts should be moved into another function
that does the matching and report to the caller instead of throwing an
error. Then DefGetCopyHeader() could call that non-throwing function.

> existing truth checks don't need to be changed.

It's already 0 by default. Not changing some truth checks does work, but
then we get some code that treat CopyFromState.header_line like
a boolean and some other code like an enum, which I found not
ideal wrt clarity in an earlier round of review [1]

It's not a major issue though, as it  concerns only 3 lines of code in the
v12
patch:
-	if (opts_out->binary && opts_out->header_line)
+	if (opts_out->binary && opts_out->header_line != COPY_HEADER_ABSENT)

+	/* on input check that the header line is correct if needed */
+	if (cstate->cur_lineno == 0 && cstate->opts.header_line !=
COPY_HEADER_ABSENT)

-		if (cstate->opts.header_line)
+		if (cstate->opts.header_line != COPY_HEADER_ABSENT)

> we should allow null column values.  Someone, this should be an error. 

+1

[1]
https://www.postgresql.org/message-id/80a9b594-01d6-4ee4-a612-9ae9fd3c1194%40manitou-mail.org

Best regards,
-- 
Daniel Vérité
https://postgresql.verite.pro/
Twitter: @DanielVerite

---

From: Peter Eisentraut <[email-masked]>
Date: Tue, 29 Mar 2022 17:02:04 +0200

> work on it should I move it to the next release or could it be fixed

I will try to finish up this patch.

---

From: Peter Eisentraut <[email-masked]>
Date: Wed, 30 Mar 2022 09:11:09 +0200

> I will try to finish up this patch.

Committed, after some further refinements as discussed.

---

From: Julien Rouhaud <[email-masked]>
Date: Tue, 7 Jun 2022 23:47:44 +0800

Hi,

> Committed, after some further refinements as discussed.

While working on nearby code, I found some problems with this feature.

First, probably nitpicking, the HEADER MATCH is allowed for COPY TO, is that
expected?  The documentation isn't really explicit about it, but there's
nothing to match when exporting data it's a bit surprising.  I'm not opposed to
have HEADER MATCH means HEADER ON for COPY TO, as as-is one can easily reuse
the commands history, but maybe it should be clearly documented?

Then, apparently HEADER MATCH doesn't let you do sanity checks against a custom
column list.  This one looks like a clear oversight, as something like that
should be entirely valid IMHO:

CREATE TABLE tbl(col1 int, col2 int);
COPY tbl (col2, col1) TO '/path/to/file' WITH (HEADER MATCH);
COPY tbl (col2, col1) FROM '/path/to/file' WITH (HEADER MATCH);

but right now it errors out with:

ERROR:  column name mismatch in header line field 1: got "col1", expected "col2"

Note that the error message is bogus if you specify attributes in a
different order from the relation, as the code is mixing access to the tuple
desc and access to the raw fields with the same offset.

This also means that it will actually fail to detect a mismatch in the provided
column list and let you import data in the wrong position as long as the
datatypes are compatible and the column header in the file are in the correct
order.  For instance:

CREATE TABLE abc (a text, b text, c text);
INSERT INTO abc SELECT 'a', 'b', 'c';
COPY abc TO '/path/to/file' WITH (HEADER MATCH);

You can then import the data with any of those:
COPY abc(c, b, a) TO '/path/to/file' WITH (HEADER MATCH);
COPY abc(c, a, b) TO '/path/to/file' WITH (HEADER MATCH);
[...]
SELECT * FROM abc;

Even worse, if you try to do a COPY ... FROM ... WITH (HEADER ON) on a table
that has some dropped attribute(s).  The current code will access random memory
as there's no exact attnum / raw field mapping anymore.

I can work on a fix if needed (with some additional regression test to cover
those cases), but I'm still not sure that having a user provided column list is
supposed to be accepted or not for the HEADER MATCH.  In the meantime I will
add an open item.

---

From: Andrew Dunstan <[email-masked]>
Date: Sun, 12 Jun 2022 09:36:13 -0400

> the commands history, but maybe it should be clearly documented?

I think it makes more sense to have a sanity check to prevent HEADER
MATCH with COPY TO.

> as there's no exact attnum / raw field mapping anymore.

Ouch! That certainly needs to be fixed.

> supposed to be accepted or not for the HEADER MATCH.  In the meantime I will

I think it should, but a temporary alternative would be to forbid HEADER
MATCH with explicit column lists until we can make it work right.

cheers

andrew

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

---

From: Julien Rouhaud <[email-masked]>
Date: Mon, 13 Jun 2022 10:32:13 +0800

Hi,

> I think it makes more sense to have a sanity check to prevent HEADER

I'm fine with it.  I added such a check and mentioned it in the documentation.

> MATCH with explicit column lists until we can make it work right.

I think it would still be problematic if the target table has dropped columns.
Fortunately, as I initially thought the problem is only due to a thinko in the
original commit which used a wrong variable for the raw_fields offset.  Once
fixed (attached v1) I didn't see any other problem in the rest of the logic and
all the added regression tests work as expected.

---

From: Peter Eisentraut <[email-masked]>
Date: Mon, 13 Jun 2022 09:35:16 +0200

> all the added regression tests work as expected.

Thanks for this patch.  I'll check it in detail in a bit.  It looks good 
to me at first glance.

---

From: Michael Paquier <[email-masked]>
Date: Mon, 13 Jun 2022 16:46:46 +0900

> I'm fine with it.  I added such a check and mentioned it in the documentation.

An error looks like the right call at this stage of the game.  I am
not sure what the combination of MATCH with COPY TO would mean,
actually.  And with the concept of SELECT queries on top of it, the
whole idea gets blurrier.

> all the added regression tests work as expected.

Interesting catch.  One thing that I've always found useful when it
comes to tests that stress dropped columns is to have tests where we
reduce the number of total columns that still exist.  An extra thing
is to look after ........pg.dropped.N........ a bit, and I would put
something in one of the headers.

>  	if (pg_strcasecmp(sval, "match") == 0)
> +	{
> +		/* match is only valid for COPY FROM */
> +		if (!is_from)
> +			ereport(ERROR,
> +				(errcode(ERRCODE_SYNTAX_ERROR),
> +			 errmsg("%s match is only valid for COPY FROM",
> +					def->defname)));

Some nits.  I would suggest to reword this error message, like "cannot
use \"match\" with HEADER in COPY TO".  There is no need for the extra
comment, and the error code should be ERRCODE_FEATURE_NOT_SUPPORTED.
--
Michael
From af4e06ca5f0a13ad5922abae446bd6716bbdf3c1 Mon Sep 17 00:00:00 2001
From: Julien Rouhaud <[email-masked]>
Date: Mon, 13 Jun 2022 09:49:23 +0800
Subject: [PATCH v2] Fix processing of header match option for COPY.

Thinko in 072132f04en which used the attnum offset to access the raw_fields
array, leading to incorrect results of crash.  Use the correct variable, and
add some regression tests to cover a bit more scenario for the HEADER MATCH
option.

While at it, disallow HEADER MATCH in COPY TO as there is no validation that
can be done in that case.

Author: Julien Rouhaud
Discussion: https://postgr.es/m/20220607154744.vvmitnqhyxrne5ms@jrouhaud
---
 src/backend/commands/copy.c          | 11 +++++--
 src/backend/commands/copyfromparse.c |  5 ++--
 src/test/regress/expected/copy.out   | 43 ++++++++++++++++++++++++++-
 src/test/regress/sql/copy.sql        | 44 ++++++++++++++++++++++++++--
 doc/src/sgml/ref/copy.sgml           |  2 ++
 5 files changed, 97 insertions(+), 8 deletions(-)

diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index f448d39c7e..e2870e3c11 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -318,7 +318,7 @@ DoCopy(ParseState *pstate, const CopyStmt *stmt,
  * defGetBoolean() but also accepts the special value "match".
  */
 static CopyHeaderChoice
-defGetCopyHeaderChoice(DefElem *def)
+defGetCopyHeaderChoice(DefElem *def, bool is_from)
 {
 	/*
 	 * If no parameter given, assume "true" is meant.
@@ -360,7 +360,14 @@ defGetCopyHeaderChoice(DefElem *def)
 				if (pg_strcasecmp(sval, "off") == 0)
 					return COPY_HEADER_FALSE;
 				if (pg_strcasecmp(sval, "match") == 0)
+				{
+					if (!is_from)
+						ereport(ERROR,
+								(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+								 errmsg("cannot use \"%s\" with HEADER in COPY TO",
+										sval)));
 					return COPY_HEADER_MATCH;
+				}
 			}
 			break;
 	}
@@ -452,7 +459,7 @@ ProcessCopyOptions(ParseState *pstate,
 			if (header_specified)
 				errorConflictingDefElem(defel, pstate);
 			header_specified = true;
-			opts_out->header_line = defGetCopyHeaderChoice(defel);
+			opts_out->header_line = defGetCopyHeaderChoice(defel, is_from);
 		}
 		else if (strcmp(defel->defname, "quote") == 0)
 		{
diff --git a/src/backend/commands/copyfromparse.c b/src/backend/commands/copyfromparse.c
index e06534943f..57813b3458 100644
--- a/src/backend/commands/copyfromparse.c
+++ b/src/backend/commands/copyfromparse.c
@@ -789,11 +789,12 @@ NextCopyFromRawFields(CopyFromState cstate, char ***fields, int *nfields)
 			foreach(cur, cstate->attnumlist)
 			{
 				int			attnum = lfirst_int(cur);
-				char	   *colName = cstate->raw_fields[attnum - 1];
+				char	   *colName;
 				Form_pg_attribute attr = TupleDescAttr(tupDesc, attnum - 1);
 
-				fldnum++;
+				Assert(fldnum < cstate->max_fields);
 
+				colName = cstate->raw_fields[fldnum++];
 				if (colName == NULL)
 					ereport(ERROR,
 							(errcode(ERRCODE_BAD_COPY_FILE_FORMAT),
diff --git a/src/test/regress/expected/copy.out b/src/test/regress/expected/copy.out
index e8d6b4fc13..7f2f4ae4ae 100644
--- a/src/test/regress/expected/copy.out
+++ b/src/test/regress/expected/copy.out
@@ -182,9 +182,21 @@ create table header_copytest (
 	b int,
 	c text
 );
+-- Make sure it works with with dropped columns
+alter table header_copytest drop column c;
+alter table header_copytest add column c text;
+copy header_copytest to stdout with (header match);
+ERROR:  cannot use "match" with HEADER in COPY TO
 copy header_copytest from stdin with (header wrong_choice);
 ERROR:  header requires a Boolean value or "match"
+-- works
 copy header_copytest from stdin with (header match);
+copy header_copytest (c, a, b) from stdin with (header match);
+copy header_copytest from stdin with (header match, format csv);
+-- errors
+copy header_copytest (c, b, a) from stdin with (header match);
+ERROR:  column name mismatch in header line field 1: got "a", expected "c"
+CONTEXT:  COPY header_copytest, line 1: "a	b	c"
 copy header_copytest from stdin with (header match);
 ERROR:  column name mismatch in header line field 3: got null value ("\N"), expected "c"
 CONTEXT:  COPY header_copytest, line 1: "a	b	\N"
@@ -197,5 +209,34 @@ CONTEXT:  COPY header_copytest, line 1: "a	b	c	d"
 copy header_copytest from stdin with (header match);
 ERROR:  column name mismatch in header line field 3: got "d", expected "c"
 CONTEXT:  COPY header_copytest, line 1: "a	b	d"
-copy header_copytest from stdin with (header match, format csv);
+SELECT * FROM header_copytest ORDER BY a;
+ a | b |  c  
+---+---+-----
+ 1 | 2 | foo
+ 3 | 4 | bar
+ 5 | 6 | baz
+(3 rows)
+
+-- Drop an extra column, in the middle of the existing set.
+alter table header_copytest drop column b;
+-- works
+copy header_copytest (c, a) from stdin with (header match);
+copy header_copytest (a, c) from stdin with (header match);
+-- errors
+copy header_copytest from stdin with (header match);
+ERROR:  wrong number of fields in header line: field count is 3, expected 2
+CONTEXT:  COPY header_copytest, line 1: "a	........pg.dropped.2........	c"
+copy header_copytest (a, c) from stdin with (header match);
+ERROR:  wrong number of fields in header line: field count is 3, expected 2
+CONTEXT:  COPY header_copytest, line 1: "a	c	b"
+SELECT * FROM header_copytest ORDER BY a;
+ a |  c  
+---+-----
+ 1 | foo
+ 3 | bar
+ 5 | baz
+ 7 | foo
+ 8 | foo
+(5 rows)
+
 drop table header_copytest;
diff --git a/src/test/regress/sql/copy.sql b/src/test/regress/sql/copy.sql
index d72d226f34..285022e07c 100644
--- a/src/test/regress/sql/copy.sql
+++ b/src/test/regress/sql/copy.sql
@@ -204,11 +204,29 @@ create table header_copytest (
 	b int,
 	c text
 );
+-- Make sure it works with with dropped columns
+alter table header_copytest drop column c;
+alter table header_copytest add column c text;
+copy header_copytest to stdout with (header match);
 copy header_copytest from stdin with (header wrong_choice);
+-- works
 copy header_copytest from stdin with (header match);
 a	b	c
 1	2	foo
 \.
+copy header_copytest (c, a, b) from stdin with (header match);
+c	a	b
+bar	3	4
+\.
+copy header_copytest from stdin with (header match, format csv);
+a,b,c
+5,6,baz
+\.
+-- errors
+copy header_copytest (c, b, a) from stdin with (header match);
+a	b	c
+1	2	foo
+\.
 copy header_copytest from stdin with (header match);
 a	b	\N
 1	2	foo
@@ -225,8 +243,28 @@ copy header_copytest from stdin with (header match);
 a	b	d
 1	2	foo
 \.
-copy header_copytest from stdin with (header match, format csv);
-a,b,c
-1,2,foo
+SELECT * FROM header_copytest ORDER BY a;
+
+-- Drop an extra column, in the middle of the existing set.
+alter table header_copytest drop column b;
+-- works
+copy header_copytest (c, a) from stdin with (header match);
+c	a
+foo	7
 \.
+copy header_copytest (a, c) from stdin with (header match);
+a	c
+8	foo
+\.
+-- errors
+copy header_copytest from stdin with (header match);
+a	........pg.dropped.2........	c
+1	2	foo
+\.
+copy header_copytest (a, c) from stdin with (header match);
+a	c	b
+1	foo	2
+\.
+
+SELECT * FROM header_copytest ORDER BY a;
 drop table header_copytest;
diff --git a/doc/src/sgml/ref/copy.sgml b/doc/src/sgml/ref/copy.sgml
index 40af423ccf..8aae711b3b 100644
--- a/doc/src/sgml/ref/copy.sgml
+++ b/doc/src/sgml/ref/copy.sgml
@@ -282,6 +282,8 @@ COPY { <replaceable class="parameter">table_name</replaceable> [ ( <replaceable
       of the columns in the header line must match the actual column names of
       the table, otherwise an error is raised.
       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>
    </varlistentry>
-- 
2.36.1

---

From: Julien Rouhaud <[email-masked]>
Date: Tue, 14 Jun 2022 17:13:19 +0800

On Mon, Jun 13, 2022 at 04:46:46PM +0900, Michael Paquier wrote:
> use \"match\" with HEADER in COPY TO".

Agreed.

> There is no need for the extra

Is there any rule for what error code should be used?

Maybe that's just me but I understand "not supported" as "this makes sense, but
this is currently a limitation that might be lifted later".

Here I don't think it can ever make to use MATCH for a COPY TO, apart from
ignoring its meaning and accept it as an alias for HEADER ON.  But if we don't
allow this loose alias now it would just cause trouble to later allow it so
having an invalid syntax or something like that sounds more suited.

---

From: Peter Eisentraut <[email-masked]>
Date: Wed, 15 Jun 2022 11:14:33 +0200

> this is currently a limitation that might be lifted later".

I tend to agree with that interpretation.

Also, when you consider the way SQL rules and error codes are set up, 
errors that are detected during parse analysis should be a subclass of 
"syntax error or access rule violation".

---

From: "Daniel Verite" <[email-masked]>
Date: Wed, 15 Jun 2022 13:50:14 +0200

Julien Rouhaud wrote:

> Maybe that's just me but I understand "not supported" as "this makes
> sense, but this is currently a limitation that might be lifted
> later".
 
Looking at ProcessCopyOptions(), there are quite a few invalid
combinations of options that produce
ERRCODE_FEATURE_NOT_SUPPORTED currently:

- HEADER in binary mode
- FORCE_QUOTE outside of csv
- FORCE_QUOTE outside of COPY TO
- FORCE_NOT_NULL outside of csv
- FORCE_NOT_NULL outside of COPY FROM
- ESCAPE outside of csv
- delimiter appearing in the NULL specification
- csv quote appearing in the NULL specification

FORCE_QUOTE and FORCE_NOT_NULL are options that only make sense in one
direction, so the errors when using these in the wrong direction are
comparable to the "HEADER MATCH outside of COPY FROM" error that we
want to add. In that sense, ERRCODE_FEATURE_NOT_SUPPORTED would be
consistent.

The other errors in the list above are more about the format itself,
with options that make sense only for one format. So the way we're
supposed to understand ERRCODE_FEATURE_NOT_SUPPORTED in these
other cases is that such format does not support such feature,
but without implying that it's a limitation.

Best regards,

---

From: Peter Eisentraut <[email-masked]>
Date: Thu, 16 Jun 2022 09:24:56 +0200

> but without implying that it's a limitation.

I don't feel very strongly about this.  It makes sense to stay 
consistent with the existing COPY code.

---

From: Michael Paquier <[email-masked]>
Date: Mon, 20 Jun 2022 09:03:23 +0900

On Thu, Jun 16, 2022 at 09:24:56AM +0200, Peter Eisentraut wrote:
> I don't feel very strongly about this.  It makes sense to stay consistent
> with the existing COPY code.

Yes, my previous argument is based on consistency with the
surroundings.  I am not saying that this could not be made better, it
surely can, but I would recommend to tackle that in a separate patch,
and apply that to more areas than this specific one.
--
Michael

---

From: Michael Paquier <[email-masked]>
Date: Wed, 22 Jun 2022 08:34:34 +0900

> and apply that to more areas than this specific one.

Peter, beta2 is planned for next week.  Do you think that you would be
able to address this open item by the end of this week?  If not, and I
have already looked at the proposed patch, I can jump in and help.
--
Michael

---

From: Peter Eisentraut <[email-masked]>
Date: Wed, 22 Jun 2022 12:22:01 +0200

> have already looked at the proposed patch, I can jump in and help.

The latest patch was posted by you, so I was deferring to you to commit 
it.  Would you like me to do it?

---

From: Michael Paquier <[email-masked]>
Date: Wed, 22 Jun 2022 20:00:15 +0900

On Wed, Jun 22, 2022 at 12:22:01PM +0200, Peter Eisentraut wrote:
> The latest patch was posted by you, so I was deferring to you to commit it.
> Would you like me to do it?

OK.  As this is originally a feature you have committed, I originally
thought that you would take care of it, even if I sent a patch.  I'll
handle that tomorrow then, if that's fine for you, of course.  Happy
to help.
--
Michael

---

From: Michael Paquier <[email-masked]>
Date: Thu, 23 Jun 2022 13:26:29 +0900

> handle that tomorrow then, if that's fine for you, of course.  Happy

And done.  Thanks.
--
Michael

---

From: Julien Rouhaud <[email-masked]>
Date: Thu, 23 Jun 2022 12:46:46 +0800

Thanks!