From: | Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> |
---|---|
To: | tgl(at)sss(dot)pgh(dot)pa(dot)us |
Cc: | adrian(dot)klaver(at)aklaver(dot)com, frazer(at)frazermclean(dot)co(dot)uk, pgsql-general(at)postgresql(dot)org |
Subject: | Re: Unexpected interval comparison |
Date: | 2017-04-04 08:15:03 |
Message-ID: | 20170404.171503.228263525.horiguchi.kyotaro@lab.ntt.co.jp |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-general |
Thank you for the comment.
At Mon, 03 Apr 2017 11:35:25 -0400, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote in <23053(dot)1491233725(at)sss(dot)pgh(dot)pa(dot)us>
> Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> writes:
> > Ok, the attached patch changes the result type of
> > interval_cmp_value from TimeOffset(=int64) to new 128 bit
> > LinearInterval. The value is hidden under the functions
> > interval_eq/ge.../cmp and all other stuff seems to use the
> > functions.
>
> Looking at this now ... why isn't the INT64_AU32 macro just
>
> #define INT64_AU32(i64) ((i64) >> 32)
>
> ? The business with subtracting and re-adding 1 seems unnecessary, and it
> also creates a risk of overflow with the minimum possible int64 value.
It is equivalent to "i64 / (1<<32)" except for -INT64_MAX.
INT64_AU32 gives the value for the first term in the following
polynomial.
(int64)INT64_AU32(i64) * (2^32) + (int64)INT64_AL32(i64) = i64
The previous expression intended to avoid decimal arithmetic, but
gcc optimizes the simple division better (using cmovns-add-sar)
than the current INT64_AU32 (jmp-sar) so I changed it. This
doesn't suffer overflow.
-#define INT64_AU32(i64) (((i64) < 0 ? (((i64) - 1) >> 32) + 1: ((i64) >> 32)))
+#define INT64_AU32(i64) ((i64) / (1LL<<32))
In summation of terms in 128bit multiplication expression, I
noticed that the value of the second term's lower 32bit loses MSB
for certain cases. I changed LINEARINTERVAL_ADD_INT64 to accept
the MSB (as the 65th bit) separately.
The first attached is the revised patch and the second is
temporary sanity check code for non-128bit environment code. (but
works only on 128 bit environment)
regards,
--
Kyotaro Horiguchi
NTT Open Source Software Center
Attachment | Content-Type | Size |
---|---|---|
0001-Fix-overflow-during-interval-comparison.patch | text/x-patch | 9.4 KB |
0002-sanity-check-code.patch | text/x-patch | 1.2 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | vinny | 2017-04-04 09:02:54 | Re: "Reverse" inheritance? |
Previous Message | Tim Uckun | 2017-04-04 07:12:40 | Re: "Reverse" inheritance? |