> On Nov 5, 2026, at 16:04, Michael Paquier <[email protected]> wrote: > > On Thu, Nov 05, 2026 at 02:43:54PM +0800, Chao Li wrote: >> So the fix is simple: we can add the cleanup in the >> COPY_ON_ERROR_IGNORE branch and remove the COPY_ON_ERROR_SET_NULL >> branch, letting it fall through so that the cleanup is performed. > > I've also wondered exactly about that a couple of hours ago while > looking at a different bug in the same area of the code.. Interesting > timing. > > I don't buy that the current coding on HEAD is correct, based on what > I can see here from this test: > * SQL input: > CREATE TABLE check_ign_err (n int, m int[], k int); > COPY check_ign_err FROM STDIN WITH (on_error ignore); > bad {1} 3 > 1 {1} 3 abc > \. > > * HEAD: > ERROR: 22P04: extra data after last expected column > CONTEXT: COPY check_ign_err, line 2, column n: "1" > * with your patch: > ERROR: 22P04: extra data after last expected column > CONTEXT: COPY check_ign_err, line 2: "1 {1} 3 4" > > Complaining about the first column on the second line is incorrect and > gets fed from the first incorrect line: we have extra fields on this > line, they should be mentioned. > -- > Michael
Hi Michael, I just saw that you pushed 23e2a332dd, which resolves the "ON_ERROR ignore" issue, but I think this patch is still needed to resolve the "ON_ERROR set_null" problem. If I revert this patch's code changes and only add the new test case, "make check" fails: ``` # diff -U3 /Users/chaol/Documents/code/postgresql/src/test/regress/expected/copy2.out /Users/chaol/Documents/code/postgresql/src/test/regress/results/copy2.out # --- /Users/chaol/Documents/code/postgresql/src/test/regress/expected/copy2.out 2026-11-06 10:06:21 # +++ /Users/chaol/Documents/code/postgresql/src/test/regress/results/copy2.out 2026-11-06 10:11:55 # @@ -829,7 +829,7 @@ # -- fail, less data after a conversion error. # COPY t_on_error_null FROM STDIN WITH (delimiter ',', on_error set_null); # ERROR: missing data for column "c" # -CONTEXT: COPY t_on_error_null, line 1: "1,bad" # +CONTEXT: COPY t_on_error_null, line 1, column b: "bad" # -- fail, extra data. # COPY t_on_error_null FROM STDIN WITH (delimiter ',', on_error set_null); # ERROR: extra data after last expected column not ok 201 + copy2 ``` This patch also clears the current attribute in CopyFromTextLikeOneRow() before returning for ON_ERROR ignore. I thought that might be redundant with the clearing done by 23e2a332dd, so I tried removing it. The test then failed against this patch's updated expected output: ``` # diff -U3 /Users/chaol/Documents/code/postgresql/src/test/regress/expected/copy2.out /Users/chaol/Documents/code/postgresql/src/test/regress/results/copy2.out # --- /Users/chaol/Documents/code/postgresql/src/test/regress/expected/copy2.out 2026-11-06 10:06:21 # +++ /Users/chaol/Documents/code/postgresql/src/test/regress/results/copy2.out 2026-11-06 10:21:09 # @@ -905,7 +905,7 @@ # -- tests for reject_limit option # COPY check_ign_err FROM STDIN WITH (on_error ignore, reject_limit 3); # ERROR: skipped more than REJECT_LIMIT (3) rows due to data type incompatibility # -CONTEXT: COPY check_ign_err, line 5: "" # +CONTEXT: COPY check_ign_err, line 5, column n: "" # COPY check_ign_err FROM STDIN WITH (on_error ignore, reject_limit 4); # NOTICE: 4 rows were skipped due to data type incompatibility ``` Then I realized that the REJECT_LIMIT error is raised before the next call to NextCopyFrom(), so clearing the attribute in NextCopyFrom() doesn’t cover the REJECT_LIMIT path. With this patch's cleanup in place, I think we can remove the clearing from NextCopyFrom(). I haven't done so in this patch. I'd leave that decision to you. V2 is a rebased version. Best regards, -- Chao Li (Evan) HighGo Software Co., Ltd. https://www.highgo.com/
v2-0001-Fix-stale-column-context-after-COPY-FROM-conversi.patch
Description: Binary data
