Re: refactoring - share str2*int64 functions

On Mon, Sep 09, 2019 at 05:27:04AM -0700, Andres Freund wrote:
> On 2019-09-09 20:57:46 +0900, Michael Paquier wrote:
> But ISTM all of them ought to just use the C types, rather than the SQL
> types however. Since in the above proposal the caller determines the
> type names, if you want a different type - like the SQL input routines -
> can just invoke pg_strtoint_error() themselves (or just have it open
> coded).

Yep, that was my line of thoughts.

>> And for errors which should never happen we could just use
>> elog(). For the input functions of int2/4/8 we still need the
>> existing errors of course.
>
> Right, there it makes sense to continue to refer the SQL level types.

Actually, I found your suggestion of using a noreturn function for the
error reporting to be a very clean alternative. I didn't know though
that gcc is not able to detect that a function does not return if you
don't have a default in the switch for all the status codes. And this
even if all the values of the enum for the switch are listed.

> I'm saying that we shouldn't need the whole logic of trying to parse the
> string as an int, and then fail to float if it's not that. But that it's
> not this patchset's task to fix this.

Ah, sure. Agreed.

>> Not sure about that. I would keep the scope of the patch simple as of
>> now, where we make sure that we have the right interface for
>> everything. There are a couple of extra improvements which could be
>> done afterwards, and if we move everything in the same place that
>> should be easier to move on with more improvements. Hopefully.
>
> The only reason for thinking about it now is that we'd then avoid
> changing the API twice.

What I think we would be looking for here is an extra argument for the
low-level routines to control the behavior of the function in an
extensible way, say a bits16 for a set of flags, with one flag to
ignore checks for trailing and leading whitespace. This feels a bit
over-engineered though for this purpose.

Attached is an updated patch? How does it look? I have left the
parts of readfuncs.c for now as there are more issues behind that than
doing a single switch, short reads are one, long reads a second. And
the patch already does a lot. There could be also an argument for
having extra _check wrappers for the unsigned portions but these would
be mostly unused in the backend code, so I have left that out on
purpose.

After all that stuff, there are still some issues which need more
care, in short:
- the T_Float conversion.
- removal of strtoint()
- the part for readfuncs.c
--
Michael