From: Tomas Vondra <tomas.vondra@enterprisedb.com>
To: John Naylor <john.naylor@enterprisedb.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Dmitry Dolgov <9erthalion6@gmail.com>
Cc: tharakan@gmail.com
Cc: pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #17774: Assert triggered on brin_minmax_multi.c
Date: Thu, 9 Feb 2023 17:18:47 +0100
Message-ID: <5a8acd6d-79ba-1017-e1df-46dd948347f1@enterprisedb.com> (raw)
In-Reply-To: <CAFBsxsG36dYXDFCNLsZ-muj1zKdBe-bB8ToOnXaCBp5uwQ8voA@mail.gmail.com>
References: <17774-c6f3e36dd4471e67@postgresql.org>
<CAFBsxsEMigeTVhBOpsBHw+SJS1n776akNqVC8ATEDmGo_OPgrw@mail.gmail.com>
<20230208100658.mz2kkeruoma6iqtw@ddolgov.remote.csb>
<3485884.1675868581@sss.pgh.pa.us>
<20230208153313.fym7at3ubgrec67a@ddolgov.remote.csb>
<3492169.1675872316@sss.pgh.pa.us>
<CAFBsxsG36dYXDFCNLsZ-muj1zKdBe-bB8ToOnXaCBp5uwQ8voA@mail.gmail.com>
On 2/9/23 04:53, John Naylor wrote:
>
> On Wed, Feb 8, 2023 at 11:05 PM Tom Lane <tgl@sss.pgh.pa.us
> <mailto:tgl@sss.pgh.pa.us>> wrote:
>>
>> Dmitry Dolgov <9erthalion6@gmail.com <mailto:9erthalion6@gmail.com>>
> writes:
>> >> On Wed, Feb 08, 2023 at 10:03:01AM -0500, Tom Lane wrote:
>> >> I'd believe this argument more readily if the calculation weren't being
>> >> done in float arithmetic. Since it is, you're at the mercy of roundoff
>> >> error ... and that small negative delta could certainly pass for
>> >> roundoff error.
>>
>> > Hmm...yeah, good point. In both the reproducer I've posted and the
>> > backtrace from the thread the delta is indeed rather small.
>>
>> I bet also it only fails when dealing with IPv6 addresses.
>> With 32-bit IPv4 addresses, a float8 would have enough mantissa
>> bits that the calculation wouldn't become imprecise.
>
> Addresses from different families are treated as a distance of one, so I
> don't think inserting a single IPv6 address would get this far, and in
> fact I still get a crash in an empty table by taking out the IPv6
> address from Dmitry's example:
>
> insert into brin_test values('127.0.0.1/0' <http://127.0.0.1/0';);
> insert into brin_test values('0.0.0.0/12' <http://0.0.0.0/12';);
>
> Adding some debug calls shows it starts off negative from the start:
>
> =# insert into brin_test values('127.0.0.1/0' <http://127.0.0.1/0';);
> INSERT 0 1
> =# insert into brin_test values('0.0.0.0/12' <http://0.0.0.0/12';);
> NOTICE: idx: 3 before div: -1.000000
> NOTICE: idx: 3 after div: -0.003906
> NOTICE: idx: 2 before div: -0.003906
> NOTICE: idx: 2 after div: -0.000015
> NOTICE: idx: 1 before div: -0.000015
> NOTICE: idx: 1 after div: -0.000000
> NOTICE: idx: 0 before div: -0.000000
> NOTICE: idx: 0 after div: -0.000000
>
> ...so something else is wrong here but I haven't dug deeper yet.
>
I believe the bug is pretty trivial - the code applies the netmask
incorrectly, so that with 127.0.0.1/0 it ends with 0.0.0.1, and because
it assumes 0.0.0.1 < 0.0.0.0 it ends with negative delta.
In particular, the issue is that the code does this:
lena = ip_bits(ipa); -- 0
len = ip_addrsize(ipa); -- 4
for (for (i = 0; i < len; i++)
{
nbits = lena - (i * 8);
...
mask = (0xFF << (8 - nbits));
...
}
But for 127.0.0.1/0 we get lena=0, so for i>0 nbits gets negative, and
the shift is probably going to do something silly (not sure what
exactly, but AFAICS it's undefined behavior).
Attached is a fixup that resolves this failure for me. I need to look a
bit closer if there are some other issues (e.g. with the float rounding
errors, etc.).
Good thing is this is this won't break anything without the assert - we
may pick incorrect ranges to merge, so the index is a bit less
efficient, but still valid/correct.
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Attachments:
[text/x-patch] brin-inet-fixup.patch (635B, ../5a8acd6d-79ba-1017-e1df-46dd948347f1@enterprisedb.com/2-brin-inet-fixup.patch)
download | inline diff:diff --git a/src/backend/access/brin/brin_minmax_multi.c b/src/backend/access/brin/brin_minmax_multi.cindex ac670fd02d7..be03d61c4ca 100644--- a/src/backend/access/brin/brin_minmax_multi.c+++ b/src/backend/access/brin/brin_minmax_multi.c@@ -2364,14 +2364,14 @@ brin_minmax_multi_distance_inet(PG_FUNCTION_ARGS)
unsigned char mask;
int nbits;
- nbits = lena - (i * 8);+ nbits = Max(0, lena - (i * 8));
if (nbits < 8)
{
mask = (0xFF << (8 - nbits));
addra[i] = (addra[i] & mask);
}
- nbits = lenb - (i * 8);+ nbits = Max(0, lenb - (i * 8));
if (nbits < 8)
{
mask = (0xFF << (8 - nbits));
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-bugs@postgresql.org
Cc: tomas.vondra@enterprisedb.com, john.naylor@enterprisedb.com, tgl@sss.pgh.pa.us, 9erthalion6@gmail.com, tharakan@gmail.com, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #17774: Assert triggered on brin_minmax_multi.c
In-Reply-To: <5a8acd6d-79ba-1017-e1df-46dd948347f1@enterprisedb.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox