On 20/11/2025 05:36, Chao Li wrote:
> I have no concern if we decide to no longer support tailing spaces, while I still got a couple of small comments:
>
> 1
> ```
> -/* return whether str matches "^\s*[-+]?[0-9]+$" */
> +/*
> + * Return whether str matches "^\s*[-+]?[0-9]+$"
> + *
> + * This should agree with strtoint64() on what's accepted, ignoring overflows.
> + */
> static bool
> is_an_int(const char *str)
> ```
>
> Here you added a comment saying "ignoring overflows”, yes, is_an_int() doesn’t check if the integer overflows.
>
> But looking at where the function is called:
> ```
> else if (is_an_int(var->svalue))
> {
> /* if it looks like an int, it must be an int without overflow */
> int64 iv;
>
> if (!strtoint64(var->svalue, false, &iv))
> return false;
>
> setIntValue(&var->value, iv);
> }
> ```
>
> The comment says “it must be an int without overflow”, so this comment should be updated as well.
Hmm, I don't think it's wrong as it is, and this patch doesn't change
that behavior. That comment is a little vague though.
How about the following phrasing:
/*
* If it looks like an integer, treat it as such. If it turns out to be
* too large for 'int64', return failure rather than fall back to 'double'.
*/
I don't feel the urge to refactor this myself right now, but we probably
could simplify this further. For example, I wonder if we should remove
is_an_int() altogether and rely on strtoi64() to return failure if the
input does't look like a integer. Also, strtodouble() is never called
with "errorOk != false".
> 2
> ```
> + if (unlikely(errno != 0))
> {
> - int8 digit = (*ptr++ - '0');
> -
> - if (unlikely(pg_mul_s64_overflow(tmp, 10, &tmp)) ||
> - unlikely(pg_sub_s64_overflow(tmp, digit, &tmp)))
> - goto out_of_range;
> + if (!errorOK)
> + pg_log_error("value \"%s\" is out of range for type bigint", str);
> + return false;
> }
> ```
>
> Here we log an “out out range” error when errno is not 0, which is inaccurate, we should check ERANGE.
>
> strtoi64() maps to strtol()/strtoll(), the functions could return more errors than ERANGE.
Good point. The existing strtodouble() function, which uses strtod(),
has the same issue (per POSIX spec at
https://pubs.opengroup.org/onlinepubs/000095399/functions/strtod.html):
> These functions may fail if:
>
> [EINVAL]
> [CX] [Option Start] No conversion could be performed. [Option End]
Fixed that and committed. Thanks for the review!
- Heikki
