Обсуждение: Use strtoi64() in pgbench, replacing its open-coded implementation

Here's a small patch to replace the int64 parsing code in pgbench with a 
call to strtoi64(). Makes it a little simpler.

Spotted this while grepping for all the different integer parsing 
functions we have. We could probably consolidate them some more, we 
still have quite a different integer-parsing routines in the backend and 
in the frontend. But this is one small, straightforward step in that 
direction.

- Heikki

Neil Chen <carpenter.nail.cz@gmail.com> writes:
> +1 on this simplification – it definitely makes the code cleaner.
> One small note: the updated code doesn’t handle trailing spaces in the
> input string. Should we consider this a concern?

Heikki's draft commit message addresses that point:

    The old implementation accepted trailing whitespace, but that seemed
    unnecessary. Firstly, its sibling function for parsing decimals,
    strtodouble(), does not accept trailing whitespace. Secondly, none of
    the callers can pass a string with trailing whitespace to it.

I didn't try to verify the latter assertion, but if it's true,
we don't need the extra complication.

            regards, tom lane

> On Nov 19, 2025, at 23:37, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>
> Here's a small patch to replace the int64 parsing code in pgbench with a call to strtoi64(). Makes it a little
simpler.
>
> Spotted this while grepping for all the different integer parsing functions we have. We could probably consolidate
themsome more, we still have quite a different integer-parsing routines in the backend and in the frontend. But this is
onesmall, straightforward step in that direction. 
>
> - Heikki
> <0001-Use-strtoi64-in-pgbench-replacing-its-open-coded-imp.patch>

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.

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.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

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

On 2025-Nov-21, Heikki Linnakangas wrote:

> 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.

I had the same thought -- is_an_int() is not doing anything useful and
it would be better to get rid of it.  If we do have an integer-looking
that doesn't fit in int64, then maybe treating it as a double is not
wrong.  (I suppose if we wanted to have numeric values beyond int64
range and not lose precision, we would have to add separate support for
that.)

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/