Re: Add new COPY option REJECT_LIMIT

On 2024-09-28 10:48, Jian he wrote:

Thanks for your comments!

> +/*
> + * Extract REJECT_LIMIT value from a DefElem.
> + */
> +static int64
> +defGetCopyRejectLimitOptions(DefElem *def)
> +{
> + int64 reject_limit;
> +
> + if (def->arg == NULL)
> + ereport(ERROR,
> + (errcode(ERRCODE_SYNTAX_ERROR),
> + errmsg("REJECT_LIMIT requires a positive integer")));
> +
> + if (nodeTag(def->arg) == T_Integer)
> + {
> + reject_limit = defGetInt64(def);
> + if (reject_limit <= 0)
> + ereport(ERROR,
> + (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> + errmsg("REJECT_LIMIT (%lld) must be greater than zero",
> + (long long) reject_limit)));
> + }
> + else
> + {
> + char   *sval = defGetString(def);
> + ereport(ERROR,
> + (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> + errmsg("REJECT_LIMIT (%s) must be a positive integer",
> + sval)));
> + }
> +
> + return reject_limit;
> +}
> there will be an integer overflow issue?
> 
> Can you try the following?

Since defGetInt64() checks not only whether the input value exceeds the 
range of bigint but also input value is specified, attached v6 patch 
checks them by directly calling defGetInt64().

> 
> static int64
> defGetCopyRejectLimitOptions(DefElem *def)
> {
>     int64                    reject_limit;
>     reject_limit = defGetInt64(def);
>     if (reject_limit <= 0)
>         ereport(ERROR,
>             (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
>                 errmsg("REJECT_LIMIT (%lld) must be greater than zero",
>                     (long long) reject_limit)));
> }
> 
> 
> 
> 
> REJECT_LIMIT <replaceable class="parameter">integer</replaceable>
> i think, you want REJECT_LIMIT be bigint?
> so here it should be
> REJECT_LIMIT <replaceable class="parameter">bigint</replaceable>\
> ?

Hmm, bigint and integer include numbers less than 0, but REJECT_LIMIT 
only accepts numbers greater than 0. I now feel both may not be 
appropriate.
Referring to the manual of CREATE SEQUENCE[1], here we may be able to 
use not only the data types, such as bigint and integer but something 
like minvalue, maxvalue.
I'm wondering if we can use the wording maxerror as in the attached 
patch.


[1] https://www.postgresql.org/docs/devel/sql-createsequence.html


-- 
Regards,

--
Atsushi Torikoshi
NTT DATA Group Corporation