> On Nov 4, 2026, at 16:57, Ayoub Kazar <[email protected]> wrote: > > Hi, > Thanks for the review! > On 04/11/2026 07:54, Chao Li wrote: >> ERROR: column name mismatch in header line field 2: got null value ("a"), >> expected "a" >> CONTEXT: COPY t, line 1: "id,a" >> ``` >> >> Here, a is a valid column name in the header line and should not be rejected >> simply because it matches the NULL marker. >> >> Adding an is_header parameter allows us to disable both NULL and DEFAULT >> marker recognition when parsing header lines, addressing both issues with >> the same approach. It also avoids temporarily modifying the COPY options. >> >> So, I’m proposing v2 with this approach. I kept the regression tests you >> added in v1 and added tests for NULL markers and the text format. Would you >> please review v2? > I have mixed feelings about this: > > CREATE TABLE t (id int, "N" text); > > If user inputs this in COPY: > id \N > 1 \N >
Good point. I hadn’t considered this case. This would incorrectly allow COPY to
succeed, even though \N in the header line should cause it to fail.
I believe this can be resolved if we pursue v2’s approach, but some refactoring
will likely be needed.
> same is in diffed existing regression test for it:
> -ERROR: column name mismatch in header line field 3: got null value ("\N"),
> expected "c"
> +ERROR: column name mismatch in header line field 3: got "N", expected "c"
> IMO, i prefer having this error out for NULL marker reason in a column name,
> rather than reading '\N' as 'N' in header, and this might be the intention of
> who wrote it... maybe.
> I agree this should be data rows only, but the documentation doesn't specify
> that either, at least not as expected as the case of DEFAULT marker. So i'm
> not sure...
>
> CSV case:
> In CSV, CopyReadAttributesCSV() already checks `!saw_quote`. If a user has a
> column named "a" and sets `null 'a'`, quoting the column name in the header
> (`id,"a"`) already prevents it from being treated as NULL.
>
That’s true, but I think v2 would remove this restriction, so users would no
longer have to quote column names in the header line just because they match
the NULL marker. I consider that an improvement.
>
> I have an extra worry about performance regressions, previously from [1], i
> saw that very minimal changes effect the binary layout and branching (without
> going into much details), therefore having 1-3% performance regressions or
> improvements that appear just like that (this applies everywhere in Postgres
> really...), CopyReadAttributesCSV() is very sensitive, it runs millions of
> times very easily and its a high registers-pressured code.
>
> A not too serious benchmark on v2 already shows 5% regressions.
Could you share how you ran the benchmark and measured the 5% regression?
I ran a brief benchmark with assertions disabled and -O2. The test copies 1
million rows into a table with 32 text columns. The results are:
```
| Version | Format | Runs | Average time | Change vs master |
|---|---|---:|---:|---:|
| Master (`590788ad0bf`) | CSV | 6 | 625.382 ms | — |
| Master | Text | 5 | 627.275 ms | — |
| Master | Binary | 5 | 795.974 ms | — |
| v2 | CSV | 5 | 641.186 ms | +2.53% |
| v2 | Text | 5 | 607.293 ms | −3.19% |
| v2 | Binary | 6 | 785.810 ms | −1.28% |
```
These results don’t show a clear performance regression to me, though this was
only a brief benchmark.
Overall, I agree that point 1 has to be addressed, and doing so would require
more work beyond the scope of this patch. Let’s drop v2 for now; I’ll propose
it in a separate thread.
The attached v3 restores the v1 fix. I only adjusted the commit message
formatting and added author/reviewer annotations.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
v3-0001-Fix-DEFAULT-marker-handling-in-COPY-FROM-.-HEADER.patch
Description: Binary data
