Re: refactoring - share str2*int64 functions - Mailing list pgsql-hackers

Hi,

On 2019-07-15 07:08:42 +0200, Fabien COELHO wrote:
> I do not think that changing the error handling capability is appropriate,
> it is really a feature of the function. The function could try to use an
> internal pg_strtoint64 which would look like the other unsigned version, but
> then it would not differentiate the various error conditions (out of range
> vs syntax error).

> The compromise I can offer is to change the name of the first one, say to
> "pg_scanint8" to reflect its former backend name. Attached a v4 which does a
> renaming so as to avoid the name similarity but signature difference. I also
> made both error messages identical.

I think the interface of that function is not that good, and the "scan"
in the name isn't great for discoverability (for one it's a different
naming than pg_strtoint16 etc), and the *8 meaning 64bit is confusing
enough in the backend, we definitely shouldn't extend that to frontend
code.

Referencing "bigint" and "input syntax" from frontend code imo doesn't
make a lot of sense. And int8in is the only caller that uses
errorOK=False anyway, so there's currently no need for the frontend
error strings afaict.

ISTM that something like

typedef enum StrToIntConversion
{
        STRTOINT_OK = 0,
        STRTOINT_SYNTAX_ERROR = 1,
        STRTOINT_OUT_OF_RANGE = 2
} StrToIntConversion;
StrToIntConversion pg_strtoint64(const char *str, int64 *result);

would make more sense.

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.

int32
pg_strtoint32(const char *s)
{
    int32 result;

    switch (pg_strtoint32_e(s, &result))
    {
            case STRTOINT_OK:
                 return result;

            case STRTOINT_SYNTAX_ERROR:
                ereport(ERROR,
                        (errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
                         errmsg("value \"%s\" is out of range for type %s",
                                s, "integer")));

            case STRTOINT_OUT_OF_RANGE:
                ereport(ERROR,
                        (errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
                         errmsg("invalid input syntax for type %s: \"%s\"",
                                "integer", s)));

    }

    return 0;                   /* keep compiler quiet */
}

which does seem nicer than what we have right now.


Greetings,

Andres Freund