From: | Michael Paquier <michael(at)paquier(dot)xyz> |
---|---|
To: | Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr> |
Cc: | Andres Freund <andres(at)anarazel(dot)de>, 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-08-30 13:44:38 |
Message-ID: | 20190830134438.GA32558@paquier.xyz |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Fri, Aug 30, 2019 at 10:06:11AM +0200, Fabien COELHO wrote:
> Patch applies cleanly, compiles, "make check" ok, but the added functions
> are not used (yet).
Thanks.
> I think that factoring out comments is a good move.
>
> For symmetry and efficiency, ISTM that uint16 mul overflow could use uint32
> and uint32 could use uint64, and the division-based method be dropped in
> these cases.
Yes, the division would be worse than the other. What do you think
about using the previous module I sent and measure how long it takes
to evaluate the overflows in some specific cases N times in loops?
> Maybe I would add a comment before each new section telling about the type,
> eg:
>
> /*
> * UINT16
> */
> add/sub/mul uint16 functions.
Let's do as you suggest here.
> I would tend to commit working solutions per type rather than by
> installment, so that at least all added functions are actually used
> somewhere, but it does not matter much, really.
I prefer by section, with testing dedicated to each part properly
done so as we can move to the next parts.
> I was unsure that having int128 implies uint128 availability, so I did not
> use it.
The recent Ryu-floating point work has begun using them (see f2s.c and
d2s.c).
> The checking extension is funny, but ISTM that these checks should be (are
> already?) included in some standard sql test, which will test the macros
> from direct SQL operations:
Sure. But not for the unsigned part as there are no equivalent
in-core data types, still it is possible to trick things with signed
integer arguments. I found my toy useful to check test all
implementations consistently.
> fabien=# SELECT INT2 '1234512434343';
> ERROR: value "1234512434343" is out of range for type smallint
>
> Well, a quick look at "src/test/regress/sql/int2.sql" suggests that
> there the existing tests should be extended… :-(
We can tackle that separately. -32768 is perfectly legal for
smallint, and the test is wrong here.
--
Michael
From | Date | Subject | |
---|---|---|---|
Next Message | Tom Lane | 2019-08-30 13:54:13 | Re: pg_upgrade: Error out on too many command-line arguments |
Previous Message | Nikhil Sontakke | 2019-08-30 13:36:34 | Re: [HACKERS] PATCH: Batch/pipelining support for libpq |