Re: refactoring - share str2*int64 functions

From: Michael Paquier <michael(at)paquier(dot)xyz>
To: Andres Freund <andres(at)anarazel(dot)de>
Cc: Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr>, Thomas Munro <thomas(dot)munro(at)gmail(dot)com>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, PostgreSQL Developers <pgsql-hackers(at)lists(dot)postgresql(dot)org>
Subject: Re: refactoring - share str2*int64 functions
Date: 2019-09-10 03:05:25
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

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

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

Attachment Content-Type Size
str2int-13.patch text/x-diff 38.5 KB

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Joe Nelson 2019-09-10 04:02:49 Re: Change atoi to strtol in same place
Previous Message Amit Kapila 2019-09-10 02:59:43 Re: [HACKERS] [PATCH] pageinspect function to decode infomasks