Re: WIP: BRIN multi-range indexes - Mailing list pgsql-hackers

From Zhihong Yu
Subject Re: WIP: BRIN multi-range indexes
Date
Msg-id CALNJ-vRR21OrbrBJdAxUs=tZeK-cuAtPGv1Ak_Lnky9AEUtn1Q@mail.gmail.com
Whole thread Raw
In response to Re: WIP: BRIN multi-range indexes  (Tomas Vondra <tomas.vondra@enterprisedb.com>)
List pgsql-hackers
Hi,
bq. Not sure why would that be an issue

Moving the (start > end) check is up to your discretion.

But the midpoint computation should follow text book :-)

Cheers

On Wed, Feb 3, 2021 at 4:59 PM Tomas Vondra <tomas.vondra@enterprisedb.com> wrote:


On 2/4/21 1:49 AM, Zhihong Yu wrote:
> Hi,
> For 0007-Remove-the-special-batch-mode-use-a-larger--20210203.patch :
>
> +       /* same as preceding value, so store it */
> +       if (compare_values(&range->values[start + i - 1],
> +                          &range->values[start + i],
> +                          (void *) &cxt) == 0)
> +           continue;
> +
> +       range->values[start + n] = range->values[start + i];
>
> It seems the comment doesn't match the code: the value is stored when
> subsequent value is different from the previous.
>

Yeah, you're right the comment is wrong - the code is doing exactly the
opposite. I'll need to go through this more carefully.

> For has_matching_range():
> +       int     midpoint = (start + end) / 2;
>
> I think the standard notion for midpoint is start + (end-start)/2.
>
> +       /* this means we ran out of ranges in the last step */
> +       if (start > end)
> +           return false;
>
> It seems the above should be ahead of computation of midpoint.
>

Not sure why would that be an issue, as we're not using the value and
the values are just plain integers (so no overflows ...).


regards

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

pgsql-hackers by date:

Previous
From: "tsunakawa.takay@fujitsu.com"
Date:
Subject: RE: libpq debug log
Next
From: Mark Dilger
Date:
Subject: possibly outdated pg_file_read() errhint