Re: refactoring - share str2*int64 functions

On Tue, Sep 03, 2019 at 08:10:37PM +0200, Fabien COELHO wrote:
> Attached a rebased version which implements the int64/uint64 stuff. It is
> basically the previous patch without the overflow inlined functions.

- if (!strtoint64(yytext, true, &yylval->ival))
+ if (unlikely(pg_strtoint64(yytext, &yylval->ival) != PG_STRTOINT_OK))
It seems to me that we should have unlikely() only within
pg_strtoint64(), pg_strtouint64(), etc.

- /* skip leading spaces; cast is consistent with strtoint64 */
- while (*ptr && isspace((unsigned char) *ptr))
+ /* skip leading spaces; cast is consistent with pg_strtoint64 */
+ while (isspace((unsigned char) *ptr))
ptr++;
What do you think about splitting this part in two? I would suggest
to do the refactoring in a first patch, and the consider all the
optimizations for the routines you have in mind afterwards.

I think that we don't actually need is_an_int() and str2int64() at all
in pgbench.c as we could just check for the return code of
pg_strtoint64() and switch to the double parsing only if we don't have
PG_STRTOINT_OK.

scanint8() only has one caller in the backend with your patch, and we
don't check after its return result in int8.c, so I would suggest to
move the error handling directly in int8in() and to remove scanint8().

I think that we should also introduce the [u]int[16|32] flavors and
expand them in the code in a single patch, in a way consistent with
what you have done for int64/uint64 as the state that we reach with
the patch is kind of weird as there are existing versions numutils.c.

Have you done some performance testing of the patch? The COPY
bulkload is a good example of that:
https://www.postgresql.org/message-id/20190717181428.krqpmduejbqr4m6g%40alap3.anarazel.de
--
Michael