> On Mar 15, 2016, at 8:35 AM, Mark Dilger <hornschnor...@gmail.com> wrote: > > >> On Mar 14, 2016, at 5:12 PM, Vitaly Burovoy <vitaly.buro...@gmail.com> wrote: >> >> On 3/14/16, Mark Dilger <hornschnor...@gmail.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
I forgot to mention that I am not trying to block this commit. These were just my observations about the patch, for your consideration. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
