Re: Add new COPY option REJECT_LIMIT

Thanks for your review!

On Thu, Oct 3, 2024 at 4:27 PM jian he <jian.universality@gmail.com> 
wrote:

> mentioning <replaceable class="parameter">maxerror</replaceable> is a
> bigint type
> or explicitly mentioning the maximum allowed value of "maxerror" would 
> be great.

Added a description that it allows positive bigint.


On Thu, Oct 3, 2024 at 11:43 PM Fujii Masao 
<masao.fujii@oss.nttdata.com> wrote:

> +       if (opts_out->reject_limit && !opts_out->on_error)
> +               ereport(ERROR,
> +                               
> (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> +               /*- translator: first and second %s are the names of 
> COPY
> +                * option, e.g. ON_ERROR, third is the value of the 
> COPY option,
> +                * e.g. IGNORE */
> +                                errmsg("COPY %s requires %s to be set 
> to %s",
> +                                                "REJECT_LIMIT", 
> "ON_ERROR", "IGNORE")));
> 
> This check might be better moved to other place, for example at
> the bottom of ProcessCopyOptions(). I noticed a comment in the code:
> "Check for incompatible options (must do these two before inserting 
> defaults)."
> You added your condition after this comment and before inserting the 
> defaults,
> but it's not related to that process. So, moving this check to other 
> place
> seems more appropriate.

Agreed. Moved it to the bottom of ProcessCopyOptions().

> While reviewing, I also noticed that the check for
> "opts_out->binary && opts_out->on_error != COPY_ON_ERROR_STOP"
> is similarly placed before setting the defaults, which might not
> be correct. This check should probably be moved as well.
> Additionally, the comment mentioning "must do these two" should be
> updated to "must do these three." These changes should be handled
> in a separate patch.

Agreed and attached 0002 patch.

> +                       if (cstate->opts.reject_limit &&
> 
> Wouldn't it be clearer to use cstate->opts.reject_limit > 0 for better 
> readability?

Agreed.

> +                               cstate->num_errors > 
> cstate->opts.reject_limit)
> +                               ereport(ERROR,
> +                                               
> (errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
> +                                                errmsg("skipped more 
> than REJECT_LIMIT rows: \"%lld\",",
> +                                                               (long 
> long) cstate->opts.reject_limit)));
> 
> To make the error message clearer, similar to the NOTICE about
> skipped rows, how about rewording it to:
> "skipped more than REJECT_LIMIT (%lld) rows due to data type 
> incompatibility"?

Agreed.
Also considering when REJECT_LIMIT is specified to 1, attached patch 
uses errmsg_plural() instead of errmsg.


-- 
Regards,

--
Atsushi Torikoshi
NTT DATA Group Corporation