Re: Add new COPY option REJECT_LIMIT

Thanks for your review!

On Thu, Oct 3, 2024 at 4:27 PM jian he <jian(dot)universality(at)gmail(dot)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(dot)fujii(at)oss(dot)nttdata(dot)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