Message-ID: <a4d5563f-b62a-61aa-8022-12eed3fbcf21@enterprisedb.com>
Date: Thu, 19 Oct 2023 11:01:44 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.15.1
Subject: Re: BRIN minmax multi - incorrect distance for infinite
 timestamp/date
Content-Language: en-US
To: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
Cc: Dean Rasheed <dean.a.rasheed@gmail.com>,
 PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
References: <eef0ea8c-4aaa-8d0d-027f-58b1f35dd170@enterprisedb.com>
 <CAEZATCWwqTdcBC1ee1uLp+KG4XZLmxFwEGX9y9sBLUR+OdQHeA@mail.gmail.com>
 <14c205c6-fbf4-d2f2-3ea8-a7d5c5d63180@enterprisedb.com>
 <CAEZATCUNjgmTfTJnkR+DMXfRbrm+bqJuEZ-fKRJF3X4mr=o3LQ@mail.gmail.com>
 <75de1895-3bd9-242e-cf13-3f5b0281e676@enterprisedb.com>
 <CAEZATCV0kY0gWCLChf_x_Drh4KUEKcmBPD+cggcSju6yHaRYYA@mail.gmail.com>
 <CAExHW5tsrfu3k5We1tr_ANaQ_0OJc6Zf2YMB+TkmB2PQwirAPQ@mail.gmail.com>
 <dd7fcf77-414b-b4b2-358a-0fc505255fba@enterprisedb.com>
 <CAExHW5vcq6mJJRH3wPytDataDw+HZgs-E7b=e+SXpWgo7BoTsg@mail.gmail.com>
 <31400145-6f71-a6b2-e371-048a8e1584ef@enterprisedb.com>
 <CAExHW5vi3Yjk_5vvwtZWrvy-ZWbx68hbET_GpyH5fjS=2KMZrw@mail.gmail.com>
 <575aa0fe-311d-d3d0-1f1a-abb1b9665a63@enterprisedb.com>
 <CAExHW5vThLf2BKHYfj5SJjsrotHe-zaDiZwS0k27BW_FP4nXaA@mail.gmail.com>
From: Tomas Vondra <tomas.vondra@enterprisedb.com>
In-Reply-To: 
 <CAExHW5vThLf2BKHYfj5SJjsrotHe-zaDiZwS0k27BW_FP4nXaA@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Archived-At: 
 <https://www.postgresql.org/message-id/a4d5563f-b62a-61aa-8022-12eed3fbcf21%40enterprisedb.com>
Precedence: bulk


On 10/19/23 06:32, Ashutosh Bapat wrote:
> On Wed, Oct 18, 2023 at 8:23 PM Tomas Vondra
> <tomas.vondra@enterprisedb.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.
> 

I don't think we have documentation other than what's at the beginning
of the file. What the comment tries to explain is that the summary has a
maximum size (32 values by default), and each value can be either a
point or a range. A point requires one value, range requires two. So we
accumulate values one by one - until 32 values that's fine. Once we get
33, we have to merge some of the points into ranges, and we do that in a
greedy way by distance.

For example, this may happen:

33 values
-> 31 values + 1 range  [requires 33]
-> 30 values + 1 range  [requires 32]
...

The exact steps depend on which values/ranges are picked for the merge,
of course. In any case, there's no difference between the initial 32
values and the values added later.

Does that explain the algorithm? I'm not against clarifying the comment,
of course.

>>>
>>
>> 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.
> 

Hmmm, yeah. The comment should mention the date doesn't have issue with
overflows, but other bugs.


regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company