Re: Add new COPY option REJECT_LIMIT

On 2024-09-25 02:22, Fujii Masao wrote:
Thanks for your review!

Attached v5 patch.

> On 2024/09/24 14:25, torikoshia wrote:
>> Updated the patch.
> 
> Thanks for updating the patch!
> 
> +    REJECT_LIMIT { <replaceable 
> class="parameter">integer</replaceable> }
> 
> The curly braces {} seem unnecessary here.
> 
> +      When a positive integer value is specified,
> <command>COPY</command> limits
> +      the maximum tolerable number of errors while converting a 
> column's input
> +      value into its data type.
> +      If input data caused more errors than the specified value, 
> entire
> +      <command>COPY</command> fails.
> +      Otherwise, <command>COPY</command> discards the input row and 
> continues
> +      with the next one.
> +      This option must be used with <literal>ON_ERROR</literal> to be 
> set to
> +      <literal>ignore</literal>.
> +      Just setting <literal>ON_ERROR</literal> to 
> <literal>ignore</literal>
> +      tolerates unlimited number of errors.
> 
> Regarding the description of REJECT_LIMIT, how about clarifying what
> the option specifies up front, like this:
> 
> ----------------
> Specifies the maximum number of errors tolerated while converting a 
> column's
> input value to its data type, when on_error is set to "ignore." If the 
> input
> causes more errors than the specified value, the COPY command fails,
> even with on_error set to "ignore." This value must be positive and 
> used with
> on_error="ignore". If not specified, on_error="ignore" allows an 
> unlimited
> number of errors, meaning COPY will skip all erroneous data.
> ----------------

Thanks for writing it. It seems better.

> +			ereport(ERROR,
> +				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> +				 errmsg("number for REJECT_LIMIT must be greater than zero")));
> +	}
> +	else
> +		ereport(ERROR,
> +			(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> +			 errmsg("value for REJECT_LIMIT must be positive integer")));
> 
> The phrases "number for" and "value for" seem unnecessary in the error 
> messages.
> Also, "positive number" should be "a positive number." Would it be
> better to display
> the actual specified value in the error message, like:
> errmsg("REJECT_LIMIT (%s) must
> be a positive number", value)?

Agreed.

> Lastly, during my testing, if nothing is specified for REJECT_LIMIT
> (e.g., COPY ... WITH (..., REJECT_LIMIT)), the COPY command caused a
> segmentation fault.

Oh, thanks for finding it. Fixed.
> 
> 
>>> I'd like to hear if anyone has an opinion on the need for supporting 
>>> ratio.
>> 
>> Since there was no opinion about it, attached a patch only for 
>> REJECT_LIMIT specifying number.
> 
> +1
> 
>> As there are no opposing opinions, removed 'INFINITY' as well.
> 
> +1
> 
> Regards,

-- 
Regards,

--
Atsushi Torikoshi
NTT DATA Group Corporation