On Thu, Jan 22, 2026 at 2:47 AM Matheus Alcantara <[email protected]> wrote: > > Thanks for the new version. I have some comments on this first round of > review: > > + errmsg_plural("invalid values in %" PRIu64 " row was replaced with null due > to data type incompatibility", > + "invalid values in %" PRIu64 " rows were replaced with null due to data > type incompatibility", > > I think that we could remove the "invalid values in" to make it > consistency with the COPY_ON_ERROR_IGNORE NOTICE > sure.
> ---------- > > + cstate->domain_with_constraint = (bool *) palloc0(attr_count > * sizeof(bool)); > > I think that we can use palloc_array? > sure. > ---------- > > Should FORCE_NOT_NULL be allowed to be used with ON_ERROR set_null? It > seems to me that ON_ERROR set_null overwrite the FORCE_NOT_NULL > behaviour: > > postgres=# create table t4(a int, b varchar(5)); > CREATE TABLE > > postgres=# copy t4 from 'data.csv' with (FORCE_NOT_NULL(b), format csv, > delimiter ',', NULL 'NULL', ON_ERROR set_null); > NOTICE: invalid values in 2 rows were replaced with null due to data type > incompatibility > COPY 5 > > postgres=# \pset null 'NULL' > Null display is "NULL". > postgres=# select * from t4; > a | b > ---+------ > 1 | aaaa > 2 | bbbb > 2 | NULL > 2 | NULL > 5 | NULL > (5 rows) > > Note that only the ccccc rows on .csv file was inserted with a NULL > value on b column. The 5,NULL row was inserted with a "NULL" string as a > value: > > postgres=# select * from t4 where b is null; > a | b > ---+------ > 2 | NULL > 2 | NULL > (2 rows) > > The contents of data.csv: > 1,aaaa > 2,bbbb > 2,ccccc > 2,ccccc > 5,NULL > > Perhaps we should block the usage of FORCE_NOT_NULL with ON_ERROR > SET_NULL? > FORCE_NOT_NULL is related to how we handle NULL string in column value. We first process cstate->opts.force_notnull_flags, cstate->opts.force_null_flags then InputFunctionCallSafe. see copyfromparse.c, CopyFromTextLikeOneRow ``if (is_csv)``loop. I think these two are unrelated things, FORCE_NOT_NULL should be fine with ON_ERROR SET_NULL. you can see related tests in https://git.postgresql.org/cgit/postgresql.git/tree/src/test/regress/sql/copy2.sql#n330 Am I missing something? > > On monitoring.sgml we have the following for pg_stat_progress_copy > tuples_skipped: > Number of tuples skipped because they contain malformed data. > This counter only advances when a value other than > <literal>stop</literal> is specified to the <literal>ON_ERROR</literal> > > IIUC we are not updating this view if we set a column to NULL due to an > error, perhaps this documentation should be updated to mention that it > will not be updated with ON_ERROR set_null? > IMHO, we don't need to mention ON_ERROR set_null, since we do not support it. change to the following should be ok, i think. <para> Number of tuples skipped because they contain malformed data. This counter only advances when <literal>ignore</literal> is specified to the <literal>ON_ERROR</literal> option. </para></entry> > > I may have missing something, but we are still considering implementing > the REJECT_LIMIT + ON_ERROR set_null? Possibly as a separate patch later. -- jian https://www.enterprisedb.com/
