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

On Thu, 12 Oct 2023 at 23:43, Tomas Vondra
<tomas(dot)vondra(at)enterprisedb(dot)com> wrote:
>
> Ashutosh Bapat reported me off-list a possible issue in how BRIN
> minmax-multi calculate distance for infinite timestamp/date values.
>
> The current code does this:
>
> if (TIMESTAMP_NOT_FINITE(dt1) || TIMESTAMP_NOT_FINITE(dt2))
> PG_RETURN_FLOAT8(0);
>

Yes indeed, that looks wrong. I noticed the same thing while reviewing
the infinite interval patch.

> so means infinite values are "very close" to any other value, and thus
> likely to be merged into a summary range. That's exactly the opposite of
> what we want to do, possibly resulting in inefficient indexes.
>

Is this only inefficient? Or can it also lead to wrong query results?

> Attached is a patch fixing this
>

I wonder if it's actually necessary to give infinity any special
handling at all for dates and timestamps. For those types, "infinity"
is actually just INT_MIN/MAX, which compares correctly with any finite
value, and will be much larger/smaller than any common value, so it
seems like it isn't necessary to give "infinite" values any special
treatment. That would be consistent with date_cmp() and
timestamp_cmp().

Something else that looks wrong about that BRIN code is that the
integer subtraction might lead to overflow -- it is subtracting two
integer values, then casting the result to float8. It should cast each
input before subtracting, more like brin_minmax_multi_distance_int8().

IOW, I think brin_minmax_multi_distance_date/timestamp could be made
basically identical to brin_minmax_multi_distance_int4/8.

Regards,
Dean