Re: refactoring - share str2*int64 functions

Hi,

On 2019-07-17 12:18:19 +0900, Michael Paquier wrote:
> On Tue, Jul 16, 2019 at 01:04:38PM -0700, Andres Freund wrote:
> > There is the issue that there already is pg_strtoint16 and
> > pg_strtoint32, which do not have the option to not raise an error. I'd
> > probably name the non-error throwing ones something like pg_strtointNN_e
> > (for extended, or error handling), and have pg_strtointNN wrappers that
> > just handle the errors, or reverse the naming (which'd cause a bit of
> > churn, but not that much).
> >
> > That'd also make the code for pg_strtointNN a bit nicer, because we'd
> > not need the gotos anymore, they're just there to avoid redundant error
> > messages - which'd not be an issue if the error handling were just a
> > switch in a separate function. E.g.
>
> Agreed on that. I am wondering if we should use a common wrapper for
> all the internal functions taking in input a set of bits16 flags to
> control its behavior and put all that into common/script.c:
> - Issue an error.
> - Check for signedness.
> - Base length: 16, 32 or 64.

That'd be considerably slower, so I'm *strongly* against that. These
conversion routines are *really* hot in a number of workloads,
e.g. bulk-loading with COPY. Check e.g.
https://www.postgresql.org/message-id/20171208214437.qgn6zdltyq5hmjpk%40alap3.anarazel.de

> I would also rather not touch the strtol wrappers that we have able to
> handle the base. There is nothing in the tree using it, but likely
> there are extensions relying on it.

I doubt it - it's not of that long-standing vintage (23a27b039d9,
2016-03-12), and if so they are very likely to use base 10. We shouldn't
keep some barely tested function around, just for the hypothetical
scenario that some extension uses it. Especially if that function is
considerably slower than the potential replacement.

Greetings,

Andres Freund