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

On Mon, Sep 09, 2019 at 03:17:38AM -0700, Andres Freund wrote:
> On 2019-09-09 14:28:14 +0900, Michael Paquier wrote:
> I *VEHEMENTLY* oppose the introduction of any future pseudo-generic
> routines taking the type width as a parameter. They're weakening the
> type system and they're unnecessarily inefficient.

I saw that, using the previous wrapper increases the time of one call
from roughly 0.9ms to 1.1ns.

> I don't really buy that saving a few copies of a strings is worth that
> much. But if you really want to do that, the right approach imo would be
> to move the error reporting into a separate function. I.e. something
>
> void pg_attribute_noreturn()
> pg_strtoint_error(pg_strtoint_status status, const char *input, const char *type)
>
> that you'd call in small wrappers. Something like

I am not completely sure if we should do that either anyway.  Another
approach would be to try to make the callers of the routines generate
their own error messages.  The errors we have now are really linked to
the data types we have in core for signed integers (smallint, int,
bigint).  In most cases do they really make sense (varlena.c, pqmq.c,
etc.)?  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.

>> So, it seems to me that if we want to have a consolidation of
>> everything, we basically need to have the generic error messages for
>> the backend directly into common/string.c where the routines are
>> refactored, and I think that we should do the following based on the
>> patch attached:
>
>> - Just remove pg_strtoint(), and move the error generation logic in
>> common/string.c.
>
> I'm not quite sure what you mean by moving the "error generation logic"?

I was referring to the error messages we have on HEAD in scanint8()
and pg_strtoint16() for bad inputs and overflows.

> Seems like these actually could just ought to use the error-checked
> variants. And I think it ought to change all of
> READ_{INT,UINT,LONG,UINT64,OID}_FIELD, rather than just redirecting one

Right.

>>  static void pcb_error_callback(void *arg);
>> @@ -496,7 +496,7 @@ make_const(ParseState *pstate, Value *value, int location)
>>
>>          case T_Float:
>>              /* could be an oversize integer as well as a float ... */
>> -            if (scanint8(strVal(value), true, &val64))
>> +            if (pg_strtoint64(strVal(value), &val64) == PG_STRTOINT_OK)
>>              {
>>                  /*
>>                   * It might actually fit in int32. Probably only INT_MIN can
>
> Not for this change, but we really ought to move away from this crappy
> logic. It's really bonkers to have T_Float represent large integers and
> floats.

I am not sure but what are you suggesting here?

>> +    /* skip leading spaces */
>> +    while (likely(*ptr) && isspace((unsigned char) *ptr))
>> +        ptr++;
>> +
>> +    /* handle sign */
>> +    if (*ptr == '-')
>> +    {
>> +        ptr++;
>> +        neg = true;
>> +    }
>> +    else if (*ptr == '+')
>> +        ptr++;
>
>> +    /* require at least one digit */
>> +    if (unlikely(!isdigit((unsigned char) *ptr)))
>> +        return PG_STRTOINT_SYNTAX_ERROR;
>
> Wonder if there's an argument for moving this behaviour to someplace
> else - in most cases we really don't expect whitespace, and checking for
> it is unnecessary overhead.

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.
--
Michael