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

On Wed, Sep 04, 2019 at 12:49:17PM +0200, Fabien COELHO wrote:
> From a compiler perspective, the (un)likely tip is potentially useful on any
> test. We know when parsing a that it is very unlikely that the string
> conversion would fail, so we can tell that, so that the compiler knows which
> branch it should optimize first.
>
> You can argue against that if the functions are inlined, because maybe the
> compiler would propagate the information, but for distinct functions
> compiled separately the information is useful at each level.

Hmm.  There has been an extra recent argument on the matter here:
https://www.postgresql.org/message-id/20190904090839.stp3madovtynq3px@alap3.anarazel.de

I am not sure that we should tackle that as part of the first
refactoring though, as what we'd want is first to put all the
interfaces in a single place we can deal with afterwards.

> Yep, you are right, now that the conversion functions does not error out a
> message, its failure can be used as a test.
>
> The version attached changes slightly the semantics, because on int
> overflows a double conversion will be attempted instead of erroring. I do
> not think that it is worth the effort of preserving the previous semantic of
> erroring.

Yes.  I would move things in this direction.  I may reconsider this
part again with more testing but switching from one to the other is
simple enough so let's keep the code as you suggest for now.

>> scanint8() only has one caller in the backend with your patch, and we
>> don't check after its return result in int8.c, so I would suggest to
>> move the error handling directly in int8in() and to remove scanint8().
>
> Ok.

As per the extra comments of upthread, this should use a switch
without a default.

> Before dealing with the 16/32 versions, which involve quite a significant
> amount of changes, I would want a clear message that "the 64 bit approach"
> is the model to follow.
>
> Moreover, I'm unsure how to rename the existing pg_strtoint32 and others
> which call ereport, if the name is used for the common error returning
> version.

Right, there was this part.  This brings also the point of having one
interface for the backend as all the error messages for the backend
are actually the same, with the most simple name being that:
pg_strtoint(value, size, error_ok).

This then calls all the sub-routines we have in src/common/.  There
were more suggestions here:
https://www.postgresql.org/message-id/20190729050539.d7mbjabcrlv7bxc3@alap3.anarazel.de

> I would not expect any significant performance difference when loading int8
> things because basically scanint8 has just been renamed pg_strtoint64 and
> made global, and that is more or less all. It might be a little bit slower
> because possible the compiler cannot inline the conversion, but on the other
> hand, the *likely hints and removed tests may marginaly help performance. I
> think that the only way to test performance significantly would be to write
> a specific program that loops over a conversion.

I would suspect a change for pg_strtouint64().
--
Michael