> On Mar 14, 2016, at 6:23 AM, David Steele <david@pgmasters.net> wrote:
>
> On 2/25/16 4:44 PM, Vitaly Burovoy wrote:
>
>> Added to the commitfest 2016-03.
>>
>> [CF] https://commitfest.postgresql.org/9/540/
>
> This looks like a fairly straight-forward bug fix (the size of the patch is deceptive because there a lot of new
testsincluded). It applies cleanly.
>
> Anastasia, I see you have signed up to review. Do you have an idea when you will get the chance to do that?
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,
rather than impose a runtime penalty. The #defines would be less brittle in the event, for
example, that the postgres epoch were ever changed.
Mark Dilger