Re: [PATCH] Integer overflow in timestamp[tz]_part() and date/time boundaries check

> On Mar 14, 2016, at 5:12 PM, Vitaly Burovoy <vitaly(dot)burovoy(at)gmail(dot)com> wrote:
>
> On 3/14/16, Mark Dilger <hornschnorter(at)gmail(dot)com> wrote:
>> The first thing I notice about this patch is that
>> src/include/datatype/timestamp.h
>> has some #defines that are brittle. The #defines have comments explaining
>> their logic, but I'd rather embed that in the #define directly:
>>
>> This:
>>
>> +#ifdef HAVE_INT64_TIMESTAMP
>> +#define MIN_TIMESTAMP INT64CONST(-211813488000000000)
>> +/* == (0 - POSTGRES_EPOCH_JDATE) * 86400 * USECS_PER_SEC */
>> +#define MAX_TIMESTAMP INT64CONST(9223371331200000000)
>> +/* == (JULIAN_MAX4STAMPS - POSTGRES_EPOCH_JDATE) * 86400 * USECS_PER_SEC
>> */
>> +#else
>> +#define MIN_TIMESTAMP -211813488000.0
>> +/* == (0 - POSTGRES_EPOCH_JDATE) * 86400 */
>> +#define MAX_TIMESTAMP 9223371331200.0
>> +/* == (JULIAN_MAX4STAMPS - POSTGRES_EPOCH_JDATE) * 86400 */
>> +#endif
>>
>> Could be written as:
>>
>> #ifdef HAVE_INT64_TIMESTAMP
>> #define MIN_TIMESTAMP
>> ((INT64CONST(0) - POSTGRES_EPOCH_JDATE) * USECS_PER_DAY)
>> #define MAX_TIMESTAMP
>> ((JULIAN_MAX4STAMPS - POSTGRES_EPOCH_JDATE) * USECS_PER_DAY)
>> #else
>> #define MIN_TIMESTAMP
>> ((INT64CONST(0) - POSTGRES_EPOCH_JDATE) * SECS_PER_DAY)
>> #define MAX_TIMESTAMP
>> ((JULIAN_MAX4STAMPS - POSTGRES_EPOCH_JDATE) * SECS_PER_DAY)
>> #endif
>>
>> I assume modern compilers would convert these to the same constants at
>> compile-time,
>
> Firstly, Postgres is compiling not only by modern compilers.

Do you have an example of a compiler that will not do this constant folding
at compile time?

>> rather than impose a runtime penalty.
>
> Secondary, It is hard to write it correctly obeying Postgres' coding
> standard (e.g. 80-columns border) and making it clear: it is not so
> visual difference between USECS_PER_DAY and SECS_PER_DAY in the
> expressions above (but it is vivid in comments in the file).

Hmm. I think using USECS_PER_DAY is perfectly clear, but that is a personal
opinion. I don't see any way to argue if you don't see it that way.

> Also a
> questions raise "Why is INT64CONST(0) necessary in `#else' block"
> (whereas `float' is necessary there)

You appear to be correct. The second half should be defined in terms of
float. I compiled on a system with int64 support, so I didn't notice. Thanks
for catching that.

> or "Why is INT64CONST set for
> MIN_TIMESTAMP, but not for MAX_TIMESTAMP?" (JULIAN_MAX4STAMPS is _not_
> int64).

I was only casting the zero to int64. That doesn't seem necessary, so it can
be removed. Both MIN_TIMESTAMP and MAX_TIMESTAMP were defined
in terms of USECS_PER_DAY, which itself is defined in terms of INT64CONST,
so I believe they both work out to be an int64 constant.

> The file is full of constants. For example, JULIAN_MAX and
> SECS_PER_DAY are one of them.

JULIAN_MAXYEAR and JULIAN_MAXYEAR4STAMPS appear to be magic
numbers with no explanation. I would not point to them as examples to be
followed.

> I would leave it as is.
>
>> The #defines would be less brittle in
>> the event, for example, that the postgres epoch were ever changed.
>
> I don't think it is real, and even in such case all constants are
> collected together in the file and will be found and changed at once.

I agree that they would be found at once. I disagree that the example
is not real, as I have changed the postgres epoch myself in some builds,
to be able to use int32 timestamps on small devices.

Regards,
Mark Dilger