Re: Change COPY ... ON_ERROR ignore to ON_ERROR ignore_row - Mailing list pgsql-hackers

I have a few more cosmetic changes to suggest:

- doc/src/sgml/ref/copy.sgml

+      and <literal>set_null</literal> means replace field containing 
invalid

should be "the field" and "the invalid"

+      input value with <literal>NULL</literal> and continue to the next 
field.

change <literal>NULL</literal> to "a null value"

+    <para>
+      For <literal>ignore</literal> option, a <literal>NOTICE</literal> 
message
+      containing the ignored row count is emitted at the end of the 
<command>COPY FROM</command>
+      if at least one row was discarded.
+      For <literal>set_null</literal> option, a <literal>NOTICE</literal>
+      message indicating the number of rows where invalid input values were
+      replaced with null is emitted at the end of the <command>COPY 
FROM</command>
+      if at least one row was replaced.
+     </para>

I think this could be written more compactly, like

If on_error is set to ignore or set_null, a NOTICE message is emitted
at the end of the COPY FROM command containing the count of rows that
were ignored or changed, if at least one row was affected.


- src/backend/commands/copy.c

     /*
-    * Allow "stop", or "ignore" values.
+    * Allow "stop", "ignore", "set_null" values.
      */

Just remove that comment.  It is evident from the following code.


- src/backend/commands/copyfrom.c

+ "%" PRIu64 " rows were replaced with null due to data type 
incompatibility"

I think this is not quite correctly worded.  It should be something like

     in NNN rows, columns were set to null due to ...

because you are not setting the whole row to null.

         /*
-        * Currently we only support COPY_ON_ERROR_IGNORE. We'll add other
-        * options later
+        * Currently we only support COPY_ON_ERROR_IGNORE,
+        * COPY_ON_ERROR_SET_NULL. We'll add other options later
          */

Delete this comment.

+   if (cstate->opts.on_error == COPY_ON_ERROR_SET_NULL)
+   {
+       int         attr_count = list_length(cstate->attnumlist);
+
+       cstate->domain_with_constraint = palloc0_array(bool, attr_count);

Maybe add a comment for this block to explain that you are collecting 
information about domains for later.


- src/backend/commands/copyfromparse.c

         /*
-        * If ON_ERROR is specified with IGNORE, skip rows with soft errors
+        * If ON_ERROR is specified with IGNORE, skip rows with soft errors.
+        * If ON_ERROR is specified with SET_NULL, try to replace with null.
          */

Trim this comment.  Maybe "If ON_ERROR is specified, handle the 
different options".  We don't need to re-explain here what the options do.

+           /*
+            * If the column type is a constrained domain, an additional
+            * InputFunctionCallSafe may be needed to raise error for
+            * domain constraint violation.
+            */

Why "may be needed"?  Is it sometimes not needed?  Why, under what 
circumstances?

The subsequent error message writes "domain ... does not allow null 
values", but AFAICT a domain input failure could also be due to a check 
constraint failure?  How would that be handled?  The flow here is a bit 
confusing.

- src/test/regress/sql/copy2.sql

I suggest adding a space after "--" in a comment, like "-- error" 
instead of "--error".

Similarly, a space after CHECK, like "CHECK (...)".