Re: BRIN minmax multi - incorrect distance for infinite timestamp/date

On Wed, Oct 18, 2023 at 8:23 PM Tomas Vondra
<tomas(dot)vondra(at)enterprisedb(dot)com> wrote:

>
> I did use that many values to actually force "compaction" and merging of
> points into ranges. We only keep 32 values per page range, so with 2
> values we'll not build a range. You're right it may still trigger the
> overflow (we probably still calculate distances, I didn't realize that),
> but without the compaction we can't check the query plans.
>
> However, I agree 60 values may be a bit too much. And I realized we can
> reduce the count quite a bit by using the values_per_range option. We
> could set it to 8 (which is the minimum).
>

I haven't read BRIN code, except the comments in the beginning of the
file. From what you describe it seems we will store first 32 values as
is, but later as the number of values grow create ranges from those?
Please point me to the relevant source code/documentation. Anyway, if
we can reduce the number of rows we insert, that will be good.

> >
>
> Not sure what you mean by "crash". Yes, it doesn't hit an assert,
> because there's none when calculating distance for date. It however
> should fail in the query plan check due to the incorrect order of
> subtractions.
>
> Also, the commit message does not claim to fix overflow. In fact, it
> says it can't overflow ...
>

Reading the commit message
"Tests for overflows with dates and timestamps in BRIN ...

...

The new regression tests check this for date and timestamp data types.
It adds tables with data close to the allowed min/max values, and builds
a minmax-multi index on it."

I expected the CREATE INDEX statement to throw an error or fail the
"Assert(delta >= 0);" in brin_minmax_multi_distance_date(). But a
later commit mentions that the overflow is not possible.

> >
> > We may want to combine various test cases though. Like the test adding
> > infinity and extreme values could be combined. Also the number of
> > values it inserts in the table for the reasons stated above.
> >
>
> I prefer not to do that. I find it more comprehensible to keep the tests
> separate / testing different things. If the tests were expensive to
> setup or something like that, that'd be a different situation.

Fair enough.

> >>
> >
> > Right. Do we highlight that in the commit message so that the person
> > writing release notes picks it up from there?
> >
>
> Yes, I think I'll mention what impact each issue can have on indexes.

Thanks.

--
Best Wishes,
Ashutosh Bapat