Hi Ayoub,
Thanks for the report and the patch, and thanks to Chao for
the review round that shaped v3. I reproduced the bug, read
the fix, and tested v3 (the version Chao posted on Nov 5);
this review is of v3 as posted.
I agree with the mechanism. The header line and the first
data row are read inside the same NextCopyFrom() call, and
cstate->defaults is only cleared at the top of that
function, so a header field that matches the DEFAULT
marker sets the flag during header parsing and it leaks
into row 1. The fix sits at the right layer: it disables
marker recognition only while the header line is parsed
under HEADER MATCH, by clearing opts.default_print around
the CopyReadAttributesCSV()/Text() call and restoring it
before any data row is read.
One scope note, following the thread: I reviewed the
DEFAULT marker fix only. The NULL marker question from
Chao's v2 is parked for a separate thread, as agreed, and
I checked only that v3 does not change the existing
NULL/header behaviour (see below).
What I verified, on master c07545dfe1a, with v3
cherry-picked on top (it applies cleanly; the patch was
written against fc1900915b1):
Reproduced on unpatched master, in both CSV and text
formats:
With a default on the column: header "id,default,note"
with DEFAULT 'default' loads row 1 with the column
default (0) instead of the supplied value (5). Silent
data corruption; row 2 loads normally.
Without a default: COPY aborts with ERROR "unexpected
default marker in COPY data", "Column "d" has no
default value", pointing at line 1, the header.
The same failure through file_fdw (a foreign table with
header 'match' and a default marker): the scan errors
on line 1 the same way.
With v3 applied:
Both cases load correctly in CSV and in text: row 1
keeps its supplied value, and the no-default case loads
cleanly. file_fdw loads correctly too.
A genuine DEFAULT marker in the data rows still works,
including in the first data row for the very column
whose name matched in the header, so the fix exempts
only the header line and does not weaken marker
handling in data.
A header field of \N errors exactly as on master
("column name mismatch ... got "\N""), so the existing
NULL/header behaviour is untouched, as is HEADER TRUE
with a data marker.
Scope check: I grepped the tree for default_print. It is
consulted for marker recognition only in
CopyReadAttributesText() and CopyReadAttributesCSV(),
which are exactly the two readers the patched call site
wraps; the remaining uses are option parsing/validation
and building default expressions. HEADER TRUE never
parses header fields and binary format has no header
parsing, so the HEADER MATCH call site is the complete
affected surface.
Tests: the two new copy2 cases pin both failure modes,
and the second one also proves a data-row marker still
fires after a matching header. As a discrimination check
I ran v3's test files against unpatched master: copy2
fails, and it is the only failure in the core suite
(1 of 239). With the patch, the full core regression
suite passes (239/239). installcheck-world against a
running server also passes: 54 suites, 658 tests, all
passed. I then reconfigured the same tree with TAP
tests enabled and ran check-world, which also passes
end to end, TAP suites included: 320 test files,
26,839 subtests, no failures. 32 TAP files skip for
test-configuration reasons (suites gated behind
PG_TEST_EXTRA, Windows-only tests, and one test that
needs readline, which this build omits), and the
kerberos and ldap suites are not part of this build
configuration.
One small suggestion, not a blocker: the new regression
tests cover CSV only. Both readers are reached from the
one patched call site, and I verified the text format by
hand in both failure modes, but a text-format case in
copy2 would round out the coverage.
For the committer: the bug is present from v16 (which
added the DEFAULT option) together with HEADER MATCH
(v15), so REL_16_STABLE through master are affected;
how far to backpatch I will leave to you. The commit
message already carries the Author, Reported-by,
Reviewed-by and Discussion lines.
the builds,the probe battery, and the test-suite runs
were done with AI assistance, and I reviewed the results
and the patch itself as described above.
Verdict: I am marking this Ready for Committer.
Regards,
Prashant Chinnam
