Re: BRIN minmax multi - incorrect distance for infinite timestamp/date
Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
From: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
To: Dean Rasheed <dean.a.rasheed@gmail.com>
Cc: Tomas Vondra <tomas.vondra@enterprisedb.com>, PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Date: 2023-10-16T09:25:24Z
Lists: pgsql-hackers
Thanks Tomas for bringing this discussion to hackers. On Fri, Oct 13, 2023 at 8:58 PM Dean Rasheed <dean.a.rasheed@gmail.com> wrote: > > On Fri, 13 Oct 2023 at 13:17, Tomas Vondra > <tomas.vondra@enterprisedb.com> wrote: > > > > I do plan to backpatch this, yes. I don't think there are many people > > affected by this (few people are using infinite dates/timestamps, but > > maybe the overflow could be more common). > > The example you gave is missing CREATE INDEX command. Is it "create index test_idx_a on test using brin(a);" Do already create indexes have this issue? Do they need to rebuilt after upgrading? > > OK, though I doubt that such values are common in practice. > > There's also an overflow problem in > brin_minmax_multi_distance_interval() though, and I think that's worse > because overflows there throw "interval out of range" errors, which > can prevent index creation or inserts. > > There's a patch (0009 in [1]) as part of the infinite interval work, > but that just uses interval_mi(), and so doesn't fix the > interval-out-of-range errors, except for infinite intervals, which are > treated as special cases, which I don't think is really necessary. > Right. I used interval_mi() to preserve the finite value behaviour as is. But ... > I think this should be rewritten to compute delta from ia and ib > without going via an intermediate Interval value. I.e., instead of > computing "result", just do something like > > dayfraction = (ib->time % USECS_PER_DAY) - (ia->time % USECS_PER_DAY); > days = (ib->time / USECS_PER_DAY) - (ia->time / USECS_PER_DAY); > days += (int64) ib->day - (int64) ia->day; > days += ((int64) ib->month - (int64) ia->month) * INT64CONST(30); > > then convert to double precision as it does now: > > delta = (double) days + dayfraction / (double) USECS_PER_DAY; > Given Tomas's explanation of how these functions are supposed to work, I think your suggestions is better. I was worried that above calculations may not produce the same result as the current code when there is no error because modulo and integer division are not distributive over subtraction. But it looks like together they behave as normal division which is distributive over subtraction. I couldn't find an example where that is not true. Tomas, you may want to incorporate this in your patch. If not, I will incorporate it in my infinite interval patchset in [1]. [1] https://www.postgresql.org/message-id/CAExHW5u1JE7dxK=WLzqhCszNToxQzJdieRmhREpW6r8w6kcRGQ@mail.gmail.com -- Best Wishes, Ashutosh Bapat
Commits
-
Fix minmax-multi distance for extreme interval values
- 0fa73c5cd050 14.10 landed
- 2fbb2fcb0c63 15.5 landed
- 924e0e2ee058 16.1 landed
- c6cf6d353c28 17.0 landed
-
Fix minmax-multi on infinite date/timestamp values
- 52c934cc1f2e 14.10 landed
- d04a9283b707 15.5 landed
- 497fc92086f8 16.1 landed
- 8da86d62a112 17.0 landed
-
Fix calculation in brin_minmax_multi_distance_date
- d1740e169d95 14.10 landed
- 088233f8db6c 15.5 landed
- e7965226d551 16.1 landed
- 394d51731495 17.0 landed
-
Fix overflow when calculating timestamp distance in BRIN
- 90c4da6d4392 14.10 landed
- daa7b0d7ce14 15.5 landed
- 0635fe02b426 16.1 landed
- b5489b75c6ce 17.0 landed