Re: pgsql: Further code review for pg_lsn data type.

On 02/20/2014 09:47 AM, Andres Freund wrote:
> On 2014-02-20 08:25:01 +0200, Heikki Linnakangas wrote:
>> On 02/20/2014 02:56 AM, Andres Freund wrote:
>>> On 2014-02-19 15:10:52 +0000, Robert Haas wrote:
>>>> Change input function error messages to be more consistent with what is
>>>> done elsewhere. Remove a bunch of redundant type casts, so that the
>>>> compiler will warn us if we screw up. Don't pass LSNs by value on
>>>> platforms where a Datum is only 32 bytes, per buildfarm. Move macros
>>>> for packing and unpacking LSNs to pg_lsn.h so that we can include
>>>> access/xlogdefs.h, to avoid an unsatisfied dependency on XLogRecPtr.
>>>
>>> Hm, won't
>>> #define DatumGetLSN(X) ((XLogRecPtr) DatumGetInt64(X))
>>> #define LSNGetDatum(X) (Int64GetDatum((int64) (X)))
>>> possibly truncate the value if it's larger than 2^(63-1) as int is
>>> signed but XLogRecPtr is unsigned?
>>
>> No. Casting between unsigned and signed integers of same width doesn't lose
>> information. For example with 16-bit integers, casting unsigned 40000 to
>> signed gives -25536. Casting signed -25536 back to unsigned gives back
>> 40000.
>
> Are you sure?
>
> 6.3.1.3 Signed and unsigned integers, paragraph 3:
> "Otherwise, the new type is signed and the value cannot be represented
> in it; either the result is implementation-defined or an
> implementation-defined signal is raised."
>
> Afaik unsigned to signed always safe, but not the other way round?

Oh, that's interesting, I didn't know that. We do signed to unsigned
conversions in a few places:

$ grep -r -I PG_GETARG_INT . | grep uint
./src/backend/access/hash/hashfunc.c: return hash_uint32((int32)
PG_GETARG_INT16(0));
./src/backend/access/hash/hashfunc.c: return
hash_uint32(PG_GETARG_INT32(0));
./src/backend/utils/adt/varlena.c: uint32 value = (uint32)
PG_GETARG_INT32(0);
./src/backend/utils/adt/varlena.c: uint64 value = (uint64)
PG_GETARG_INT64(0);

And in fact, the SET_X_BYTES macros also work by casting the value to an
unsigned integer. So if signed -> unsigned is undefined, then the
behavior of IntXGetDatum macros is also undefined.

- Heikki