Re: refactoring - share str2*int64 functions

On Fri, Sep 13, 2019 at 06:38:31PM -0700, Andres Freund wrote:
> On 2019-09-10 12:05:25 +0900, Michael Paquier wrote:
>> On Mon, Sep 09, 2019 at 05:27:04AM -0700, Andres Freund wrote:
>> 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.
>
> Hm? I don't know what you mean by those issues.

I think that we have more issues than it looks.  For example:
- Switching UINT to use pg_strtouint32() causes an incompatibility
issue compared to atoui().
- Switching INT to use pg_strtoint32() causes a set of warnings as for
example with AttrNumber:
72 |  (void) pg_strtoint32(token, &local_node->fldname)
   |                              ^~~~~~~~~~~~~~~~~~~~~
   |                              |
   |                              AttrNumber * {aka short int *}
And it is not like we should use a cast either, as we could hide real
issues.     Hence it seems to me that we need to have a new routine
definition for shorter integers and switch more flags to that.
- Switching LONG to use pg_strtoint64() leads to another set of
issues, particularly one could see an assertion failure related to Agg
nodes.  I am not sure either that we should use int64 here as the size
can be at least 32b.
- Switching OID to use pg_strtoint32 causes a failure with initdb.

So while I agree with you that a switch should be doable, there is a
large set of issues to ponder about here, and the patch already does a
lot, so I really think that we had better do a closer lookup at those
issues separately, once the basics are in place, and consider them if
they actually make sense.  There is much more than just doing a direct
switch in this area with the family of ato*() system calls.

>> 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.
>
> I'd value consistency higher here.

Okay, no objections to that.

>> @@ -2338,8 +2339,7 @@ _SPI_execute_plan(SPIPlanPtr plan, ParamListInfo paramLI,
>>                      CreateTableAsStmt *ctastmt = (CreateTableAsStmt *) stmt->utilityStmt;
>>
>>                      if (strncmp(completionTag, "SELECT ", 7) == 0)
>> -                        _SPI_current->processed =
>> -                            pg_strtouint64(completionTag + 7, NULL, 10);
>> +                        (void) pg_strtouint64(completionTag + 7, &_SPI_current->processed);
>
> I'd just use the checked version here, seems like a good thing to check
> for, and I can't imagine it matters performance wise.

Yeah, makes sense.  I don't think it matters either for
pg_stat_statements in the same context.  So changed that part as
well.

>> @@ -63,8 +63,16 @@ Datum
>>  int2in(PG_FUNCTION_ARGS)
>>  {
>>      char       *num = PG_GETARG_CSTRING(0);
>> +    int16        res;
>> +    pg_strtoint_status status;
>>
>> -    PG_RETURN_INT16(pg_strtoint16(num));
>> +    /* Use a custom set of error messages here adapted to the data type */
>> +    status = pg_strtoint16(num, &res);
>
> I don't know what that comment is supposed to mean?

I mean here that the _check equivalent cannot be used as any error
messages generated need to be consistent with the SQL data type.  I
have updated the comment, does it look better now?

>> +/*
>> + * pg_strtoint64_check
>> + *
>> + * Convert input string to a signed 64-bit integer.
>> + *
>> + * This throws ereport() upon bad input format or overflow.
>> + */
>> +int64
>> +pg_strtoint64_check(const char *s)
>> +{
>> +    int64        result;
>> +    pg_strtoint_status status = pg_strtoint64(s, &result);
>> +
>> +    if (unlikely(status != PG_STRTOINT_OK))
>> +        pg_strtoint_error(status, s, "int64");
>> +    return result;
>> +}
>
> I think I'd just put these as inlines in the header.

I have not considered that.  This bloats a bit more builtins.h.  We
could live with that, or just move that into a separate header in
include/utils/, say int.h?  Even if common/int.h exists?

Attached is an updated patch.  Perhaps you have something else in
mind?
--
Michael